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
