Re: [PATCH v2 5/8] x86/pv: allow reading FEATURE_CONTROL MSR

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.08.2020 17:12, Roger Pau Monné wrote:
> On Thu, Aug 27, 2020 at 05:53:16PM +0200, Jan Beulich wrote:
>> On 20.08.2020 17:08, Roger Pau Monne wrote:
>>> @@ -181,6 +182,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
>>> *val)
>>>          /* Not offered to guests. */
>>>          goto gp_fault;
>>>  
>>> +    case MSR_IA32_FEATURE_CONTROL:
>>> +        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
>>> +            goto gp_fault;
>>
>> Can we really do it this way round, rather than raising #GP when
>> we know the MSR isn't there (AMD / Hygon)? I realized code e.g.
>> ...
>>
>>> +        *val = IA32_FEATURE_CONTROL_LOCK;
>>> +        if ( vmce_has_lmce(v) )
>>> +            *val |= IA32_FEATURE_CONTROL_LMCE_ON;
>>> +        if ( nestedhvm_enabled(d) )
>>> +            *val |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
>>> +        break;
>>> +
>>> +
>>>      case MSR_IA32_PLATFORM_ID:
>>>          if ( !(cp->x86_vendor & X86_VENDOR_INTEL) ||
>>>               !(boot_cpu_data.x86_vendor & X86_VENDOR_INTEL) )
>>
>> ... in context right here does it the same way, but I still
>> wonder whether we wouldn't better switch existing instances, too.
> 
> Hm, no idea really. Right now it seems better to check for != Intel
> rather than AMD | Hygon | Centaur | Shanghai, as that's a MSR specific
> to Intel.
> 
> Do those MSRs exist in Centaur / Shanghai?

I can't tell for sure, but I suppose they do, as they're (in a way)
cloning Intel's implementation. My thinking here is that by not
exposing an MSR when we should we potentially cause guests to crash.
Whereas by exposing an MSR that ought not be there fallout would
result only if the vendor actually has something else at that index.
And I'd hope vendors apply some care to avoid doing so.

Jan