diff mbox series

x86/boot: skip realmode init code when running as Xen PV guest

Message ID 20221121162433.28070-1-jgross@suse.com
State New
Headers
Series x86/boot: skip realmode init code when running as Xen PV guest |

Commit Message

Juergen Gross Nov. 21, 2022, 4:24 p.m. UTC 
  When running as a Xen PV guest there is no need for setting up the
realmode trampoline, as realmode isn't supported in this environment.

Trying to setup the trampoline has been proven to be problematic in
some cases, especially when trying to debug early boot problems with
Xen requiring to keep the EFI boot-services memory mapped (some
firmware variants seem to claim basically all memory below 1M for boot
services).

Skip the trampoline setup code for Xen PV guests.

Fixes: 084ee1c641a0 ("x86, realmode: Relocator for realmode code")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/realmode.h | 4 ++--
 arch/x86/realmode/init.c        | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Borislav Petkov Nov. 21, 2022, 7:08 p.m. UTC | #1 
On Mon, Nov 21, 2022 at 05:24:33PM +0100, Juergen Gross wrote:
> When running as a Xen PV guest there is no need for setting up the
> realmode trampoline, as realmode isn't supported in this environment.
> 
> Trying to setup the trampoline has been proven to be problematic in
> some cases, especially when trying to debug early boot problems with
> Xen requiring to keep the EFI boot-services memory mapped (some
> firmware variants seem to claim basically all memory below 1M for boot
> services).
> 
> Skip the trampoline setup code for Xen PV guests.
> 
> Fixes: 084ee1c641a0 ("x86, realmode: Relocator for realmode code")
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/include/asm/realmode.h | 4 ++--
>  arch/x86/realmode/init.c        | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
 
> diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
> index fd6f6e5b755a..5bfce58f1bab 100644
> --- a/arch/x86/include/asm/realmode.h
> +++ b/arch/x86/include/asm/realmode.h
> @@ -78,8 +78,8 @@ extern unsigned char secondary_startup_64_no_verify[];
>  
>  static inline size_t real_mode_size_needed(void)
>  {
> -	if (real_mode_header)
> -		return 0;	/* already allocated. */
> +	if (real_mode_header || cpu_feature_enabled(X86_FEATURE_XENPV))
> +		return 0;	/* already allocated or not needed. */
>  
>  	return ALIGN(real_mode_blob_end - real_mode_blob, PAGE_SIZE);
>  }
> diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
> index 41d7669a97ad..1826700b156e 100644
> --- a/arch/x86/realmode/init.c
> +++ b/arch/x86/realmode/init.c
> @@ -202,6 +202,9 @@ static void __init set_real_mode_permissions(void)
>  
>  static int __init init_real_mode(void)
>  {
> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))a

This reminds me of the notorious if (xen) sprinkling from years ago.
Please don't do that.
Juergen Gross Nov. 22, 2022, 6:28 a.m. UTC | #2 
On 21.11.22 20:08, Borislav Petkov wrote:
> On Mon, Nov 21, 2022 at 05:24:33PM +0100, Juergen Gross wrote:
>> When running as a Xen PV guest there is no need for setting up the
>> realmode trampoline, as realmode isn't supported in this environment.
>>
>> Trying to setup the trampoline has been proven to be problematic in
>> some cases, especially when trying to debug early boot problems with
>> Xen requiring to keep the EFI boot-services memory mapped (some
>> firmware variants seem to claim basically all memory below 1M for boot
>> services).
>>
>> Skip the trampoline setup code for Xen PV guests.
>>
>> Fixes: 084ee1c641a0 ("x86, realmode: Relocator for realmode code")
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   arch/x86/include/asm/realmode.h | 4 ++--
>>   arch/x86/realmode/init.c        | 3 +++
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>   
>> diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
>> index fd6f6e5b755a..5bfce58f1bab 100644
>> --- a/arch/x86/include/asm/realmode.h
>> +++ b/arch/x86/include/asm/realmode.h
>> @@ -78,8 +78,8 @@ extern unsigned char secondary_startup_64_no_verify[];
>>   
>>   static inline size_t real_mode_size_needed(void)
>>   {
>> -	if (real_mode_header)
>> -		return 0;	/* already allocated. */
>> +	if (real_mode_header || cpu_feature_enabled(X86_FEATURE_XENPV))
>> +		return 0;	/* already allocated or not needed. */
>>   
>>   	return ALIGN(real_mode_blob_end - real_mode_blob, PAGE_SIZE);
>>   }
>> diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
>> index 41d7669a97ad..1826700b156e 100644
>> --- a/arch/x86/realmode/init.c
>> +++ b/arch/x86/realmode/init.c
>> @@ -202,6 +202,9 @@ static void __init set_real_mode_permissions(void)
>>   
>>   static int __init init_real_mode(void)
>>   {
>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))a
> 
> This reminds me of the notorious if (xen) sprinkling from years ago.
> Please don't do that.
> 

Okay, what about plan B:

- rework realmode/rm to:
   + replace header.S with main.c making it possible to initialize
     struct real_mode_header using the struct definition
   + optional: merge stack.S into main.c
- include realmode/rm addresses needed outside of it in struct
   real_mode_header
- setup a dummy struct real_mode_header in Xen PV code removing the
   need to skip init_real_mode(), but making it basically a nop

Would you be fine with that?


Juergen
H. Peter Anvin Nov. 23, 2022, 12:07 a.m. UTC | #3 
On November 21, 2022 10:28:21 PM PST, Juergen Gross <jgross@suse.com> wrote:
>On 21.11.22 20:08, Borislav Petkov wrote:
>> On Mon, Nov 21, 2022 at 05:24:33PM +0100, Juergen Gross wrote:
>>> When running as a Xen PV guest there is no need for setting up the
>>> realmode trampoline, as realmode isn't supported in this environment.
>>> 
>>> Trying to setup the trampoline has been proven to be problematic in
>>> some cases, especially when trying to debug early boot problems with
>>> Xen requiring to keep the EFI boot-services memory mapped (some
>>> firmware variants seem to claim basically all memory below 1M for boot
>>> services).
>>> 
>>> Skip the trampoline setup code for Xen PV guests.
>>> 
>>> Fixes: 084ee1c641a0 ("x86, realmode: Relocator for realmode code")
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>   arch/x86/include/asm/realmode.h | 4 ++--
>>>   arch/x86/realmode/init.c        | 3 +++
>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>   
>>> diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
>>> index fd6f6e5b755a..5bfce58f1bab 100644
>>> --- a/arch/x86/include/asm/realmode.h
>>> +++ b/arch/x86/include/asm/realmode.h
>>> @@ -78,8 +78,8 @@ extern unsigned char secondary_startup_64_no_verify[];
>>>     static inline size_t real_mode_size_needed(void)
>>>   {
>>> -	if (real_mode_header)
>>> -		return 0;	/* already allocated. */
>>> +	if (real_mode_header || cpu_feature_enabled(X86_FEATURE_XENPV))
>>> +		return 0;	/* already allocated or not needed. */
>>>     	return ALIGN(real_mode_blob_end - real_mode_blob, PAGE_SIZE);
>>>   }
>>> diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
>>> index 41d7669a97ad..1826700b156e 100644
>>> --- a/arch/x86/realmode/init.c
>>> +++ b/arch/x86/realmode/init.c
>>> @@ -202,6 +202,9 @@ static void __init set_real_mode_permissions(void)
>>>     static int __init init_real_mode(void)
>>>   {
>>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))a
>> 
>> This reminds me of the notorious if (xen) sprinkling from years ago.
>> Please don't do that.
>> 
>
>Okay, what about plan B:
>
>- rework realmode/rm to:
>  + replace header.S with main.c making it possible to initialize
>    struct real_mode_header using the struct definition
>  + optional: merge stack.S into main.c
>- include realmode/rm addresses needed outside of it in struct
>  real_mode_header
>- setup a dummy struct real_mode_header in Xen PV code removing the
>  need to skip init_real_mode(), but making it basically a nop
>
>Would you be fine with that?
>
>
>Juergen

I'm wondering if init_real_mode should not simply be part of the platform ops. It's called exactly twice per boot, it is hard to be less performance critical than that.
Juergen Gross Nov. 23, 2022, 6:29 a.m. UTC | #4 
On 23.11.22 01:07, H. Peter Anvin wrote:
> On November 21, 2022 10:28:21 PM PST, Juergen Gross <jgross@suse.com> wrote:
>> On 21.11.22 20:08, Borislav Petkov wrote:
>>> On Mon, Nov 21, 2022 at 05:24:33PM +0100, Juergen Gross wrote:
>>>> When running as a Xen PV guest there is no need for setting up the
>>>> realmode trampoline, as realmode isn't supported in this environment.
>>>>
>>>> Trying to setup the trampoline has been proven to be problematic in
>>>> some cases, especially when trying to debug early boot problems with
>>>> Xen requiring to keep the EFI boot-services memory mapped (some
>>>> firmware variants seem to claim basically all memory below 1M for boot
>>>> services).
>>>>
>>>> Skip the trampoline setup code for Xen PV guests.
>>>>
>>>> Fixes: 084ee1c641a0 ("x86, realmode: Relocator for realmode code")
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>    arch/x86/include/asm/realmode.h | 4 ++--
>>>>    arch/x86/realmode/init.c        | 3 +++
>>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>    
>>>> diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
>>>> index fd6f6e5b755a..5bfce58f1bab 100644
>>>> --- a/arch/x86/include/asm/realmode.h
>>>> +++ b/arch/x86/include/asm/realmode.h
>>>> @@ -78,8 +78,8 @@ extern unsigned char secondary_startup_64_no_verify[];
>>>>      static inline size_t real_mode_size_needed(void)
>>>>    {
>>>> -	if (real_mode_header)
>>>> -		return 0;	/* already allocated. */
>>>> +	if (real_mode_header || cpu_feature_enabled(X86_FEATURE_XENPV))
>>>> +		return 0;	/* already allocated or not needed. */
>>>>      	return ALIGN(real_mode_blob_end - real_mode_blob, PAGE_SIZE);
>>>>    }
>>>> diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
>>>> index 41d7669a97ad..1826700b156e 100644
>>>> --- a/arch/x86/realmode/init.c
>>>> +++ b/arch/x86/realmode/init.c
>>>> @@ -202,6 +202,9 @@ static void __init set_real_mode_permissions(void)
>>>>      static int __init init_real_mode(void)
>>>>    {
>>>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))a
>>>
>>> This reminds me of the notorious if (xen) sprinkling from years ago.
>>> Please don't do that.
>>>
>>
>> Okay, what about plan B:
>>
>> - rework realmode/rm to:
>>   + replace header.S with main.c making it possible to initialize
>>     struct real_mode_header using the struct definition
>>   + optional: merge stack.S into main.c
>> - include realmode/rm addresses needed outside of it in struct
>>   real_mode_header
>> - setup a dummy struct real_mode_header in Xen PV code removing the
>>   need to skip init_real_mode(), but making it basically a nop
>>
>> Would you be fine with that?
>>
>>
>> Juergen
> 
> I'm wondering if init_real_mode should not simply be part of the platform ops. It's called exactly twice per boot, it is hard to be less performance critical than that.

Actually, it is called only once. :-)

Any preferences regarding the call hierarchy? I could either keep
the early_initcall() and call the platform_op from that initcall, or
I could introduce a small wrapper called by kernel_init_freeable(),
which would be empty on all but the x86 architecture.

Boris, if you agree, I can go that route.


Juergen
diff mbox series

Patch

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index fd6f6e5b755a..5bfce58f1bab 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -78,8 +78,8 @@  extern unsigned char secondary_startup_64_no_verify[];
 
 static inline size_t real_mode_size_needed(void)
 {
-	if (real_mode_header)
-		return 0;	/* already allocated. */
+	if (real_mode_header || cpu_feature_enabled(X86_FEATURE_XENPV))
+		return 0;	/* already allocated or not needed. */
 
 	return ALIGN(real_mode_blob_end - real_mode_blob, PAGE_SIZE);
 }
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index 41d7669a97ad..1826700b156e 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -202,6 +202,9 @@  static void __init set_real_mode_permissions(void)
 
 static int __init init_real_mode(void)
 {
+	if (cpu_feature_enabled(X86_FEATURE_XENPV))
+		return 0;
+
 	if (!real_mode_header)
 		panic("Real mode trampoline was not allocated");