Re: [PATCH 2/3] x86/boot: Refactor BIOS/PVH start

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.09.2024 10:02, Frediano Ziglio wrote:
> On Sun, Sep 15, 2024 at 7:16 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 10.09.2024 18:16, Frediano Ziglio wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -25,6 +25,9 @@
>>>  #define MB2_HT(name)      (MULTIBOOT2_HEADER_TAG_##name)
>>>  #define MB2_TT(name)      (MULTIBOOT2_TAG_TYPE_##name)
>>>
>>> +#define BOOT_TYPE_BIOS 1
>>> +#define BOOT_TYPE_PVH 2
>>
>> Did you consider using 0 and 1, to be able to use XOR on the BIOS
>> path and TEST for checking?
>>
> 
> Not clear what you are trying to achieve. Fewer bytes using the XOR? I
> think the TEST in this case is only reducing readability, it's an
> enumeration.

Except that in practice we don't really need an enum here; a boolean
would suffice.

> If you are concerns about code size I would use an 8 bit register (I
> would say DL) and use EBP register to temporary save EAX, 8 bit
> registers have usually tiny instructions, MOV has same size as XOR you
> mentioned not loosing any readability or forcing to change values.

No, it's not code size alone. It's larger code size for no good reason.
Using 8-bit ops is an option when you're truly code size constrained,
yet that's not the case here. Hence my take is - use the cheapest insns
possible.

>>> +        cld
>>
>> So you fold the STDs but not the STIs, despite that not even having
>> been first on the PVH path. This decision wants explaining in the
>> description, even if just briefly.
> 
> Just in case, I disable interrupts ASAP. Not that this should change
> much the result.
> Would you prefer to fold it too?

That or explain the difference.

> By "description" do you mean having an additional comment in the code
> or something in the commit message?

Sorry, I use "description" as a common synonym for "commit message".

>>> @@ -433,14 +451,9 @@ __pvh_start:
>>>          /* Set up stack. */
>>>          lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
>>>
>>> -        call    initialise_bss
>>
>> I'm little puzzled: The previous patch moved it "as early as possible"
>> just for it to be moved to a later point again here?
>>
> 
> The rationale is being able to use C, for that you need a stack,
> correct segments and BSS.
> Are you suggesting any change?

I'm trying to understand the sum effect of both changes, which appear
to be moving in opposite direction.

Jan