Re: [PATCH v1 02/18] introduction of generalized boot info

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.07.2022 18:01, Daniel P. Smith wrote:
> On 7/21/22 12:00, Jan Beulich wrote:
>> On 21.07.2022 16:28, Daniel P. Smith wrote:
>>> On 7/19/22 09:11, Jan Beulich wrote:
>>>> On 06.07.2022 23:04, Daniel P. Smith wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/include/asm/bootinfo.h
>>>>> @@ -0,0 +1,48 @@
>>>>> +#ifndef __ARCH_X86_BOOTINFO_H__
>>>>> +#define __ARCH_X86_BOOTINFO_H__
>>>>> +
>>>>> +/* unused for x86 */
>>>>> +struct arch_bootstring { };
>>>>> +
>>>>> +struct __packed arch_bootmodule {
>>>>> +#define BOOTMOD_FLAG_X86_RELOCATED      1U << 0
>>>>
>>>> Such macro expansions need parenthesizing.
>>>
>>> Ack.
>>>
>>>>> +    uint32_t flags;
>>>>> +    uint32_t headroom;
>>>>> +};
>>>>
>>>> Since you're not following any external spec, on top of what Julien
>>>> said about the __packed attribute I'd also like to point out that
>>>> in many cases here there's no need to use fixed-width types.
>>>
>>> Oh, I forgot to mention that in the reply to Julien. Yes, the __packed
>>> is needed to correctly cross the 32bit to 64bit bridge from the x86
>>> bootstrap in patch 4.
>>
>> I'm afraid I don't follow you here. I did briefly look at patch 4 (but
>> that really also falls in the "wants to be split" category), but I
>> can't see why a purely internally used struct may need packing. I'd
>> appreciate if you could expand on that.
> 
> Originally, patch 3 and patch 4 were a single patch, and obviously was
> way too large. To split them, I realized I could introduce a temporary
> conversion function that would allow the patch to be split into a post
> start_xen() patch (patch 3) and a pre start_xen() patch, (patch 4). For
> x86, pre start_xen() consists of 3 different entry points. These being
> the classic/traditional/old multiboot1/2 entry, EFI entry, and PVH entry
> (aka Xen Guest). The latter two are all internal, 64bit, but the former
> is located in arch/x86/boot and is compiled as 32bit. I tried different
> approaches to support using a single header between these two
> environments. Ultimately, IMHO, the cleanest approach is what is
> introduced in patch 4 as it enabled the use of Xen types in the
> structures and maintain a single structure that need to be passed
> around. To do this, a 32bit specific version of the structures were
> defined in arch/x86/boot/boot_info32.h that is populated under 32bit
> mode, then they can be fixed up after getting into start_xen() and in
> 64bit code. To ensure no unexpected insertion of padding, I focused on
> ensuring everything was 32bit aligned and packed. As Julien pointed out,
> I messed up with the use of enum as its size is not guaranteed as the
> enum list grows and I forgot to consider keeping pointers 64bit aligned.
> 
> Does that help?

It helps as background info, yes, but I continue to be unhappy with the
new uses of the __packed attribute.

>>>>> +struct __packed arch_boot_info {
>>>>> +    uint32_t flags;
>>>>> +#define BOOTINFO_FLAG_X86_MEMLIMITS      1U << 0
>>>>> +#define BOOTINFO_FLAG_X86_BOOTDEV        1U << 1
>>>>> +#define BOOTINFO_FLAG_X86_CMDLINE        1U << 2
>>>>> +#define BOOTINFO_FLAG_X86_MODULES        1U << 3
>>>>> +#define BOOTINFO_FLAG_X86_AOUT_SYMS      1U << 4
>>>>> +#define BOOTINFO_FLAG_X86_ELF_SYMS       1U << 5
>>>>> +#define BOOTINFO_FLAG_X86_MEMMAP         1U << 6
>>>>> +#define BOOTINFO_FLAG_X86_DRIVES         1U << 7
>>>>> +#define BOOTINFO_FLAG_X86_BIOSCONFIG     1U << 8
>>>>> +#define BOOTINFO_FLAG_X86_LOADERNAME     1U << 9
>>>>> +#define BOOTINFO_FLAG_X86_APM            1U << 10
>>>>> +
>>>>> +    bool xen_guest;
>>>>
>>>> As the example of this, with just the header files being introduced
>>>> here it is not really possible to figure what these fields are to
>>>> be used for and hence whether they're legitimately represented here.
>>>
>>> I can add a comment to clarify these are a mirror of the multiboot
>>> flags. These were mirrored to allow the multiboot flags to be direct
>>> copied and eased the replacement locations where an mb flag is checked.
>>
>> Multiboot flags? The context here is the "xen_guest" field.
> 
> Apologies, I thought you were referring to all the fields and I forgot
> to explain xen_guest. So to clarify, flags is to carry the MB flags
> passed up from the MB entry point and xen_guest is meant to carry the
> xen_guest bool passed up from the PVH/Xen Guest entry point.

That was my guess, but then my request stands: The fields should be
added to the struct at the time they're being made use of.

Jan