Re: [PATCH v1 04/18] x86: refactor entrypoints to new boot info

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

On 7/18/22 09:58, Smith, Jackson wrote:
> Hi Daniel,
> 
> I hope outlook gets this reply right.

Looks good to me, thank you for taking the time to review.

>> -----Original Message-----
>> Subject: [PATCH v1 04/18] x86: refactor entrypoints to new boot info
> 
>> diff --git a/xen/arch/x86/guest/xen/pvh-boot.c
>> b/xen/arch/x86/guest/xen/pvh-boot.c
>> index 834b1ad16b..28cf5df0a3 100644
>> --- a/xen/arch/x86/guest/xen/pvh-boot.c
>> +++ b/xen/arch/x86/guest/xen/pvh-boot.c
> 
>> @@ -99,13 +118,16 @@ static void __init get_memory_map(void)
>>      sanitize_e820_map(e820_raw.map, &e820_raw.nr_map);
>>  }
>>
>> -void __init pvh_init(multiboot_info_t **mbi, module_t **mod)
>> +void __init pvh_init(struct boot_info **bi)
>>  {
>> -    convert_pvh_info(mbi, mod);
>> +    *bi = init_pvh_info();
>> +    convert_pvh_info(*bi);
>>
>>      hypervisor_probe();
>>      ASSERT(xen_guest);
>>
>> +    (*bi)->arch->xen_guest = xen_guest;
> 
> I think you may have a typo/missed refactoring here?
> I changed this line to "(*bi)->arch->xenguest = xen_guest;" to get the 
> patchset to build.

Hmm, I guess I missed one. I originally was going to mimic the name
xen_guest in the structure definition but when xen guest support is
disable the xen_guest global turns into a #define which replaces the
reference resulting in a compilation error.

> The arch_boot_info struct in boot_info32.h has a field 'xen_guest' but the 
> same field in asm/bootinfo.h was re-named from 'xen_guest' to 'xenguest' in 
> the 'x86: adopt new boot info structures' commit.
> 
> What was your intent?

As I mentioned above, the renaming was intentional, and it looks like I
do a poor job catching everywhere where the renaming need to be done.

>> +
>>      get_memory_map();
>>  }
>>
> 
> Thanks,
> Jackson Smith

v/r,
dps