Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall <julien.grall@arm.com>]

Hi Wei,

On 28/08/18 17:45, Wei Liu wrote:
> On Thu, Aug 23, 2018 at 09:58:57AM +0200, Roger Pau Monné wrote:
> [...]
>>
>>> What I wanted to do here is resetting the union to 0 so you don't get data
>>> mangled by the pv fields.
>>
>> Another possible option I think would be to mark those fields as
>> deprecated in the IDL, and libxl__domain_build_info_copy_deprecated
> 
> I think this is a better approach.
> 
>> will take care of copying them to the new place. In fact I think all
>> guest types should be using the top level kernel, ramdisk and cmdline
>> fields.
>>
>> I'm not specially comfortable with changing the guest type in the
>> middle of libxl__domain_build_info_setdefault, but I also don't have a
>> much better suggestion apart from using the deprecation helper.
>>
>>  From what you say above I assume bootloader or bootloader arguments
>> are not used by ARM?
>>
>>>>
>>>>>    }
>>>>>    /*
>>>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>>>>> index d4fa06daea..a6431c5d3f 100644
>>>>> --- a/tools/libxl/libxl_create.c
>>>>> +++ b/tools/libxl/libxl_create.c
>>>>> @@ -215,7 +215,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>>>>        if (!b_info->event_channels)
>>>>>            b_info->event_channels = 1023;
>>>>> -    libxl__arch_domain_build_info_setdefault(b_info);
>>>>> +    libxl__arch_domain_build_info_setdefault(gc, b_info);
>>>>>        libxl_defbool_setdefault(&b_info->dm_restrict, false);
>>>>>        switch (b_info->type) {
>>>>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>>>>> index 81523a568f..8b6759c089 100644
>>>>> --- a/tools/libxl/libxl_x86.c
>>>>> +++ b/tools/libxl/libxl_x86.c
>>>>> @@ -613,7 +613,8 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>>>>>        return rc;
>>>>>    }
>>>>> -void libxl__arch_domain_build_info_setdefault(libxl_domain_build_info *b_info)
>>>>> +void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>>>>> +                                              libxl_domain_build_info *b_info)
>>>>>    {
>>>>>        libxl_defbool_setdefault(&b_info->acpi, true);
>>>>>    }
>>>>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>>>>> index 971ec1bc56..0bda28152b 100644
>>>>> --- a/tools/xl/xl_parse.c
>>>>> +++ b/tools/xl/xl_parse.c
>>>>> @@ -1286,7 +1286,11 @@ void parse_config_data(const char *config_source,
>>>>>        }
>>>>>        if (c_info->type == LIBXL_DOMAIN_TYPE_INVALID)
>>>>> +#if defined(__arm__) || defined(__aarch64__)
>>>>
>>>> I think #ifdef CONFIG_ARM should DTRT and it's cleaner IMO.
>>>
>>> CONFIG_ARM is not defined in the tools C source. So that's the only way to
>>> know if you are on Arm. This follows what is done in libxc.
>>>
>>> I would be happy to introduce CONFIG_ARM/CONFIG_X86 if people thinks this
>>> would be useful in other places.
>>
>> The tools makefile already uses CONFIG_ARM/X86, so I think it would
>> make sense to have this for the code as well. In any case, I don't
>> feel this should be done just for this patch, so I'm fine as-is.
>>
> 
> I think CONFIG_ARM should already work.

I don't think so. The top makefile does not pass -DCONFIG_ARM on 
compiler command line. This is only done in sub-directory such as 
console and libacpi.

> 
> There are several CONFIG_ARM* in toolstack code, though not in libxl. In
> any case, I think the code is fine as-is.

I am happy to introduce CONFIG_ARM/CONFIG_X86 if you don't mind adding 
-DCONFIG_ARM on the command line.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel