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 <[email protected]>
---
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))
+ return 0;
+
if (!real_mode_header)
panic("Real mode trampoline was not allocated");
--
2.35.3
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 <[email protected]>
> ---
> 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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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 <[email protected]>
>> ---
>> 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
On November 21, 2022 10:28:21 PM PST, Juergen Gross <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
On 23.11.22 01:07, H. Peter Anvin wrote:
> On November 21, 2022 10:28:21 PM PST, Juergen Gross <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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