Re: [PATCH v5 01/10] x86: Replace arch-specific boot_module with common one

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed Jul 2, 2025 at 2:43 PM CEST, Jan Beulich wrote:
> On 01.07.2025 12:56, Alejandro Vallejo wrote:
>> These types resemble each other very closely in layout and intent,
>> and with "struct boot_module" already in common code it makes perfect
>> sense to merge them. In order to do so, add an arch-specific area for
>> x86-specific tidbits, and rename identical fields with conflicting
>> names.
>> 
>> No functional change intended.
>> 
>> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> I'm largely okay with this change, just one question:
>
>> --- a/xen/include/xen/bootfdt.h
>> +++ b/xen/include/xen/bootfdt.h
>> @@ -7,6 +7,10 @@
>>  #include <xen/macros.h>
>>  #include <xen/xmalloc.h>
>>  
>> +#if __has_include(<asm/bootfdt.h>)
>> +#include <asm/bootfdt.h>
>> +#endif
>> +
>>  #define MIN_FDT_ALIGN 8
>>  
>>  #define NR_MEM_BANKS 256
>> @@ -108,6 +112,10 @@ struct boot_module {
>>      bool domU;
>>      paddr_t start;
>>      paddr_t size;
>> +
>> +#if __has_include(<asm/bootfdt.h>)
>> +    struct arch_boot_module arch;
>> +#endif
>>  };
>
> The pre-existing domU field isn't used by x86. Shouldn't that move into Arm's
> (to be created) struct arch_boot_module? Or is that intended to become
> dependent upon CONFIG_DOM0LESS_BOOT? (While we apparently didn't adopt Misra
> rule 2.2, this is imo precisely the situation where we would better follow it:
> An unused struct field raises questions.)

That can be moved to an arch-specific header, yes.

I expect that domU field to eventually drop after dom0less adopts the
more powerful hyperlaunch bindings for privilege separation. At that point
it doesn't matter whether a domain is a domU or not, it's jut a domain to be
constructed.

Cheers,
Alejandro