* [PATCH] Fix hypervisor crash when writing to VPMU MSR @ 2017-04-20 17:49 Mohit Gambhir 2017-04-20 17:49 ` [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault Mohit Gambhir 0 siblings, 1 reply; 7+ messages in thread From: Mohit Gambhir @ 2017-04-20 17:49 UTC (permalink / raw) To: jun.nakajima, kevin.tian, xen-devel; +Cc: boris.ostrovsky, Mohit Gambhir In order to address the concerns raised in XSA-163, I am writing XTF based tests to validate PMU MSR read/writes from HVM guests. While testing, I found a scenario where setting the Pin Control Flag bit (19) of IA32_PERF_EVTSELx results in a General Protection Fault followed by a hypervisor crash. While Intel SDM Vol 3B, Section 18.2.1.1 Architectural Performance Monitoring Version 1 Facilities, describes the bit functionality, it is unclear why the fault happens. There are two possible solutions to prevent the hypervisor from crashing: 1. Mask the PC bit in the VPMU so as to not allow any writes to it from guests on any Intel machine. 2. Use wrmsr_safe() function to write to IA32_PERF_EVTSELx register and return any resulting fault to the guest OS. The attached patch uses solution 2 so as to not disable PC flag bit on machines that do not fault. Mohit Gambhir (1): x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault xen/arch/x86/cpu/vpmu_intel.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) -- 2.9.3 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault 2017-04-20 17:49 [PATCH] Fix hypervisor crash when writing to VPMU MSR Mohit Gambhir @ 2017-04-20 17:49 ` Mohit Gambhir 2017-04-21 7:14 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Mohit Gambhir @ 2017-04-20 17:49 UTC (permalink / raw) To: jun.nakajima, kevin.tian, xen-devel; +Cc: boris.ostrovsky, Mohit Gambhir This patch changes wrmsrl() calls to write to MSR_P6_EVTSEL register in the VPMU to wrmsr_safe(). There are known (and possibly some unknown) cases where setting certain bits in MSR_P6_EVTSEL reg. can cause a General Protection fault on some machines. Unless we catch this fault when it happens, it will result in a hypervisor crash. For instance, setting Pin Control (PC) bit (19) in MSR_P6_EVNTSEL results in a General Protection Fault on Broadwell machines and thus causes the hypervisor to crash. This patch fixes the above mentioned crash (and other possible hypervisor crashes that may occur while writing MSR_P6_EVNTSEL reg) by catching and returning the fault to the guest OS. Signed-off-by: Mohit Gambhir <mohit.gambhir@oracle.com> --- xen/arch/x86/cpu/vpmu_intel.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c index 3f0322c..13808b5 100644 --- a/xen/arch/x86/cpu/vpmu_intel.c +++ b/xen/arch/x86/cpu/vpmu_intel.c @@ -338,7 +338,7 @@ static int core2_vpmu_save(struct vcpu *v, bool_t to_guest) return 1; } -static inline void __core2_vpmu_load(struct vcpu *v) +static inline int __core2_vpmu_load(struct vcpu *v) { unsigned int i, pmc_start; struct xen_pmu_intel_ctxt *core2_vpmu_cxt = vcpu_vpmu(v)->context; @@ -356,7 +356,9 @@ static inline void __core2_vpmu_load(struct vcpu *v) for ( i = 0; i < arch_pmc_cnt; i++ ) { wrmsrl(pmc_start + i, xen_pmu_cntr_pair[i].counter); - wrmsrl(MSR_P6_EVNTSEL(i), xen_pmu_cntr_pair[i].control); + if ( wrmsr_safe(MSR_P6_EVNTSEL(i), xen_pmu_cntr_pair[i].control) == + -EFAULT) + return -EFAULT; } wrmsrl(MSR_CORE_PERF_FIXED_CTR_CTRL, core2_vpmu_cxt->fixed_ctrl); @@ -369,6 +371,7 @@ static inline void __core2_vpmu_load(struct vcpu *v) core2_vpmu_cxt->global_ovf_ctrl = 0; wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, core2_vpmu_cxt->global_ctrl); } + return 0; } static int core2_vpmu_verify(struct vcpu *v) @@ -461,9 +464,8 @@ static int core2_vpmu_load(struct vcpu *v, bool_t from_guest) vpmu_set(vpmu, VPMU_CONTEXT_LOADED); - __core2_vpmu_load(v); + return __core2_vpmu_load(v); - return 0; } static int core2_vpmu_alloc_resource(struct vcpu *v) @@ -538,7 +540,8 @@ static int core2_vpmu_msr_common_check(u32 msr_index, int *type, int *index) /* Do the lazy load staff. */ if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) ) { - __core2_vpmu_load(current); + if ( __core2_vpmu_load(current) ) + return 0; vpmu_set(vpmu, VPMU_CONTEXT_LOADED); if ( is_hvm_vcpu(current) && cpu_has_vmx_msr_bitmap ) @@ -719,8 +722,11 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, } } - if ( type != MSR_TYPE_GLOBAL ) - wrmsrl(msr, msr_content); + if ( type != MSR_TYPE_GLOBAL) + { + if ( wrmsr_safe(msr, msr_content) == -EFAULT ) + return -EFAULT; + } else { if ( is_hvm_vcpu(v) ) -- 2.9.3 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault 2017-04-20 17:49 ` [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault Mohit Gambhir @ 2017-04-21 7:14 ` Jan Beulich 2017-04-24 15:44 ` Mohit Gambhir 0 siblings, 1 reply; 7+ messages in thread From: Jan Beulich @ 2017-04-21 7:14 UTC (permalink / raw) To: Mohit Gambhir; +Cc: kevin.tian, boris.ostrovsky, jun.nakajima, xen-devel >>> On 20.04.17 at 19:49, <mohit.gambhir@oracle.com> wrote: > This patch changes wrmsrl() calls to write to MSR_P6_EVTSEL register in the > VPMU to wrmsr_safe(). There are known (and possibly some unknown) cases where > setting certain bits in MSR_P6_EVTSEL reg. can cause a General Protection > fault on some machines. Unless we catch this fault when it happens, it will > result in a hypervisor crash. > > For instance, setting Pin Control (PC) bit (19) in MSR_P6_EVNTSEL results > in a General Protection Fault on Broadwell machines and thus causes the > hypervisor to crash. I can't seem to find indication of this in the SDM. Is this removal of a valid bit from an architectural MSR documented anywhere? > @@ -356,7 +356,9 @@ static inline void __core2_vpmu_load(struct vcpu *v) > for ( i = 0; i < arch_pmc_cnt; i++ ) > { > wrmsrl(pmc_start + i, xen_pmu_cntr_pair[i].counter); > - wrmsrl(MSR_P6_EVNTSEL(i), xen_pmu_cntr_pair[i].control); > + if ( wrmsr_safe(MSR_P6_EVNTSEL(i), xen_pmu_cntr_pair[i].control) == > + -EFAULT) > + return -EFAULT; If the function already returns an error code, please use it to avoid a latent bug when in the future it may also return other than -EFAULT. > @@ -369,6 +371,7 @@ static inline void __core2_vpmu_load(struct vcpu *v) > core2_vpmu_cxt->global_ovf_ctrl = 0; > wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, core2_vpmu_cxt->global_ctrl); > } > + return 0; Blank line above the main return statement of a function please. > @@ -461,9 +464,8 @@ static int core2_vpmu_load(struct vcpu *v, bool_t > from_guest) > > vpmu_set(vpmu, VPMU_CONTEXT_LOADED); > > - __core2_vpmu_load(v); > + return __core2_vpmu_load(v); > > - return 0; > } Please also remove the now stray blank line. > @@ -538,7 +540,8 @@ static int core2_vpmu_msr_common_check(u32 msr_index, int *type, int *index) > /* Do the lazy load staff. */ > if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) ) > { > - __core2_vpmu_load(current); > + if ( __core2_vpmu_load(current) ) > + return 0; Nothing you need to change, but the change here point out that the function really needs switching to returning bool. > @@ -719,8 +722,11 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, > } > } > > - if ( type != MSR_TYPE_GLOBAL ) > - wrmsrl(msr, msr_content); > + if ( type != MSR_TYPE_GLOBAL) > + { > + if ( wrmsr_safe(msr, msr_content) == -EFAULT ) > + return -EFAULT; See the earlier similar comment. Further a more general question: Why do you do this only for MSR_P6_EVNTSEL*, but not consistently for all MSRs which are being written with (partially) guest controlled values? I'd namely expect all wrmsrl() to be switched over, unless it is clear that we're dealing with an erratum here and we therefore can assume that normal guarantees for architectural MSRs apply to everything else. And finally, in the description you talk about forwarding the fault - I can see this being the case for core2_vpmu_do_wrmsr(), but how does this match up with __core2_vpmu_load() and its callers? Namely you may report an error there after already having updated some MSRs. Leaving the overall set of MSRs in an inconsistent state is not very likely to be a good idea. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault 2017-04-21 7:14 ` Jan Beulich @ 2017-04-24 15:44 ` Mohit Gambhir 2017-04-24 16:00 ` Boris Ostrovsky 2017-04-24 16:04 ` Jan Beulich 0 siblings, 2 replies; 7+ messages in thread From: Mohit Gambhir @ 2017-04-24 15:44 UTC (permalink / raw) To: Jan Beulich; +Cc: kevin.tian, boris.ostrovsky, jun.nakajima, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 5413 bytes --] On 04/21/2017 03:14 AM, Jan Beulich wrote: >>>> On 20.04.17 at 19:49,<mohit.gambhir@oracle.com> wrote: >> This patch changes wrmsrl() calls to write to MSR_P6_EVTSEL register in the >> VPMU to wrmsr_safe(). There are known (and possibly some unknown) cases where >> setting certain bits in MSR_P6_EVTSEL reg. can cause a General Protection >> fault on some machines. Unless we catch this fault when it happens, it will >> result in a hypervisor crash. >> >> For instance, setting Pin Control (PC) bit (19) in MSR_P6_EVNTSEL results >> in a General Protection Fault on Broadwell machines and thus causes the >> hypervisor to crash. > I can't seem to find indication of this in the SDM. Is this removal of a > valid bit from an architectural MSR documented anywhere? It see it as an observed hardware bug so I don't expect it to be found in the SDM. I reached out to the folks at Intel (CC) but they are unaware of it. Ideally it should work the way it says in the SDM for all events and on all machines but it doesn't - not for the all the events and certainly not on all machines. Also, from the description in the SDM, PC flag bit it seems very disruptive to me. SDM says that if the bit is set then the processor toggles the PMi pin (generating a performance monitoring interrupt?) every time the event occurs. So if we program the counter to count "unhaulted core cycles", and set PC flag bit it will generate an interrupts every cycle!? >> @@ -356,7 +356,9 @@ static inline void __core2_vpmu_load(struct vcpu *v) >> for ( i = 0; i < arch_pmc_cnt; i++ ) >> { >> wrmsrl(pmc_start + i, xen_pmu_cntr_pair[i].counter); >> - wrmsrl(MSR_P6_EVNTSEL(i), xen_pmu_cntr_pair[i].control); >> + if ( wrmsr_safe(MSR_P6_EVNTSEL(i), xen_pmu_cntr_pair[i].control) == >> + -EFAULT) >> + return -EFAULT; > If the function already returns an error code, please use it to avoid > a latent bug when in the future it may also return other than -EFAULT. Fixed in next version. >> @@ -369,6 +371,7 @@ static inline void __core2_vpmu_load(struct vcpu *v) >> core2_vpmu_cxt->global_ovf_ctrl = 0; >> wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, core2_vpmu_cxt->global_ctrl); >> } >> + return 0; > Blank line above the main return statement of a function please. > pv_inject_hw_exception(TRAP_gp_fault Fixed in next version. >> @@ -461,9 +464,8 @@ static int core2_vpmu_load(struct vcpu *v, bool_t >> from_guest) >> >> vpmu_set(vpmu, VPMU_CONTEXT_LOADED); >> >> - __core2_vpmu_load(v); >> + return __core2_vpmu_load(v); >> >> - return 0; >> } > Please also remove the now stray blank line. > Fixed in next version. >> @@ -538,7 +540,8 @@ static int core2_vpmu_msr_common_check(u32 msr_index, int *type, int *index) >> /* Do the lazy load staff. */ >> if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) ) >> { >> - __core2_vpmu_load(current); >> + if ( __core2_vpmu_load(current) ) >> + return 0; > Nothing you need to change, but the change here point out that > the function really needs switching to returning bool. Agreed. >> @@ -719,8 +722,11 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, >> } >> } >> >> - if ( type != MSR_TYPE_GLOBAL ) >> - wrmsrl(msr, msr_content); >> + if ( type != MSR_TYPE_GLOBAL) >> + { >> + if ( wrmsr_safe(msr, msr_content) == -EFAULT ) >> + return -EFAULT; > See the earlier similar comment. Fixed in next verison. > Further a more general question: Why do you do this only for > MSR_P6_EVNTSEL*, but not consistently for all MSRs which are > being written with (partially) guest controlled values? I'd namely > expect all wrmsrl() to be switched over, unless it is clear that > we're dealing with an erratum here and we therefore can > assume that normal guarantees for architectural MSRs apply to > everything else. I think it is an erratum for one specific MSR which is MSR_P6_EVTSEL > And finally, in the description you talk about forwarding the > fault - I can see this being the case for core2_vpmu_do_wrmsr(), > but how does this match up with __core2_vpmu_load() and its > callers? Namely you may report an error there after already > having updated some MSRs. Leaving the overall set of MSRs in > an inconsistent state is not very likely to be a good idea. Yes, that's something I did not think about and needs to be fixed. There are two issues here - 1. What should we set MSR values in case of an error? We can not revert them because if it is part of a context switch then we do not want to leak the values from the previous guest to the new guest. So one possible solution is to wipe all PMU MSRs (set them to 0) before returning the error. 2. __core2_vpmu_load is called in two scenarios - One from a hyper call XENPMU_FLUSH from do xenpmu_op() and the other during a context switch from vpmu_switch_to(). The hypercall seems to be handling the error correctly but vpmu_switch_to() ignores the error. So to propagate the error up to the guest kernel maybe we should inject the fault into the guest using hvm_inject_hw_exception(TRAP_gp_fault, error)/ pv_inject_hw_exception(TRAP_gp_fault, error)? > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel [-- Attachment #1.2: Type: text/html, Size: 7881 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault 2017-04-24 15:44 ` Mohit Gambhir @ 2017-04-24 16:00 ` Boris Ostrovsky 2017-04-24 18:49 ` Mohit Gambhir 2017-04-24 16:04 ` Jan Beulich 1 sibling, 1 reply; 7+ messages in thread From: Boris Ostrovsky @ 2017-04-24 16:00 UTC (permalink / raw) To: Mohit Gambhir, Jan Beulich; +Cc: kevin.tian, jun.nakajima, xen-devel > > Also, from the description in the SDM, PC flag bit it seems very > disruptive to me. > SDM says that if the bit is set then the processor toggles the PMi pin > (generating a performance monitoring interrupt?) > every time the event occurs. So if we program the counter to count > "unhaulted core cycles", and set PC flag bit > it will generate an interrupts every cycle!? I checked how Linux treats this bit and there is this interesting commit: commit a7b9d2ccc3d86303ee9314612d301966e04011c7 Author: Gleb Natapov <gleb@redhat.com> Date: Sun Feb 26 16:55:40 2012 +0200 KVM: PMU: warn when pin control is set in eventsel msr Print warning once if pin control bit is set in eventsel msr since emulation does not support it yet. Signed-off-by: Gleb Natapov <gleb@redhat.com> Signed-off-by: Avi Kivity <avi@redhat.com> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h index 096c975..f1f7182 100644 --- a/arch/x86/include/asm/perf_event.h +++ b/arch/x86/include/asm/perf_event.h @@ -23,6 +23,7 @@ #define ARCH_PERFMON_EVENTSEL_USR (1ULL << 16) #define ARCH_PERFMON_EVENTSEL_OS (1ULL << 17) #define ARCH_PERFMON_EVENTSEL_EDGE (1ULL << 18) +#define ARCH_PERFMON_EVENTSEL_PIN_CONTROL (1ULL << 19) #define ARCH_PERFMON_EVENTSEL_INT (1ULL << 20) #define ARCH_PERFMON_EVENTSEL_ANY (1ULL << 21) #define ARCH_PERFMON_EVENTSEL_ENABLE (1ULL << 22) diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c index 3e48c1d..6af9a54 100644 --- a/arch/x86/kvm/pmu.c +++ b/arch/x86/kvm/pmu.c @@ -210,6 +210,9 @@ static void reprogram_gp_counter(struct kvm_pmc *pmc, u64 eventsel) unsigned config, type = PERF_TYPE_RAW; u8 event_select, unit_mask; + if (eventsel & ARCH_PERFMON_EVENTSEL_PIN_CONTROL) + printk_once("kvm pmu: pin control bit is ignored\n"); + pmc->eventsel = eventsel; stop_counter(pmc); -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault 2017-04-24 16:00 ` Boris Ostrovsky @ 2017-04-24 18:49 ` Mohit Gambhir 0 siblings, 0 replies; 7+ messages in thread From: Mohit Gambhir @ 2017-04-24 18:49 UTC (permalink / raw) To: Boris Ostrovsky, Jan Beulich; +Cc: kevin.tian, jun.nakajima, xen-devel On 04/24/2017 12:00 PM, Boris Ostrovsky wrote: >> Also, from the description in the SDM, PC flag bit it seems very >> disruptive to me. >> SDM says that if the bit is set then the processor toggles the PMi pin >> (generating a performance monitoring interrupt?) >> every time the event occurs. So if we program the counter to count >> "unhaulted core cycles", and set PC flag bit >> it will generate an interrupts every cycle!? > I checked how Linux treats this bit and there is this interesting commit: > > > commit a7b9d2ccc3d86303ee9314612d301966e04011c7 > Author: Gleb Natapov <gleb@redhat.com> > Date: Sun Feb 26 16:55:40 2012 +0200 > > KVM: PMU: warn when pin control is set in eventsel msr > > Print warning once if pin control bit is set in eventsel msr since > emulation does not support it yet. > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > Signed-off-by: Avi Kivity <avi@redhat.com> > > diff --git a/arch/x86/include/asm/perf_event.h > b/arch/x86/include/asm/perf_event.h > index 096c975..f1f7182 100644 > --- a/arch/x86/include/asm/perf_event.h > +++ b/arch/x86/include/asm/perf_event.h > @@ -23,6 +23,7 @@ > #define ARCH_PERFMON_EVENTSEL_USR (1ULL << 16) > #define ARCH_PERFMON_EVENTSEL_OS (1ULL << 17) > #define ARCH_PERFMON_EVENTSEL_EDGE (1ULL << 18) > +#define ARCH_PERFMON_EVENTSEL_PIN_CONTROL (1ULL << 19) > #define ARCH_PERFMON_EVENTSEL_INT (1ULL << 20) > #define ARCH_PERFMON_EVENTSEL_ANY (1ULL << 21) > #define ARCH_PERFMON_EVENTSEL_ENABLE (1ULL << 22) > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c > index 3e48c1d..6af9a54 100644 > --- a/arch/x86/kvm/pmu.c > +++ b/arch/x86/kvm/pmu.c > @@ -210,6 +210,9 @@ static void reprogram_gp_counter(struct kvm_pmc > *pmc, u64 eventsel) > unsigned config, type = PERF_TYPE_RAW; > u8 event_select, unit_mask; > > + if (eventsel & ARCH_PERFMON_EVENTSEL_PIN_CONTROL) > + printk_once("kvm pmu: pin control bit is ignored\n"); > + > pmc->eventsel = eventsel; > > stop_counter(pmc); Given that we know this now, will it makes sense to go with solution 1 in my cover letter for this patch - " 1. Mask the PC bit in the VPMU so as to not allow any writes to it from guests on any Intel machine. " But even then the question remains whether we should return an error when user tries to set this bit or should we just silently mask it the way KVM does? Mohit > > > -boris > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault 2017-04-24 15:44 ` Mohit Gambhir 2017-04-24 16:00 ` Boris Ostrovsky @ 2017-04-24 16:04 ` Jan Beulich 1 sibling, 0 replies; 7+ messages in thread From: Jan Beulich @ 2017-04-24 16:04 UTC (permalink / raw) To: Mohit Gambhir; +Cc: kevin.tian, boris.ostrovsky, jun.nakajima, xen-devel >>> On 24.04.17 at 17:44, <mohit.gambhir@oracle.com> wrote: > On 04/21/2017 03:14 AM, Jan Beulich wrote: >>>>> On 20.04.17 at 19:49,<mohit.gambhir@oracle.com> wrote: >>> This patch changes wrmsrl() calls to write to MSR_P6_EVTSEL register in the >>> VPMU to wrmsr_safe(). There are known (and possibly some unknown) cases where >>> setting certain bits in MSR_P6_EVTSEL reg. can cause a General Protection >>> fault on some machines. Unless we catch this fault when it happens, it will >>> result in a hypervisor crash. >>> >>> For instance, setting Pin Control (PC) bit (19) in MSR_P6_EVNTSEL results >>> in a General Protection Fault on Broadwell machines and thus causes the >>> hypervisor to crash. >> I can't seem to find indication of this in the SDM. Is this removal of a >> valid bit from an architectural MSR documented anywhere? > It see it as an observed hardware bug so I don't expect it to be found > in the SDM. I reached out to > the folks at Intel (CC) but they are unaware of it. Ideally it should > work the way it says in the SDM > for all events and on all machines but it doesn't - not for the all the > events and certainly not on all > machines. Please have the description make clear this is an (assumed) erratum, until Intel either clarifies or adds an entry to the spec update for the affected model(s). >> Further a more general question: Why do you do this only for >> MSR_P6_EVNTSEL*, but not consistently for all MSRs which are >> being written with (partially) guest controlled values? I'd namely >> expect all wrmsrl() to be switched over, unless it is clear that >> we're dealing with an erratum here and we therefore can >> assume that normal guarantees for architectural MSRs apply to >> everything else. > I think it is an erratum for one specific MSR which is MSR_P6_EVTSEL >> And finally, in the description you talk about forwarding the >> fault - I can see this being the case for core2_vpmu_do_wrmsr(), >> but how does this match up with __core2_vpmu_load() and its >> callers? Namely you may report an error there after already >> having updated some MSRs. Leaving the overall set of MSRs in >> an inconsistent state is not very likely to be a good idea. > Yes, that's something I did not think about and needs to be fixed. There > are two issues here - > > 1. What should we set MSR values in case of an error? We can not revert > them because if it is part of a > context switch then we do not want to leak the values from the previous > guest to the new guest. > So one possible solution is to wipe all PMU MSRs (set them to 0) before > returning the error. > > 2. __core2_vpmu_load is called in two scenarios - One from a hyper call > XENPMU_FLUSH from do xenpmu_op() > and the other during a context switch from vpmu_switch_to(). The > hypercall seems to be handling the error > correctly but vpmu_switch_to() ignores the error. So to propagate the > error up to the guest kernel maybe we should > inject the fault into the guest using > hvm_inject_hw_exception(TRAP_gp_fault, error)/ > pv_inject_hw_exception(TRAP_gp_fault, error)? You can't arbitrarily inject an exception out of the context switch code. I'm not sure you can sensibly continue the guest in this case, unless - as you say - there is a way to place the PMU into a consistent state (and you provide _some_kind of indication to the guest about the vPMU state no longer matching its expectations). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-24 18:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-20 17:49 [PATCH] Fix hypervisor crash when writing to VPMU MSR Mohit Gambhir 2017-04-20 17:49 ` [PATCH] x86/vpmu_intel: Fix hypervisor crash by catching wrmsr fault Mohit Gambhir 2017-04-21 7:14 ` Jan Beulich 2017-04-24 15:44 ` Mohit Gambhir 2017-04-24 16:00 ` Boris Ostrovsky 2017-04-24 18:49 ` Mohit Gambhir 2017-04-24 16:04 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.