| Message ID | 1481253472-9209-1-git-send-email-luwei.kang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 09.12.16 at 04:17, <luwei.kang@intel.com> wrote: > IA32_PERF_GLOBAL_STATUS.OvfUncore (MSR 38EH, bit[61]) is always 0 and > writing 1 to IA32_PERF_GLOBAL_OVF_CTRL.ClrOvfUncore (MSR 390H, bit[61]) > signals #GP. > Reference "Intel Xeon Phi Procssor x200 Product Family", document > number 334646-008. I can see this being a necessary workaround, but going over the other errata there are some of quite a bit higher importance to work around (KNL13) or at least check whether we're affected (KNL4, KNL8), if we really care about supporting Xen on this processor model. I'd appreciate other opinions namely from the people on Cc. Jan
On 12/08/2016 10:17 PM, Luwei Kang wrote: > IA32_PERF_GLOBAL_STATUS.OvfUncore (MSR 38EH, bit[61]) is always 0 and > writing 1 to IA32_PERF_GLOBAL_OVF_CTRL.ClrOvfUncore (MSR 390H, bit[61]) > signals #GP. > Reference "Intel Xeon Phi Procssor x200 Product Family", document > number 334646-008. > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > --- > xen/arch/x86/cpu/vpmu_intel.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c > index e8049ed..0be78ff 100644 > --- a/xen/arch/x86/cpu/vpmu_intel.c > +++ b/xen/arch/x86/cpu/vpmu_intel.c > @@ -1058,11 +1058,17 @@ int __init core2_vpmu_init(void) > (((1ULL << fixed_pmc_cnt) - 1) << 32) | > ((1ULL << arch_pmc_cnt) - 1)); > if ( version > 2 ) > + { > /* > * Even though we don't support Uncore counters guests should be > * able to clear all available overflows. > */ > global_ovf_ctrl_mask &= ~(1ULL << 61); > + /* Knight Landing doesn't support overflow bit on uncore counters */ > + if ( current_cpu_data.x86_model == 0x57 ) > + global_ovf_ctrl_mask |= (1ULL << 61); > + > + } > I think these types of model-specific changes (or errata) should be done in check_pmc_quirk(). And is_pmc_quirk (along with handle_pmc_quirk()) should be renamed to something more specific to that particular problem. Maybe pmc_underflow_quirk? -boris
> On 12/08/2016 10:17 PM, Luwei Kang wrote: > > IA32_PERF_GLOBAL_STATUS.OvfUncore (MSR 38EH, bit[61]) is always 0 and > > writing 1 to IA32_PERF_GLOBAL_OVF_CTRL.ClrOvfUncore (MSR 390H, > > bit[61]) signals #GP. > > Reference "Intel Xeon Phi Procssor x200 Product Family", document > > number 334646-008. > > > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > > --- > > xen/arch/x86/cpu/vpmu_intel.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/xen/arch/x86/cpu/vpmu_intel.c > > b/xen/arch/x86/cpu/vpmu_intel.c index e8049ed..0be78ff 100644 > > --- a/xen/arch/x86/cpu/vpmu_intel.c > > +++ b/xen/arch/x86/cpu/vpmu_intel.c > > @@ -1058,11 +1058,17 @@ int __init core2_vpmu_init(void) > > (((1ULL << fixed_pmc_cnt) - 1) << 32) | > > ((1ULL << arch_pmc_cnt) - 1)); > > if ( version > 2 ) > > + { > > /* > > * Even though we don't support Uncore counters guests should be > > * able to clear all available overflows. > > */ > > global_ovf_ctrl_mask &= ~(1ULL << 61); > > + /* Knight Landing doesn't support overflow bit on uncore counters */ > > + if ( current_cpu_data.x86_model == 0x57 ) > > + global_ovf_ctrl_mask |= (1ULL << 61); > > + > > + } > > > > I think these types of model-specific changes (or errata) should be done in check_pmc_quirk(). And is_pmc_quirk (along with > handle_pmc_quirk()) should be renamed to something more specific to that particular problem. > Maybe pmc_underflow_quirk? > By the way, I think another place may need to do some modify as well. @@ -868,7 +868,7 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs) if ( is_pmc_quirk ) handle_pmc_quirk(msr_content); core2_vpmu_cxt->global_status |= msr_content; - msr_content = ~global_ovf_ctrl_mask; + msr_content &= ~global_ovf_ctrl_mask; wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content); } If one counter have overflow all the bit will be clean. I think it need add & with current status. Hi jan and Andrew, What is your opinion? Thanks, Luwei Kang
>>> On 12.12.16 at 07:51, <luwei.kang@intel.com> wrote: > By the way, I think another place may need to do some modify as well. > > @@ -868,7 +868,7 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs) > if ( is_pmc_quirk ) > handle_pmc_quirk(msr_content); > core2_vpmu_cxt->global_status |= msr_content; > - msr_content = ~global_ovf_ctrl_mask; > + msr_content &= ~global_ovf_ctrl_mask; > wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content); > } > > If one counter have overflow all the bit will be clean. I think it need add & > with current status. > > Hi jan and Andrew, > What is your opinion? I agree, but it looks to be an independent change. Jan
> >>> On 12.12.16 at 07:51, <luwei.kang@intel.com> wrote: > > By the way, I think another place may need to do some modify as well. > > > > @@ -868,7 +868,7 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs) > > if ( is_pmc_quirk ) > > handle_pmc_quirk(msr_content); > > core2_vpmu_cxt->global_status |= msr_content; > > - msr_content = ~global_ovf_ctrl_mask; > > + msr_content &= ~global_ovf_ctrl_mask; > > wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content); > > } > > > > If one counter have overflow all the bit will be clean. I think it > > need add & with current status. > > > > Hi jan and Andrew, > > What is your opinion? > > I agree, but it looks to be an independent change. > Hi Jan, Thanks for your reply. I will submit another patch regarding " msr_content &= ~global_ovf_ctrl_mask;" soon. What is your opinion about this patch? Need any modify? Thanks, Luwei Kang
>>> On 12.12.16 at 09:06, <luwei.kang@intel.com> wrote: >> >>> On 12.12.16 at 07:51, <luwei.kang@intel.com> wrote: >> > By the way, I think another place may need to do some modify as well. >> > >> > @@ -868,7 +868,7 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs) >> > if ( is_pmc_quirk ) >> > handle_pmc_quirk(msr_content); >> > core2_vpmu_cxt->global_status |= msr_content; >> > - msr_content = ~global_ovf_ctrl_mask; >> > + msr_content &= ~global_ovf_ctrl_mask; >> > wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content); >> > } >> > >> > If one counter have overflow all the bit will be clean. I think it >> > need add & with current status. >> > >> > Hi jan and Andrew, >> > What is your opinion? >> >> I agree, but it looks to be an independent change. >> > Thanks for your reply. I will submit another patch regarding " msr_content &= ~global_ovf_ctrl_mask;" soon. > What is your opinion about this patch? Need any modify? "This" being which one? The one visible above, or the one at the root of this thread? For the one above I've already said "I agree", so I'm not sure what else you ask for. For the one at the root of this thread, I think Boris has pointed out what changes would be wanted. Jan
> From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Friday, December 09, 2016 5:43 PM ing > > >>> On 09.12.16 at 04:17, <luwei.kang@intel.com> wrote: > > IA32_PERF_GLOBAL_STATUS.OvfUncore (MSR 38EH, bit[61]) is always 0 and > > writing 1 to IA32_PERF_GLOBAL_OVF_CTRL.ClrOvfUncore (MSR 390H, bit[61]) > > signals #GP. > > Reference "Intel Xeon Phi Procssor x200 Product Family", document > > number 334646-008. > > I can see this being a necessary workaround, but going over the > other errata there are some of quite a bit higher importance to > work around (KNL13) or at least check whether we're affected > (KNL4, KNL8), if we really care about supporting Xen on this > processor model. I'd appreciate other opinions namely from the > people on Cc. > I haven't looked at above errata in detail. Luwei, can you plan them accordingly to make sure we have a good support here? If there's any technical issue on workaround, please just raise it up. :-) Thanks Kevin
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c index e8049ed..0be78ff 100644 --- a/xen/arch/x86/cpu/vpmu_intel.c +++ b/xen/arch/x86/cpu/vpmu_intel.c @@ -1058,11 +1058,17 @@ int __init core2_vpmu_init(void) (((1ULL << fixed_pmc_cnt) - 1) << 32) | ((1ULL << arch_pmc_cnt) - 1)); if ( version > 2 ) + { /* * Even though we don't support Uncore counters guests should be * able to clear all available overflows. */ global_ovf_ctrl_mask &= ~(1ULL << 61); + /* Knight Landing doesn't support overflow bit on uncore counters */ + if ( current_cpu_data.x86_model == 0x57 ) + global_ovf_ctrl_mask |= (1ULL << 61); + + } regs_sz = (sizeof(struct xen_pmu_intel_ctxt) - regs_off) + sizeof(uint64_t) * fixed_pmc_cnt +
IA32_PERF_GLOBAL_STATUS.OvfUncore (MSR 38EH, bit[61]) is always 0 and writing 1 to IA32_PERF_GLOBAL_OVF_CTRL.ClrOvfUncore (MSR 390H, bit[61]) signals #GP. Reference "Intel Xeon Phi Procssor x200 Product Family", document number 334646-008. Signed-off-by: Luwei Kang <luwei.kang@intel.com> --- xen/arch/x86/cpu/vpmu_intel.c | 6 ++++++ 1 file changed, 6 insertions(+)