[PATCH v1] restore the fake x2apic value for cpuid

[PATCH v1] restore the fake x2apic value for cpuid

[Xin Li]

From: Talons Lee <xin.li@citrix.com>

Commit e657fcc clears cpu x2apic capability bit instead of using fake
cpuid value. However, with cpuid x2apic bit on, xen_read_msr_safe() will
not clear the EXTD bit, which leads to uncessary msr write trying to
disable x2apic in __x2apic_disable(). So restore the fake x2apic value.

Signed-off-by: Talons Lee <xin.li@citrix.com>

---
CC: Igor Druzhinin <igor.druzhinin@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Juergen Gross <jgross@suse.com>

---
 arch/x86/xen/enlighten_pv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 4b20082..156c408 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -165,12 +165,17 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
 {
 	unsigned maskebx = ~0;
+	unsigned maskecx = ~0;
 
 	/*
 	 * Mask out inconvenient features, to try and disable as many
 	 * unsupported kernel subsystems as possible.
 	 */
 	switch (*ax) {
+	case 1:
+		maskecx &= ~(1 << (X86_FEATURE_X2APIC % 32));
+		break;
+
 	case CPUID_MWAIT_LEAF:
 		/* Synthesize the values.. */
 		*ax = 0;
@@ -193,6 +198,7 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		: "0" (*ax), "2" (*cx));
 
 	*bx &= maskebx;
+	*cx &= maskecx;
 }
 STACK_FRAME_NON_STANDARD(xen_cpuid); /* XEN_EMULATE_PREFIX */
 
-- 
1.8.3.1


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

Re: [PATCH v1] restore the fake x2apic value for cpuid

[Juergen Gross]

On 22/11/2018 08:18, Xin Li wrote:
> From: Talons Lee <xin.li@citrix.com>
> 
> Commit e657fcc clears cpu x2apic capability bit instead of using fake
> cpuid value. However, with cpuid x2apic bit on, xen_read_msr_safe() will
> not clear the EXTD bit, which leads to uncessary msr write trying to
> disable x2apic in __x2apic_disable(). So restore the fake x2apic value.
> 
> Signed-off-by: Talons Lee <xin.li@citrix.com>

Wouldn't it be easier to use just rdmsr_safe() in __x2apic_disable()
instead?


Juergen

> 
> ---
> CC: Igor Druzhinin <igor.druzhinin@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> ---
>  arch/x86/xen/enlighten_pv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index 4b20082..156c408 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -165,12 +165,17 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>  		      unsigned int *cx, unsigned int *dx)
>  {
>  	unsigned maskebx = ~0;
> +	unsigned maskecx = ~0;
>  
>  	/*
>  	 * Mask out inconvenient features, to try and disable as many
>  	 * unsupported kernel subsystems as possible.
>  	 */
>  	switch (*ax) {
> +	case 1:
> +		maskecx &= ~(1 << (X86_FEATURE_X2APIC % 32));
> +		break;
> +
>  	case CPUID_MWAIT_LEAF:
>  		/* Synthesize the values.. */
>  		*ax = 0;
> @@ -193,6 +198,7 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>  		: "0" (*ax), "2" (*cx));
>  
>  	*bx &= maskebx;
> +	*cx &= maskecx;
>  }
>  STACK_FRAME_NON_STANDARD(xen_cpuid); /* XEN_EMULATE_PREFIX */
>  
> 


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

Re: [PATCH v1] restore the fake x2apic value for cpuid

[Juergen Gross]

On 03/12/2018 08:00, Juergen Gross wrote:
> On 22/11/2018 08:18, Xin Li wrote:
>> From: Talons Lee <xin.li@citrix.com>
>>
>> Commit e657fcc clears cpu x2apic capability bit instead of using fake
>> cpuid value. However, with cpuid x2apic bit on, xen_read_msr_safe() will
>> not clear the EXTD bit, which leads to uncessary msr write trying to
>> disable x2apic in __x2apic_disable(). So restore the fake x2apic value.
>>
>> Signed-off-by: Talons Lee <xin.li@citrix.com>
> 
> Wouldn't it be easier to use just rdmsr_safe() in __x2apic_disable()
> instead?

Sorry, just seeing it now: using apic_is_x2apic_enabled() might be even
better. Same in __x2apic_enable().


Juergen

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

Re: [PATCH v1] restore the fake x2apic value for cpuid

[Xin Li (Talons)]

Hi Juergen,
    thanks for your comments.

   Are you suggesting to call apic_is_x2apic_enabled() in __x2apic_disable()?
   but I think that's exact what changed due to the fake xen_cpuid value.
   Doing so will probably still see the EXTD bit on, and call wrmsrl to disable x2apic.
   
log for linux4.4:
check_x2apic()
[ 0.000000] xen_read_msr_safe native read: fee00d00
[ 0.000000] clear X2APIC_ENABLE
[ 0.000000] xen_read_msr_safe cpuid: fee00900
[ 0.000000] x2apic_enabled cpu_has_x2apic: 0 apic_is_x2apic_enabled: 0

log for linux4.19:
[ 0.000817] xen_read_msr_safe native read: fee00d00
[ 0.000819] xen_read_msr_safe cpuid: *fee00d00 * //X2APIC_ENABLE not cleared
[ 0.000820] x2apic_enabled X86_FEATURE_X2APIC: 0 apic_is_x2apic_enabled: 1

________________________________________
From: Juergen Gross <jgross@suse.com>
Sent: Monday, December 3, 2018 3:03 PM
To: Xin Li; xen-devel@lists.xen.org
Cc: Xin Li (Talons); Sergey Dyasli; Andrew Cooper; Igor Druzhinin
Subject: Re: [Xen-devel] [PATCH v1] restore the fake x2apic value for cpuid

On 03/12/2018 08:00, Juergen Gross wrote:
> On 22/11/2018 08:18, Xin Li wrote:
>> From: Talons Lee <xin.li@citrix.com>
>>
>> Commit e657fcc clears cpu x2apic capability bit instead of using fake
>> cpuid value. However, with cpuid x2apic bit on, xen_read_msr_safe() will
>> not clear the EXTD bit, which leads to uncessary msr write trying to
>> disable x2apic in __x2apic_disable(). So restore the fake x2apic value.
>>
>> Signed-off-by: Talons Lee <xin.li@citrix.com>
>
> Wouldn't it be easier to use just rdmsr_safe() in __x2apic_disable()
> instead?

Sorry, just seeing it now: using apic_is_x2apic_enabled() might be even
better. Same in __x2apic_enable().


Juergen

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

Re: [PATCH v1] restore the fake x2apic value for cpuid

[Juergen Gross]

On 03/12/2018 12:15, Xin Li (Talons) wrote:
> Hi Juergen,
>     thanks for your comments.
> 
>    Are you suggesting to call apic_is_x2apic_enabled() in __x2apic_disable()?
>    but I think that's exact what changed due to the fake xen_cpuid value.
>    Doing so will probably still see the EXTD bit on, and call wrmsrl to disable x2apic.
>    
> log for linux4.4:
> check_x2apic()
> [ 0.000000] xen_read_msr_safe native read: fee00d00
> [ 0.000000] clear X2APIC_ENABLE
> [ 0.000000] xen_read_msr_safe cpuid: fee00900
> [ 0.000000] x2apic_enabled cpu_has_x2apic: 0 apic_is_x2apic_enabled: 0
> 
> log for linux4.19:
> [ 0.000817] xen_read_msr_safe native read: fee00d00
> [ 0.000819] xen_read_msr_safe cpuid: *fee00d00 * //X2APIC_ENABLE not cleared
> [ 0.000820] x2apic_enabled X86_FEATURE_X2APIC: 0 apic_is_x2apic_enabled: 1

Oh, you are right, sorry.

I overlooked rdmsr() is already using xen_read_msr_safe().

OTOH looking into xen_read_msr_safe() I can't see why the X2APIC_ENABLE
bit should be cleared depending on the cpuid value returned. We always
do setup_clear_cpu_cap(X86_FEATURE_X2APIC) for PV domains, so there is
no reason to check the cpuid return data for deciding whether to clear
X2APIC_ENABLE or not. I think we can do that always.

So why don't we remove the #ifdef CONFIG_X86_X2APIC block from
xen_read_msr_safe() ?


Juergen

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