Re: [PATCH v2 1/2][4.15] x86/PV: conditionally avoid raising #GP for early guest MSR reads

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.03.2021 14:37, Roger Pau Monné wrote:
> On Tue, Mar 09, 2021 at 12:16:49PM +0100, Jan Beulich wrote:
>> On 09.03.2021 11:17, Roger Pau Monné wrote:
>>> On Mon, Mar 08, 2021 at 02:49:19PM +0100, Jan Beulich wrote:
>>>> On 08.03.2021 13:11, Roger Pau Monné wrote:
>>>>> On Mon, Mar 08, 2021 at 10:33:12AM +0100, Jan Beulich wrote:
>>>>>> On 08.03.2021 09:56, Roger Pau Monné wrote:
>>>>>>> On Fri, Mar 05, 2021 at 10:50:34AM +0100, Jan Beulich wrote:
>>>>>>>> --- a/xen/arch/x86/pv/emul-priv-op.c
>>>>>>>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>>>>>>>> @@ -874,7 +874,7 @@ static int read_msr(unsigned int reg, ui
>>>>>>>>      struct vcpu *curr = current;
>>>>>>>>      const struct domain *currd = curr->domain;
>>>>>>>>      const struct cpuid_policy *cp = currd->arch.cpuid;
>>>>>>>> -    bool vpmu_msr = false;
>>>>>>>> +    bool vpmu_msr = false, warn = false;
>>>>>>>>      int ret;
>>>>>>>>  
>>>>>>>>      if ( (ret = guest_rdmsr(curr, reg, val)) != X86EMUL_UNHANDLEABLE )
>>>>>>>> @@ -882,7 +882,7 @@ static int read_msr(unsigned int reg, ui
>>>>>>>>          if ( ret == X86EMUL_EXCEPTION )
>>>>>>>>              x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
>>>>>>>>  
>>>>>>>> -        return ret;
>>>>>>>> +        goto done;
>>>>>>>>      }
>>>>>>>>  
>>>>>>>>      switch ( reg )
>>>>>>>> @@ -986,7 +986,7 @@ static int read_msr(unsigned int reg, ui
>>>>>>>>          }
>>>>>>>>          /* fall through */
>>>>>>>>      default:
>>>>>>>> -        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", reg);
>>>>>>>> +        warn = true;
>>>>>>>>          break;
>>>>>>>>  
>>>>>>>>      normal:
>>>>>>>> @@ -995,7 +995,19 @@ static int read_msr(unsigned int reg, ui
>>>>>>>>          return X86EMUL_OKAY;
>>>>>>>>      }
>>>>>>>>  
>>>>>>>> -    return X86EMUL_UNHANDLEABLE;
>>>>>>>> + done:
>>>>>>>
>>>>>>> Won't this handling be better placed in the 'default' switch case
>>>>>>> above?
>>>>>>
>>>>>> No - see the "goto done" added near the top of the function.
>>>>>
>>>>> Yes, I'm not sure of that. If guest_rdmsr returns anything different
>>>>> than X86EMUL_UNHANDLEABLE it means it has handled the MSR in some way,
>>>>> and hence we shouldn't check whether the #GP handler is set or not.
>>>>>
>>>>> This is not the behavior of older Xen versions, so I'm unsure whether
>>>>> we should introduce a policy that's even less strict than the previous
>>>>> one in regard to whether a #GP is injected or not.
>>>>>
>>>>> I know injecting a #GP when the handler is not set is not helpful for
>>>>> the guest, but we should limit the workaround to kind of restoring the
>>>>> previous behavior for making buggy guests happy, not expanding it
>>>>> anymore.
>>>>
>>>> Yet then we risk breaking guests with any subsequent addition to
>>>> guest_rdmsr() which, under whatever extra conditions, wants to
>>>> raise #GP.
>>>
>>> But it's always been like that AFAICT? Additions to guest_{rd/wr}msr
>>> preventing taking the default path in the {read/write}_msr PV
>>> handlers.
>>
>> Yes, but the impact so far and the impact going forward are different.
> 
> OK, I assume this is because we plan to handle more MSRs in
> guest_{rd/wr}msr?

Yes.

> In which case those newly added handlers are not likely to inject a
> #GP?

Kind of the opposite - because it's not impossible that some
addition there may want to raise #GP.

>>> If #GP signaled by guest_{rd/wr}msr are no longer injected when the guest
>>> #GP handler is not set we might as well drop the rdmsr_safe check and
>>> just don't try to inject any #GP at all from MSR accesses unless the
>>> handler is setup?
>>
>> Well, that's what I had initially. You asked me to change to what I
>> have now.
>>
>>> I feel this is likely going too far. I agree we should attempt to
>>> restore something compatible with the previous behavior for unhandled
>>> MSRs, but also not injecting the #GPs signaled by guest_{rd/wr}msr
>>> seems to go beyond that.
>>
>> I understand this is a downside. Yet as said - the downside of _not_
>> doing so is that every further raising of #GP will risk breaking a
>> random guest kernel variant.
> 
> Right. So given this awkward position Xen is in, we should maybe make
> the lack of #GP injection as a result of an MSR access when no handler
> is set formally part of the ABI and written down somewhere?
> 
> It's not ideal, but at the end of day PV is 'our' own architecture,
> and given that this workaround will be enabled by default, and that we
> won't be able to turn it off we should have it written down as part of
> the ABI.
> 
> If you agree with this I'm fine with not injecting a #GP at all unless
> the handler is set for PV, like you proposed in your first patch. IMO
> it's not ideal, but it's better if it's a consistent behavior and
> clearly written down in the public headers (likely next to the
> hypercall used to setup the #GP handler).
> 
> I know this can be seen as broken behavior from an x86 perspective,
> but again PV is already different from x86.

I'm certainly not opposed to spelling this out somewhere; iirc you
said the other day that you couldn't spot a good place. I can't think
of a good place either. Furthermore before we spell out anything we
(which specifically includes Andrew) need to settle on the precise
behavior we want. I did suggest earlier that I could see us tighten
the condition, and there are many possible variations. For example we
could record whether a #GP handler was ever installed, so we wouldn't
return back to the relaxed behavior in case a guest zapped its handler
again. But for behavior like this the immediate question is going to
be what effect migration (or saving/restoring) of the guest ought to
have.

Jan