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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan