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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 1/22/21 6:51 AM, Jan Beulich wrote:
> On 20.01.2021 23:49, Boris Ostrovsky wrote:
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -295,7 +295,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>          }
>>  
>>          /* Fallthrough. */
>> -    case 0x40000200 ... 0x400002ff:
>> +    case 0x40000200 ... 0x400002fe:
>>          ret = guest_rdmsr_xen(v, msr, val);
>>          break;
>>  
>> @@ -514,7 +514,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>>          }
>>  
>>          /* Fallthrough. */
>> -    case 0x40000200 ... 0x400002ff:
>> +    case 0x40000200 ... 0x400002fe:
>>          ret = guest_wrmsr_xen(v, msr, val);
>>          break;
> 
> For both of these, we need some kind of connection to
> MSR_UNHANDLED. Could be a BUILD_BUG_ON() or an explicit
> "case MSR_UNHANDLED:" (preventing someone "correcting" the
> apparent mistake) or yet something else.


I actually meant to add a comment there but forgot.

I'll add an explicit 'case'.


> 
>> --- 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:
>> + *  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           0x400002ff
> 
> MSR indexes as well as definitions for MSR contents generally
> live in asm-x86/msr-index.h. I think it would be better for
> the above to also go there.
> 


I didn't put it there because I felt that file is for "real" (including hypervisor range) MSRs. But I can move it to msr-index.h


-boris