[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.