xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7
@ 2016-07-08 12:15 Jan Beulich
  0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2016-07-08 12:15 UTC (permalink / raw)
  To: david.vrabel, boris.ostrovsky, Juergen Gross; +Cc: xen-devel, linux-kernel

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 drivers/xen/xen-acpi-processor.c |   35 +++--------------------------------
 1 file changed, 3 insertions(+), 32 deletions(-)

--- 4.7-rc6/drivers/xen/xen-acpi-processor.c
+++ 4.7-rc6-xen-acpi-processor-prereqs/drivers/xen/xen-acpi-processor.c
@@ -423,36 +423,7 @@ upload:
 
 	return 0;
 }
-static int __init check_prereq(void)
-{
-	struct cpuinfo_x86 *c = &cpu_data(0);
-
-	if (!xen_initial_domain())
-		return -ENODEV;
-
-	if (!acpi_gbl_FADT.smi_command)
-		return -ENODEV;
 
-	if (c->x86_vendor == X86_VENDOR_INTEL) {
-		if (!cpu_has(c, X86_FEATURE_EST))
-			return -ENODEV;
-
-		return 0;
-	}
-	if (c->x86_vendor == X86_VENDOR_AMD) {
-		/* Copied from powernow-k8.h, can't include ../cpufreq/powernow
-		 * as we get compile warnings for the static functions.
-		 */
-#define CPUID_FREQ_VOLT_CAPABILITIES    0x80000007
-#define USE_HW_PSTATE                   0x00000080
-		u32 eax, ebx, ecx, edx;
-		cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx);
-		if ((edx & USE_HW_PSTATE) != USE_HW_PSTATE)
-			return -ENODEV;
-		return 0;
-	}
-	return -ENODEV;
-}
 /* acpi_perf_data is a pointer to percpu data. */
 static struct acpi_processor_performance __percpu *acpi_perf_data;
 
@@ -509,10 +480,10 @@ struct notifier_block xen_acpi_processor
 static int __init xen_acpi_processor_init(void)
 {
 	unsigned int i;
-	int rc = check_prereq();
+	int rc;
 
-	if (rc)
-		return rc;
+	if (!xen_initial_domain())
+		return -ENODEV;
 
 	nr_acpi_bits = get_max_acpi_id() + 1;
 	acpi_ids_done = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned long), GFP_KERNEL);




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7
       [not found]   ` <577FBE6702000078000FCA81@prv-mh.provo.novell.com>
@ 2016-07-08 13:53     ` David Vrabel
  0 siblings, 0 replies; 4+ messages in thread
From: David Vrabel @ 2016-07-08 13:53 UTC (permalink / raw)
  To: Jan Beulich, David Vrabel, boris.ostrovsky, Juergen Gross
  Cc: xen-devel, linux-kernel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7
       [not found] ` <577F9CB2.5050406@citrix.com>
@ 2016-07-08 12:53   ` Jan Beulich
       [not found]   ` <577FBE6702000078000FCA81@prv-mh.provo.novell.com>
  1 sibling, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2016-07-08 12:53 UTC (permalink / raw)
  To: David Vrabel, boris.ostrovsky, Juergen Gross; +Cc: xen-devel, linux-kernel

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7
       [not found] <577FB56B02000078000FC9DB@prv-mh.provo.novell.com>
@ 2016-07-08 12:29 ` David Vrabel
       [not found] ` <577F9CB2.5050406@citrix.com>
  1 sibling, 0 replies; 4+ messages in thread
From: David Vrabel @ 2016-07-08 12:29 UTC (permalink / raw)
  To: Jan Beulich, boris.ostrovsky, Juergen Gross; +Cc: xen-devel, linux-kernel

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)

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-07-08 13:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-08 12:15 [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7 Jan Beulich
     [not found] <577FB56B02000078000FC9DB@prv-mh.provo.novell.com>
2016-07-08 12:29 ` David Vrabel
     [not found] ` <577F9CB2.5050406@citrix.com>
2016-07-08 12:53   ` Jan Beulich
     [not found]   ` <577FBE6702000078000FCA81@prv-mh.provo.novell.com>
2016-07-08 13:53     ` David Vrabel

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).