Re: [PATCH v2 2/2][4.15] x86/AMD: expose HWCR.TscFreqSel to guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2021 12:37, Roger Pau Monné wrote:
> On Fri, Mar 05, 2021 at 10:50:54AM +0100, Jan Beulich wrote:
>> Linux has been warning ("firmware bug") about this bit being clear for a
>> long time. While writable in older hardware it has been readonly on more
>> than just most recent hardware. For simplicitly report it always set (if
>> anything we may want to log the issue ourselves if it turns out to be
>> clear on older hardware).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> One question below.
> 
>> ---
>> v2: New.
>> ---
>> There are likely more bits worthwhile to expose, but for about every one
>> of them there would be the risk of a lengthy discussion, as there are
>> clear downsides to exposing such information, the more that it would be
>> tbd whether the hardware values should be surfaced, and if so what
>> should happen when the guest gets migrated.
>>
>> The main risk with making the read not fault here is that guests might
>> imply they can also write this MSR then.
>>
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -315,6 +315,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t
>>          *val = msrs->tsc_aux;
>>          break;
>>  
>> +    case MSR_K8_HWCR:
>> +        if ( !(cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
>> +            goto gp_fault;
>> +        *val = K8_HWCR_TSC_FREQ_SEL;
> 
> I've been only able to find information about this MSR up to family
> 10h, but I think in theory Xen might also run on family 0Fh, do you
> know if the MSR is present there, and the bit has the same meaning?

>From its name (and its K7 alternative name) it's clear the register
had been there at that point. And indeed the bit has a different
meaning there (its the bottom bit of a 6-bit START_FID field if the
BKDG I'm looking at can be trusted. Since I don't think it matters
much whether we expose a value of 0x00 or a value of 0x01 there,
and since we likely don't want to make #GP raising dependent upon
family when we don't _really_ need to, I would want to propose that
the value used is good enough uniformly.

>> --- a/xen/include/asm-x86/msr-index.h
>> +++ b/xen/include/asm-x86/msr-index.h
>> @@ -287,6 +287,8 @@
>>  
>>  #define MSR_K7_HWCR                 0xc0010015
> 
> We could likely drop the K7 define here, as Xen won't be able to run
> on K7 hardware anymore AFAICT.

Indeed, but not at this point in time.

Jan