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 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.

> 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.

Jan