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.
>> +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.
>> + char *boot_loader_name;
>> + char *kextra;
>
> const?
I want to say const will not work based on usage, but I will double-check.
