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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.03.2021 10:08, Roger Pau Monné wrote:
> On Fri, Mar 12, 2021 at 08:54:46AM +0100, Jan Beulich wrote:
>> Prior to 4.15 Linux, when running in PV mode, did not install a #GP
>> handler early enough to cover for example the rdmsrl_safe() of
>> MSR_K8_TSEG_ADDR in bsp_init_amd() (not to speak of the unguarded read
>> of MSR_K7_HWCR later in the same function). The respective change
>> (42b3a4cb5609 "x86/xen: Support early interrupts in xen pv guests") was
>> backported to 4.14, but no further - presumably since it wasn't really
>> easy because of other dependencies.
>>
>> Therefore, to prevent our change in the handling of guest MSR accesses
>> to render PV Linux 4.13 and older unusable on at least AMD systems, make
>> the raising of #GP on this paths conditional upon the guest having
>> installed a handler, provided of course the MSR can be read in the first
>> place (we would have raised #GP in that case even before). Producing
>> zero for reads isn't necessarily correct and may trip code trying to
>> detect presence of MSRs early, but since such detection logic won't work
>> without a #GP handler anyway, this ought to be a fair workaround.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Release-Acked-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> 
> I think the approach is fine:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -874,7 +874,8 @@ 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;
>> +    uint64_t tmp;
>>      int ret;
>>  
>>      if ( (ret = guest_rdmsr(curr, reg, val)) != X86EMUL_UNHANDLEABLE )
>> @@ -882,7 +883,7 @@ static int read_msr(unsigned int reg, ui
>>          if ( ret == X86EMUL_EXCEPTION )
>>              x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
> 
> You might want to move the injection of the exception to the done
> label?
> 
> So that we can avoid the call to x86_emul_reset_event.

At the expense of slightly more code churn, yes, perhaps. I have
to admit though that it feels less prone to future issues to me
to have an unconditional x86_emul_reset_event() on that path.

>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -143,6 +143,12 @@ typedef unsigned long xen_ulong_t;
>>   *  Level == 1: Kernel may enter
>>   *  Level == 2: Kernel may enter
>>   *  Level == 3: Everyone may enter
>> + *
>> + * Note: For compatibility with kernels not setting up exception handlers
>> + *       early enough, Xen will avoid trying to inject #GP (and hence crash
>> + *       the domain) when an RDMSR would require this, but no handler was
>> + *       set yet. The precise conditions are implementation specific, and
> 
> You can drop the 'yet' here I think? As even if a handler has been set
> and then removed we would still prevent injecting a #GP AFAICT. Not a
> native speaker anyway, so I might be wrong on that one.

Well, I've put it there intentionally to leave room (effectively
trying to further emphasize "implementation specific") for us to
indeed only behave this way if no handler was ever set (as
opposed to a handler having got set and then zapped again).

>> + *       new code shouldn't rely on such behavior anyway.
> 
> I would use a stronger mustn't here instead of shouldn't.

Fine with me. I've been using "mustn't" in a number of cases in
the past and was told I'm using it too often, sounding sort of
impolite. I guess I'll switch to "may not", which was suggested
to me as the better replacement.

Jan