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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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?

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

v/r,
dps