[PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Jan Beulich]

xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
For this to work when NX is not available, x86_configure_nx() needs to
be called first.

Reported-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1273,16 +1273,16 @@ asmlinkage __visible void __init xen_sta
 	/* Get mfn list */
 	xen_build_dynamic_phys_to_machine();
 
+	/* Work out if we support NX */
+	get_cpu_cap(&boot_cpu_data);
+	x86_configure_nx();
+
 	/*
 	 * Set up kernel GDT and segment registers, mainly so that
 	 * -fstack-protector code can be executed.
 	 */
 	xen_setup_gdt(0);
 
-	/* Work out if we support NX */
-	get_cpu_cap(&boot_cpu_data);
-	x86_configure_nx();
-
 	/* Determine virtual and physical address sizes */
 	get_cpu_address_sizes(&boot_cpu_data);
 

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 367 bytes --]

On 20.05.21 13:42, Jan Beulich wrote:
> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
> For this to work when NX is not available, x86_configure_nx() needs to
> be called first.
> 
> Reported-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Jan Beulich]

On 20.05.2021 13:57, Juergen Gross wrote:
> On 20.05.21 13:42, Jan Beulich wrote:
>> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
>> For this to work when NX is not available, x86_configure_nx() needs to
>> be called first.
>>
>> Reported-by: Olaf Hering <olaf@aepfle.de>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks. I guess I forgot

Cc: stable@vger.kernel.org

If you agree, can you please add this before pushing to Linus?

Jan

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 630 bytes --]

On 20.05.21 14:08, Jan Beulich wrote:
> On 20.05.2021 13:57, Juergen Gross wrote:
>> On 20.05.21 13:42, Jan Beulich wrote:
>>> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
>>> For this to work when NX is not available, x86_configure_nx() needs to
>>> be called first.
>>>
>>> Reported-by: Olaf Hering <olaf@aepfle.de>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Thanks. I guess I forgot
> 
> Cc: stable@vger.kernel.org
> 
> If you agree, can you please add this before pushing to Linus?

Yes, will do that.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1030 bytes --]

On 20.05.21 14:08, Jan Beulich wrote:
> On 20.05.2021 13:57, Juergen Gross wrote:
>> On 20.05.21 13:42, Jan Beulich wrote:
>>> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
>>> For this to work when NX is not available, x86_configure_nx() needs to
>>> be called first.
>>>
>>> Reported-by: Olaf Hering <olaf@aepfle.de>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Thanks. I guess I forgot
> 
> Cc: stable@vger.kernel.org
> 
> If you agree, can you please add this before pushing to Linus?

Uh, just had a look why x86_configure_nx() was called after
xen_setup_gdt().

Upstream your patch will be fine, but before kernel 5.9 it will
break running as 32-bit PV guest (see commit 36104cb9012a82e7).

So I will take your patch as is, but for kernels 5.8 and older I
recommend a different approach by directly setting the NX
capability after checking the cpuid bit instead of letting that
do get_cpu_cap().


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Jan Beulich]

On 21.05.2021 09:18, Juergen Gross wrote:
> On 20.05.21 14:08, Jan Beulich wrote:
>> On 20.05.2021 13:57, Juergen Gross wrote:
>>> On 20.05.21 13:42, Jan Beulich wrote:
>>>> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
>>>> For this to work when NX is not available, x86_configure_nx() needs to
>>>> be called first.
>>>>
>>>> Reported-by: Olaf Hering <olaf@aepfle.de>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>
>> Thanks. I guess I forgot
>>
>> Cc: stable@vger.kernel.org
>>
>> If you agree, can you please add this before pushing to Linus?
> 
> Uh, just had a look why x86_configure_nx() was called after
> xen_setup_gdt().
> 
> Upstream your patch will be fine, but before kernel 5.9 it will
> break running as 32-bit PV guest (see commit 36104cb9012a82e7).

Oh, indeed. That commit then actually introduced the issue here,
and hence a Fixes: tag may be warranted.

> So I will take your patch as is, but for kernels 5.8 and older I
> recommend a different approach by directly setting the NX
> capability after checking the cpuid bit instead of letting that
> do get_cpu_cap().

Right - perhaps the only halfway viable option.

64-bit kernels predating 4f277295e54c may then also need that one,
but perhaps all stable ones already have it because it was tagged
for stable.

Jan

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1172 bytes --]

On 21.05.21 09:26, Jan Beulich wrote:
> On 21.05.2021 09:18, Juergen Gross wrote:
>> On 20.05.21 14:08, Jan Beulich wrote:
>>> On 20.05.2021 13:57, Juergen Gross wrote:
>>>> On 20.05.21 13:42, Jan Beulich wrote:
>>>>> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
>>>>> For this to work when NX is not available, x86_configure_nx() needs 
to
>>>>> be called first.
>>>>>
>>>>> Reported-by: Olaf Hering <olaf@aepfle.de>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>>
>>> Thanks. I guess I forgot
>>>
>>> Cc: stable@vger.kernel.org
>>>
>>> If you agree, can you please add this before pushing to Linus?
>>
>> Uh, just had a look why x86_configure_nx() was called after
>> xen_setup_gdt().
>>
>> Upstream your patch will be fine, but before kernel 5.9 it will
>> break running as 32-bit PV guest (see commit 36104cb9012a82e7).
> 
> Oh, indeed. That commit then actually introduced the issue here,
> and hence a Fixes: tag may be warranted.

Added it already. :-)

And I've limited the backport to happen not for 5.8 and older, of
course.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Boris Ostrovsky]

On 5/21/21 3:45 AM, Juergen Gross wrote:
> On 21.05.21 09:26, Jan Beulich wrote:
>> On 21.05.2021 09:18, Juergen Gross wrote:
>>> On 20.05.21 14:08, Jan Beulich wrote:
>>>> On 20.05.2021 13:57, Juergen Gross wrote:
>>>>> On 20.05.21 13:42, Jan Beulich wrote:
>>>>>> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
>>>>>> For this to work when NX is not available, x86_configure_nx() needs 
> to
>>>>>> be called first.
>>>>>>
>>>>>> Reported-by: Olaf Hering <olaf@aepfle.de>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>>>
>>>> Thanks. I guess I forgot
>>>>
>>>> Cc: stable@vger.kernel.org
>>>>
>>>> If you agree, can you please add this before pushing to Linus?
>>>
>>> Uh, just had a look why x86_configure_nx() was called after
>>> xen_setup_gdt().
>>>
>>> Upstream your patch will be fine, but before kernel 5.9 it will
>>> break running as 32-bit PV guest (see commit 36104cb9012a82e7).
>>
>> Oh, indeed. That commit then actually introduced the issue here,
>> and hence a Fixes: tag may be warranted.
>
> Added it already. :-)
>
> And I've limited the backport to happen not for 5.8 and older, of
> course.


Did something changed recently that this became a problem? That commit has been there for 3 years.


Didn't Olaf report this to be a problem only on SLES kernels?



-boris

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Jan Beulich]

On 21.05.2021 15:12, Boris Ostrovsky wrote:
> 
> On 5/21/21 3:45 AM, Juergen Gross wrote:
>> On 21.05.21 09:26, Jan Beulich wrote:
>>> On 21.05.2021 09:18, Juergen Gross wrote:
>>>> On 20.05.21 14:08, Jan Beulich wrote:
>>>>> On 20.05.2021 13:57, Juergen Gross wrote:
>>>>>> On 20.05.21 13:42, Jan Beulich wrote:
>>>>>>> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
>>>>>>> For this to work when NX is not available, x86_configure_nx() needs 
>> to
>>>>>>> be called first.
>>>>>>>
>>>>>>> Reported-by: Olaf Hering <olaf@aepfle.de>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>
>>>>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> Thanks. I guess I forgot
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>>
>>>>> If you agree, can you please add this before pushing to Linus?
>>>>
>>>> Uh, just had a look why x86_configure_nx() was called after
>>>> xen_setup_gdt().
>>>>
>>>> Upstream your patch will be fine, but before kernel 5.9 it will
>>>> break running as 32-bit PV guest (see commit 36104cb9012a82e7).
>>>
>>> Oh, indeed. That commit then actually introduced the issue here,
>>> and hence a Fixes: tag may be warranted.
>>
>> Added it already. :-)
>>
>> And I've limited the backport to happen not for 5.8 and older, of
>> course.
> 
> 
> Did something changed recently that this became a problem? That commit has been there for 3 years.

He happened to try on a system where NX was turned off in the BIOS. That's
not a setting you would usually find in use.


> Didn't Olaf report this to be a problem only on SLES kernels?

Which I assume have backports of the problematic change.

Jan

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Boris Ostrovsky]

On 5/21/21 9:15 AM, Jan Beulich wrote:
> On 21.05.2021 15:12, Boris Ostrovsky wrote:
>>
>> Did something changed recently that this became a problem? That commit has been there for 3 years.
> He happened to try on a system where NX was turned off in the BIOS.


Yes, I missed that part.


-boris

Re: [PATCH] x86/Xen: swap NX determination and GDT setup on BSP

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 362 bytes --]

On 20.05.21 13:42, Jan Beulich wrote:
> xen_setup_gdt(), via xen_load_gdt_boot(), wants to adjust page tables.
> For this to work when NX is not available, x86_configure_nx() needs to
> be called first.
> 
> Reported-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Pushed to xen/tip.git for-linus-5.13b


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]