Re: [PATCH 2/4] x86: Introduce MSR_UNHANDLED

[boris.ostrovsky@xxxxxxxxxx]

On 1/8/21 9:55 AM, Jan Beulich wrote:
> On 07.01.2021 21:34, Boris Ostrovsky wrote:
>> --- a/xen/include/xen/lib/x86/msr.h
>> +++ b/xen/include/xen/lib/x86/msr.h
>> @@ -2,8 +2,21 @@
>>  #ifndef XEN_LIB_X86_MSR_H
>>  #define XEN_LIB_X86_MSR_H
>>  
>> +/*
>> + * Behavior on accesses to MSRs that are not handled by emulation:
> What about ones handled by emulation, when emulation decides to
> raise #GP and this still causes trouble to a guest? (Slightly
> orthogonal, so we may want to defer this aspect.)


Yes, that's a good point --- in some cases the situation is somewhat similar, 
e.g. when a new bit shows up in an MSR that until now was considered invalid.


>
>> + *  0 = return #GP, warning emitted
>> + *  1 = read as 0, writes are dropped, no warning
>> + *  2 = read as 0, writes are dropped, warning emitted
>> + */
>> +#define MSR_UNHANDLED_NEVER     0
>> +#define MSR_UNHANDLED_SILENT    1
>> +#define MSR_UNHANDLED_VERBOSE   2
>> +
>> +/* MSR that is not explicitly processed by emulation */
>> +#define MSR_UNHANDLED -1
> Not sure about this choice: I'd consider an MSR index in the Xen
> range more safe to use, not the least because this value
> effectively becomes part of the migration ABI. Or if you use one
> outside, then maybe a less prominent one than 0xffffffff (I
> guess -1, irrespective of the missing parentheses, isn't
> appropriate to use here).


All MSRs in Xen range are currently handled (although most return an 
exception). I can reserve the last one (0x400002ff) if you feel it's more 
appropriate.


>
>> @@ -77,7 +78,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
>>          if ( copy_from_buffer_offset(&data, msrs, i, 1) )
>>              return -EFAULT;
>>  
>> -        if ( data.flags ) /* .flags MBZ */
>> +        if ( data.idx != MSR_UNHANDLED && data.flags )
> You permit all flags bits set here, but ...
>
>> @@ -101,6 +102,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
>>  
>>          case MSR_INTEL_PLATFORM_INFO: ASSIGN(platform_info.raw); break;
>>          case MSR_ARCH_CAPABILITIES:   ASSIGN(arch_caps.raw);     break;
>> +        case MSR_UNHANDLED:           p->ignore_msrs = data.flags & 0xff;   
>>     break;
> ... you use only the low 8 ones here. You want to forbid any we
> don't know of, and even restrict the low two ones to just the
> three values you define. E.g. for now
>
>         if ( data.idx != MSR_UNHANDLED ? data.flags
>                                        : data.flags > MSR_UNHANDLED_VERBOSE )
>         {
>             rc = -EINVAL;
>             goto err;
>
> Otoh I don't see why you need to use flags here - I think you
> could as well use the MSR value to convey the setting. And if
> you don't, you'd want to check the value to be zero.


Sure, using value will result in a slightly smaller diffstat too.



-boris