Re: [PATCH] x86/boot: skip realmode init code when running as Xen PV guest

[Juergen Gross <jgross@suse.com>]

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

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

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

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