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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> +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.

Jan