* [PATCH 0/2] Fix MCE not cleared on reset
@ 2010-07-07 11:09 Avi Kivity
2010-07-07 11:09 ` [PATCH 1/2] KVM: Expose MCE control MSRs to userspace Avi Kivity
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Avi Kivity @ 2010-07-07 11:09 UTC (permalink / raw)
To: kvm; +Cc: Marcelo Tosatti, Huang Ying
Injecting an MCE will often panic the guest kernel. After reset, however,
a new MCE will not be recognized because kvm thinks the first one is still
being processed.
Fix by exposing the MCE MSRs to userspace, which will clear them on reset.
Avi Kivity (2):
KVM: Expose MCE control MSRs to userspace
KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST
Documentation/kvm/api.txt | 4 ++++
arch/x86/kvm/x86.c | 2 ++
2 files changed, 6 insertions(+), 0 deletions(-)
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] KVM: Expose MCE control MSRs to userspace 2010-07-07 11:09 [PATCH 0/2] Fix MCE not cleared on reset Avi Kivity @ 2010-07-07 11:09 ` Avi Kivity 2010-07-08 2:07 ` Huang Ying 2010-07-07 11:09 ` [PATCH 2/2] KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST Avi Kivity 2010-07-12 18:05 ` [PATCH 0/2] Fix MCE not cleared on reset Marcelo Tosatti 2 siblings, 1 reply; 11+ messages in thread From: Avi Kivity @ 2010-07-07 11:09 UTC (permalink / raw) To: kvm; +Cc: Marcelo Tosatti, Huang Ying Userspace needs to reset and save/restore these MSRs. The MCE banks are not exposed since their number varies from vcpu to vcpu. Signed-off-by: Avi Kivity <avi@redhat.com> --- arch/x86/kvm/x86.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 7070b41..1e12cc5 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -744,6 +744,8 @@ static unsigned num_msrs_to_save; static u32 emulated_msrs[] = { MSR_IA32_MISC_ENABLE, + MSR_IA32_MCG_STATUS, + MSR_IA32_MCG_CTL, }; static int set_efer(struct kvm_vcpu *vcpu, u64 efer) -- 1.7.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace 2010-07-07 11:09 ` [PATCH 1/2] KVM: Expose MCE control MSRs to userspace Avi Kivity @ 2010-07-08 2:07 ` Huang Ying 2010-07-08 7:43 ` Avi Kivity 0 siblings, 1 reply; 11+ messages in thread From: Huang Ying @ 2010-07-08 2:07 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, Marcelo Tosatti Hi, Avi, On Wed, 2010-07-07 at 19:09 +0800, Avi Kivity wrote: > Userspace needs to reset and save/restore these MSRs. > > The MCE banks are not exposed since their number varies from vcpu to vcpu. > > Signed-off-by: Avi Kivity <avi@redhat.com> > --- > arch/x86/kvm/x86.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 7070b41..1e12cc5 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -744,6 +744,8 @@ static unsigned num_msrs_to_save; > > static u32 emulated_msrs[] = { > MSR_IA32_MISC_ENABLE, > + MSR_IA32_MCG_STATUS, > + MSR_IA32_MCG_CTL, We need only clear MSR_IA32_MCG_STATUS during reset, but should not clear MSR_IA32_MCG_CTL. > }; > > static int set_efer(struct kvm_vcpu *vcpu, u64 efer) Best Regards, Huang Ying ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace 2010-07-08 2:07 ` Huang Ying @ 2010-07-08 7:43 ` Avi Kivity 2010-07-08 8:03 ` Huang Ying 0 siblings, 1 reply; 11+ messages in thread From: Avi Kivity @ 2010-07-08 7:43 UTC (permalink / raw) To: Huang Ying; +Cc: kvm, Marcelo Tosatti On 07/08/2010 05:07 AM, Huang Ying wrote: > >> static u32 emulated_msrs[] = { >> MSR_IA32_MISC_ENABLE, >> + MSR_IA32_MCG_STATUS, >> + MSR_IA32_MCG_CTL, >> > We need only clear MSR_IA32_MCG_STATUS during reset, but should not > clear MSR_IA32_MCG_CTL. > > Why not? -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace 2010-07-08 7:43 ` Avi Kivity @ 2010-07-08 8:03 ` Huang Ying 2010-07-08 8:16 ` Avi Kivity 0 siblings, 1 reply; 11+ messages in thread From: Huang Ying @ 2010-07-08 8:03 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, Marcelo Tosatti On Thu, 2010-07-08 at 15:43 +0800, Avi Kivity wrote: > On 07/08/2010 05:07 AM, Huang Ying wrote: > > > >> static u32 emulated_msrs[] = { > >> MSR_IA32_MISC_ENABLE, > >> + MSR_IA32_MCG_STATUS, > >> + MSR_IA32_MCG_CTL, > >> > > We need only clear MSR_IA32_MCG_STATUS during reset, but should not > > clear MSR_IA32_MCG_CTL. > > > > > > Why not? According to Intel 64 and IA32 Architectures Software Developer's Manual (SDM) Vol 3A (Table 9-1), machine check MSRs should be sticky across reset. Except we need some special processing for MSR_IA32_MCG_STATUS. And if we clear MSR_IA32_MCG_CTL, the machine check reporting is disabled according to SDM Vol 3A, section 15.3.1.3 Best Regards, Huang Ying ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace 2010-07-08 8:03 ` Huang Ying @ 2010-07-08 8:16 ` Avi Kivity 2010-07-08 8:47 ` Huang Ying 0 siblings, 1 reply; 11+ messages in thread From: Avi Kivity @ 2010-07-08 8:16 UTC (permalink / raw) To: Huang Ying; +Cc: kvm, Marcelo Tosatti On 07/08/2010 11:03 AM, Huang Ying wrote: > On Thu, 2010-07-08 at 15:43 +0800, Avi Kivity wrote: > >> On 07/08/2010 05:07 AM, Huang Ying wrote: >> >>> >>>> static u32 emulated_msrs[] = { >>>> MSR_IA32_MISC_ENABLE, >>>> + MSR_IA32_MCG_STATUS, >>>> + MSR_IA32_MCG_CTL, >>>> >>>> >>> We need only clear MSR_IA32_MCG_STATUS during reset, but should not >>> clear MSR_IA32_MCG_CTL. >>> >>> >>> >> Why not? >> > According to Intel 64 and IA32 Architectures Software Developer's Manual > (SDM) Vol 3A (Table 9-1), machine check MSRs should be sticky across > reset. Well, my copy says: Machine-Check Architecture Pwr up or Reset: Undefined INIT: Unchanged So it seems we're free to clear it. But probably the best thing would be to have the bios initialize it to disabled state? Note, in any case we have to expose the MSRs for live migration. > Except we need some special processing for MSR_IA32_MCG_STATUS. > What do you have in mind? > And if we clear MSR_IA32_MCG_CTL, the machine check reporting is > disabled according to SDM Vol 3A, section 15.3.1.3 > Won't the kernel reenable MCE? In my testing, the sequence MCE-reset-MCE worked after the patch (whereas it would fail without it). -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace 2010-07-08 8:16 ` Avi Kivity @ 2010-07-08 8:47 ` Huang Ying 2010-07-08 9:12 ` Avi Kivity 0 siblings, 1 reply; 11+ messages in thread From: Huang Ying @ 2010-07-08 8:47 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, Marcelo Tosatti On Thu, 2010-07-08 at 16:16 +0800, Avi Kivity wrote: > On 07/08/2010 11:03 AM, Huang Ying wrote: > > On Thu, 2010-07-08 at 15:43 +0800, Avi Kivity wrote: > > > >> On 07/08/2010 05:07 AM, Huang Ying wrote: > >> > >>> > >>>> static u32 emulated_msrs[] = { > >>>> MSR_IA32_MISC_ENABLE, > >>>> + MSR_IA32_MCG_STATUS, > >>>> + MSR_IA32_MCG_CTL, > >>>> > >>>> > >>> We need only clear MSR_IA32_MCG_STATUS during reset, but should not > >>> clear MSR_IA32_MCG_CTL. > >>> > >>> > >>> > >> Why not? > >> > > According to Intel 64 and IA32 Architectures Software Developer's Manual > > (SDM) Vol 3A (Table 9-1), machine check MSRs should be sticky across > > reset. > > > Well, my copy says: > > Machine-Check Architecture Pwr up or Reset: Undefined > INIT: Unchanged If my understanding is correct. Pwr up or Reset is cold reboot and INIT is warm reboot. So MCE can be logged after the warm reboot. > So it seems we're free to clear it. But probably the best thing would > be to have the bios initialize it to disabled state? > > Note, in any case we have to expose the MSRs for live migration. > > > Except we need some special processing for MSR_IA32_MCG_STATUS. > > > > What do you have in mind? MSR_IA32_MCG_STATUS need to be cleared across reboot. > > And if we clear MSR_IA32_MCG_CTL, the machine check reporting is > > disabled according to SDM Vol 3A, section 15.3.1.3 > > > > Won't the kernel reenable MCE? In my testing, the sequence > MCE-reset-MCE worked after the patch (whereas it would fail without it). Yes. Because kernel will reenable it. But if we only clear MSR_IA32_MCG_STATUS only, MCE-reset-MCE should work too. Best Regards, Huang Ying ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace 2010-07-08 8:47 ` Huang Ying @ 2010-07-08 9:12 ` Avi Kivity 2010-07-12 0:10 ` Huang Ying 0 siblings, 1 reply; 11+ messages in thread From: Avi Kivity @ 2010-07-08 9:12 UTC (permalink / raw) To: Huang Ying; +Cc: kvm, Marcelo Tosatti On 07/08/2010 11:47 AM, Huang Ying wrote: > On Thu, 2010-07-08 at 16:16 +0800, Avi Kivity wrote: > >> On 07/08/2010 11:03 AM, Huang Ying wrote: >> >>> On Thu, 2010-07-08 at 15:43 +0800, Avi Kivity wrote: >>> >>> >>>> On 07/08/2010 05:07 AM, Huang Ying wrote: >>>> >>>> >>>>> >>>>> >>>>>> static u32 emulated_msrs[] = { >>>>>> MSR_IA32_MISC_ENABLE, >>>>>> + MSR_IA32_MCG_STATUS, >>>>>> + MSR_IA32_MCG_CTL, >>>>>> >>>>>> >>>>>> >>>>> We need only clear MSR_IA32_MCG_STATUS during reset, but should not >>>>> clear MSR_IA32_MCG_CTL. >>>>> >>>>> >>>>> >>>>> >>>> Why not? >>>> >>>> >>> According to Intel 64 and IA32 Architectures Software Developer's Manual >>> (SDM) Vol 3A (Table 9-1), machine check MSRs should be sticky across >>> reset. >>> >> >> Well, my copy says: >> >> Machine-Check Architecture Pwr up or Reset: Undefined >> INIT: Unchanged >> > If my understanding is correct. Pwr up or Reset is cold reboot and INIT > is warm reboot. So MCE can be logged after the warm reboot. > Yes. But we're allowed to clear it on a real RESET, just not on INIT. Our INIT handing is probably broken in this area. >> So it seems we're free to clear it. But probably the best thing would >> be to have the bios initialize it to disabled state? >> >> Note, in any case we have to expose the MSRs for live migration. >> >> >>> Except we need some special processing for MSR_IA32_MCG_STATUS. >>> >>> >> What do you have in mind? >> > MSR_IA32_MCG_STATUS need to be cleared across reboot. > Ok. >>> And if we clear MSR_IA32_MCG_CTL, the machine check reporting is >>> disabled according to SDM Vol 3A, section 15.3.1.3 >>> >>> >> Won't the kernel reenable MCE? In my testing, the sequence >> MCE-reset-MCE worked after the patch (whereas it would fail without it). >> > Yes. Because kernel will reenable it. But if we only clear > MSR_IA32_MCG_STATUS only, MCE-reset-MCE should work too. > What happens if we reboot into a kernel that doesn't enable MCE? I guess it doesn't matter: the new kernel will keep cr4.mce cleared and thus MCE will be blocked. I'd like to keep the patch as is, so live migration works for MCE (we'll need to add bank support). I think there's no problem clearing _CTL on reset. If there is, we can patch qemu not to clear the MSR. Is that acceptable? -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: Expose MCE control MSRs to userspace 2010-07-08 9:12 ` Avi Kivity @ 2010-07-12 0:10 ` Huang Ying 0 siblings, 0 replies; 11+ messages in thread From: Huang Ying @ 2010-07-12 0:10 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, Marcelo Tosatti, Andi Kleen Sorry for late. On Thu, 2010-07-08 at 17:12 +0800, Avi Kivity wrote: [..snip..] > >>> And if we clear MSR_IA32_MCG_CTL, the machine check reporting is > >>> disabled according to SDM Vol 3A, section 15.3.1.3 > >>> > >>> > >> Won't the kernel reenable MCE? In my testing, the sequence > >> MCE-reset-MCE worked after the patch (whereas it would fail without it). > >> > > Yes. Because kernel will reenable it. But if we only clear > > MSR_IA32_MCG_STATUS only, MCE-reset-MCE should work too. > > > > What happens if we reboot into a kernel that doesn't enable MCE? > > I guess it doesn't matter: the new kernel will keep cr4.mce cleared and > thus MCE will be blocked. > > I'd like to keep the patch as is, so live migration works for MCE (we'll > need to add bank support). I think there's no problem clearing _CTL on > reset. If there is, we can patch qemu not to clear the MSR. Is that > acceptable? Yes. At least for Linux that is OK. Don't know whether Windows will set _CTL during boot. Best Regards, Huang Ying ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST 2010-07-07 11:09 [PATCH 0/2] Fix MCE not cleared on reset Avi Kivity 2010-07-07 11:09 ` [PATCH 1/2] KVM: Expose MCE control MSRs to userspace Avi Kivity @ 2010-07-07 11:09 ` Avi Kivity 2010-07-12 18:05 ` [PATCH 0/2] Fix MCE not cleared on reset Marcelo Tosatti 2 siblings, 0 replies; 11+ messages in thread From: Avi Kivity @ 2010-07-07 11:09 UTC (permalink / raw) To: kvm; +Cc: Marcelo Tosatti, Huang Ying Signed-off-by: Avi Kivity <avi@redhat.com> --- Documentation/kvm/api.txt | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt index d9b00f1..179cc51 100644 --- a/Documentation/kvm/api.txt +++ b/Documentation/kvm/api.txt @@ -126,6 +126,10 @@ user fills in the size of the indices array in nmsrs, and in return kvm adjusts nmsrs to reflect the actual number of msrs and fills in the indices array with their numbers. +Note: if kvm indicates supports MCE (KVM_CAP_MCE), then the MCE bank MSRs are +not returned in the MSR list, as different vcpus can have a different number +of banks, as set via the KVM_X86_SETUP_MCE ioctl. + 4.4 KVM_CHECK_EXTENSION Capability: basic -- 1.7.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix MCE not cleared on reset 2010-07-07 11:09 [PATCH 0/2] Fix MCE not cleared on reset Avi Kivity 2010-07-07 11:09 ` [PATCH 1/2] KVM: Expose MCE control MSRs to userspace Avi Kivity 2010-07-07 11:09 ` [PATCH 2/2] KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST Avi Kivity @ 2010-07-12 18:05 ` Marcelo Tosatti 2 siblings, 0 replies; 11+ messages in thread From: Marcelo Tosatti @ 2010-07-12 18:05 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, Huang Ying On Wed, Jul 07, 2010 at 02:09:37PM +0300, Avi Kivity wrote: > Injecting an MCE will often panic the guest kernel. After reset, however, > a new MCE will not be recognized because kvm thinks the first one is still > being processed. > > Fix by exposing the MCE MSRs to userspace, which will clear them on reset. > > Avi Kivity (2): > KVM: Expose MCE control MSRs to userspace > KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST > > Documentation/kvm/api.txt | 4 ++++ > arch/x86/kvm/x86.c | 2 ++ > 2 files changed, 6 insertions(+), 0 deletions(-) Applied, thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-07-12 19:17 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-07-07 11:09 [PATCH 0/2] Fix MCE not cleared on reset Avi Kivity 2010-07-07 11:09 ` [PATCH 1/2] KVM: Expose MCE control MSRs to userspace Avi Kivity 2010-07-08 2:07 ` Huang Ying 2010-07-08 7:43 ` Avi Kivity 2010-07-08 8:03 ` Huang Ying 2010-07-08 8:16 ` Avi Kivity 2010-07-08 8:47 ` Huang Ying 2010-07-08 9:12 ` Avi Kivity 2010-07-12 0:10 ` Huang Ying 2010-07-07 11:09 ` [PATCH 2/2] KVM: Document MCE banks non-exposure via KVM_GET_MSR_INDEX_LIST Avi Kivity 2010-07-12 18:05 ` [PATCH 0/2] Fix MCE not cleared on reset Marcelo Tosatti
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).