| Message ID | 577F9CB2.5050406@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 08.07.16 at 14:29, <david.vrabel@citrix.com> wrote: > On 08/07/16 13:15, Jan Beulich wrote: >> As of Xen 4.7 PV CPUID doesn't expose either of CPUID[1].ECX[7] and >> CPUID[0x80000007].EDX[7] anymore, causing the driver to fail to load on >> both Intel and AMD systems. Doing any kind of hardware capability >> checks in the driver as a prerequisite was wrong anyway: With the >> hypervisor being in charge, all such checking should be done by it. If >> ACPI data gets uploaded despite some missing capability, the hypervisor >> is free to ignore part or all of that data. >> >> Ditch the entire check_prereq() function, and do the only valid check >> (xen_initial_domain()) in the caller in its place. > > Thanks, but I'm not sure this is sufficient. I think the generic ACPI > code needs to know the full capabilities in order to generate the > correct tables, or you won't get (for example) turbo mode working. > > We had to fake the EST feature back in. > > --- a/arch/x86/xen/enlighten.c > +++ b/arch/x86/xen/enlighten.c > @@ -448,7 +448,8 @@ static void __init xen_init_cpuid_mask(void) > if ((cx & xsave_mask) != xsave_mask) > cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE */ > if (xen_check_mwait()) > - cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32)); > + cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32) > + | 1 << (X86_FEATURE_EST % 32)); > } > > static void xen_set_debugreg(int reg, unsigned long val) Hmm, interesting. I admit I only tested on an AMD system, so I can't exclude the above is necessary. Otoh going over generic ACPI code the only use of X86_FEATURE_EST controls the logging of a message. Plus there's a use in arch_acpi_set_pdc_bits() - perhaps that's the one you mean? There's certainly no use of X86_FEATURE_HW_PSTATE anywhere in relevant code, so the AMD side would appear to be fine (which matches my testing). So I think the patch is fine as is (also avoiding cross component adjustments), and the part you suggest may then better be a separate patch? Jan
On 08/07/16 13:53, Jan Beulich wrote: >>>> On 08.07.16 at 14:29, <david.vrabel@citrix.com> wrote: >> On 08/07/16 13:15, Jan Beulich wrote: >>> As of Xen 4.7 PV CPUID doesn't expose either of CPUID[1].ECX[7] and >>> CPUID[0x80000007].EDX[7] anymore, causing the driver to fail to load on >>> both Intel and AMD systems. Doing any kind of hardware capability >>> checks in the driver as a prerequisite was wrong anyway: With the >>> hypervisor being in charge, all such checking should be done by it. If >>> ACPI data gets uploaded despite some missing capability, the hypervisor >>> is free to ignore part or all of that data. >>> >>> Ditch the entire check_prereq() function, and do the only valid check >>> (xen_initial_domain()) in the caller in its place. >> >> Thanks, but I'm not sure this is sufficient. I think the generic ACPI >> code needs to know the full capabilities in order to generate the >> correct tables, or you won't get (for example) turbo mode working. >> >> We had to fake the EST feature back in. >> >> --- a/arch/x86/xen/enlighten.c >> +++ b/arch/x86/xen/enlighten.c >> @@ -448,7 +448,8 @@ static void __init xen_init_cpuid_mask(void) >> if ((cx & xsave_mask) != xsave_mask) >> cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE */ >> if (xen_check_mwait()) >> - cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32)); >> + cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32) >> + | 1 << (X86_FEATURE_EST % 32)); >> } >> >> static void xen_set_debugreg(int reg, unsigned long val) > > Hmm, interesting. I admit I only tested on an AMD system, so I > can't exclude the above is necessary. Otoh going over generic > ACPI code the only use of X86_FEATURE_EST controls the > logging of a message. Plus there's a use in > arch_acpi_set_pdc_bits() - perhaps that's the one you mean? > > There's certainly no use of X86_FEATURE_HW_PSTATE anywhere > in relevant code, so the AMD side would appear to be fine (which > matches my testing). So I think the patch is fine as is (also avoiding > cross component adjustments), and the part you suggest may then > better be a separate patch? It's also possible that I'm misremembering why we went with the above hack. I've applied your patch to for-linus-3.7b, thanks. David
--- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -448,7 +448,8 @@ static void __init xen_init_cpuid_mask(void) if ((cx & xsave_mask) != xsave_mask) cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE */ if (xen_check_mwait()) - cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32)); + cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32) + | 1 << (X86_FEATURE_EST % 32)); } static void xen_set_debugreg(int reg, unsigned long val)