Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD

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

On 03/12/2013 12:34 PM, Jan Beulich wrote:
>>>> On 12.03.13 at 16:32, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> MSR_IA32_MCx_MISC(4) register on AMD processors is used for error
>> thresholding. PV guests may try to set it up for threshold
>> interrupts which will fail and result in these warnings in the log:
>>
>>    [Firmware Bug]: cpu 0, try to use APIC510 (LVT offset 1) for vector
>>    0xf9, but the register is already in use for vector 0x0 on this cpu
>>
>> Mark this register as invalid to avoid this. While at it, also present
>> other MSR_IA32_MCx_MISC() registers as invalid (except for the first
>> GUEST_MC_BANK_NUM which are emulated).
> Hmm, I'm not convinced. A PV guest shouldn't, by definition, try to
> set up APIC LVTs (or else it is only partially PV).

In Linux, bank 4 is assumed to support LVT interrupts
(lvt_interrupt_supported()) and that leads to the guest trying to set it
up via mce_amd_feature_init()->setup_APIC_mce()->setup_APIC_eilvt().

>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>> @@ -166,6 +166,7 @@ static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr)
>>           case MSR_F10_MC4_MISC1:
>>           case MSR_F10_MC4_MISC2:
>>           case MSR_F10_MC4_MISC3:
>> +        case MSR_IA32_MCx_MISC(GUEST_MC_BANK_NUM)...MSR_IA32_MCx_MISC(6):
> And if we take this, then I'd like to see an explanation of the magic
> 6 here, including rationale why going forward there wouldn't be a
> need to bump this to 7, 8, etc.

As of today, 6 banks are supported (although bank #6, MSR0000_041B, 
appears to
be a stub and in fact APM lists only 5 banks).

I suppose we can look at MCG_CAP[BANK_CNT]. Is that what you are suggesting.

Note though that as of today, only the first 5 bank MSRs are 
architectural since that's
all that we have in APM. In theory, AMD may decide to place the ones 
above 5 anywhere.
I'd be surprised if they did but there have been precedents.

> Plus, even if it happens to work, it's not intended for architectural
> MSRs to be dealt with in mce_vendor_bank_msr() (as that code is
> expected to match the default cases in bank_mce_{rd,wr}msr()).
> In other words, the change as is would create a latent bug.

Not sure I follow this. My understanding is that mce_vendor_bank_msr() 
is there to
indicate that the register is a bank register and should be dealt with 
in bank_mce_rdmsr().
And with this change bank_mce_rdmsr() will indeed deal with it by 
returning 0.

-boris