* [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD
@ 2013-03-12 15:32 Boris Ostrovsky
2013-03-12 15:43 ` Egger Christoph
2013-03-12 16:34 ` Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Boris Ostrovsky @ 2013-03-12 15:32 UTC (permalink / raw)
To: chegger; +Cc: bp, boris.ostrovsky, xen-devel
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).
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
xen/arch/x86/cpu/mcheck/mce.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index f2aeacb..d6526a4 100644
--- 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):
return 1;
}
break;
--
1.8.1.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD
2013-03-12 15:32 [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD Boris Ostrovsky
@ 2013-03-12 15:43 ` Egger Christoph
2013-03-12 16:34 ` Jan Beulich
1 sibling, 0 replies; 6+ messages in thread
From: Egger Christoph @ 2013-03-12 15:43 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: bp, xen-devel
On 12.03.13 16:32, Boris Ostrovsky 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).
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Christoph Egger <chegger@amazon.de>
> ---
> xen/arch/x86/cpu/mcheck/mce.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
> index f2aeacb..d6526a4 100644
> --- 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):
> return 1;
> }
> break;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD
2013-03-12 15:32 [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD Boris Ostrovsky
2013-03-12 15:43 ` Egger Christoph
@ 2013-03-12 16:34 ` Jan Beulich
2013-03-12 17:00 ` Boris Ostrovsky
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2013-03-12 16:34 UTC (permalink / raw)
To: chegger, boris.ostrovsky; +Cc: bp, xen-devel
>>> 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).
> --- 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.
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.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD
2013-03-12 16:34 ` Jan Beulich
@ 2013-03-12 17:00 ` Boris Ostrovsky
2013-03-13 7:41 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Boris Ostrovsky @ 2013-03-12 17:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: bp, chegger, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD
2013-03-12 17:00 ` Boris Ostrovsky
@ 2013-03-13 7:41 ` Jan Beulich
2013-03-13 9:16 ` Egger Christoph
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2013-03-13 7:41 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: bp, chegger, xen-devel
>>> On 12.03.13 at 18:00, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> 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().
So for the PV case this call chain needs to be broken at some
suitable point. Nothing LAPIC related should be done in a PV
kernel.
>>> --- 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.
Yes, that sounds like the right thing.
>> 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().
No. CTL, STATUS, ADDR, and MISC aren't vendor specific, and
hence should be dealt with in bank_mce_rdmsr() in any case. The
upper bound here is intentionally
MSR_IA32_MCx_CTL(v->arch.vmce.mcg_cap & MCG_CAP_COUNT)
as that's what the guest gets announced through MCG_CAP.
> And with this change bank_mce_rdmsr() will indeed deal with it by
> returning 0.
But the guest shouldn't be accessing higher banks' MSRs, as it
never was told these banks exist. Or else we have a problem
_there_, not in the handling of the MSR accesses.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD
2013-03-13 7:41 ` Jan Beulich
@ 2013-03-13 9:16 ` Egger Christoph
0 siblings, 0 replies; 6+ messages in thread
From: Egger Christoph @ 2013-03-13 9:16 UTC (permalink / raw)
To: Jan Beulich; +Cc: bp, Boris Ostrovsky, xen-devel
On 13.03.13 08:41, Jan Beulich wrote:
>>>> On 12.03.13 at 18:00, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> 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().
>
> So for the PV case this call chain needs to be broken at some
> suitable point. Nothing LAPIC related should be done in a PV
> kernel.
>
>>>> --- 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.
>
> Yes, that sounds like the right thing.
>
>>> 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().
>
> No. CTL, STATUS, ADDR, and MISC aren't vendor specific, and
> hence should be dealt with in bank_mce_rdmsr() in any case. The
> upper bound here is intentionally
>
> MSR_IA32_MCx_CTL(v->arch.vmce.mcg_cap & MCG_CAP_COUNT)
>
> as that's what the guest gets announced through MCG_CAP.
>
>> And with this change bank_mce_rdmsr() will indeed deal with it by
>> returning 0.
>
> But the guest shouldn't be accessing higher banks' MSRs, as it
> never was told these banks exist. Or else we have a problem
> _there_, not in the handling of the MSR accesses.
Good point Jan. My fault was I only considered the HVM part.
I fully agree with you and withdraw my Acked-by.
Christoph
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-13 9:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-12 15:32 [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD Boris Ostrovsky
2013-03-12 15:43 ` Egger Christoph
2013-03-12 16:34 ` Jan Beulich
2013-03-12 17:00 ` Boris Ostrovsky
2013-03-13 7:41 ` Jan Beulich
2013-03-13 9:16 ` Egger Christoph
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).