Re: [PATCH v5 10/11] arm/libxl: Emulated PCI device tree node in libxl

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

> On 7 Oct 2021, at 1:26 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 6 Oct 2021, Julien Grall wrote:
>> Hi Rahul,
>> 
>> On 06/10/2021 19:40, Rahul Singh wrote:
>>> diff --git a/tools/libs/light/libxl_types.idl
>>> b/tools/libs/light/libxl_types.idl
>>> index 3f9fff653a..78b1ddf0b8 100644
>>> --- a/tools/libs/light/libxl_types.idl
>>> +++ b/tools/libs/light/libxl_types.idl
>>> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>>        ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>>                                 ("vuart", libxl_vuart_type),
>>> +                               ("vpci", libxl_defbool),
>> 
>> I have posted some comments regarding the field in v4. To summarize, AFAICT,
>> this option is meant to be only set by libxl but you still let the toolstack
>> (e.g. xl, libvirt) to set it.
>> 
>> If you still want to expose to the toolstack, then I think the option should
>> be outside of arch_arm. Otherwise, this should be moved in an internal
>> structure (Ian, do you have any suggestion?).
> 
> 
> First let me premise that the patch is much better already and Rahul
> addessed my request well. However, Julien's point about not wanting to
> make a potentially breaking ABI change in libxl is a good one. FYI we
> had a few libvirt breakages due to this kind of changes in the past and
> I would certainly be happier if we didn't cause another one. And in
> general, it is better to avoid changes to the libxl ABI if we can.
> 
> I think in this case we can: I looked at the patch and
> b_info.arch_arm.vpci is only used within tools/libs/light/libxl_arm.c.
> Also, we don't need b_info.arch_arm.vpci if we can access
> d_config->num_pcidevs given that the check is just based on
> d_config->num_pcidevs.
> 
> So the only issue is how to check on d_config->num_pcidevs in
> libxl__prepare_dtb. libxl__prepare_dtb takes libxl_domain_build_info as
> parameter but with container_of we can retrieve libxl_domain_config and
> from there check on num_pcidevs.
> 
> Something like the appended (untested). It doesn't need any libxl struct
> changes but it requires the introduction of container_of (which is a
> simple macro). Alternatively, it would be just as simple to change
> libxl__arch_domain_init_hw_description and libxl__prepare_dtb to take a
> libxl_domain_config *d_config parameter instead of a
> libxl_domain_build_info *info parameter.

Thanks for sharing the ideas to remove the arch_arm.vpci field. 
I am ok with any options, but I feel second option is simple and better to 
avoid  
introduction of container_of(). I tested the second option and is working fine.
If everyone agree that we don’t need vpci option I will send the patch for 
review
In next version.

Regards,
Rahul
> 
> Ian, any thoughts?
> 
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2be208b99b..ee1176519c 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -102,10 +102,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>     }
> 
>     /* Enable VPCI support. */
> -    if (d_config->num_pcidevs) {
> +    if (d_config->num_pcidevs)
>         config->flags |= XEN_DOMCTL_CDF_vpci;
> -        libxl_defbool_set(&d_config->b_info.arch_arm.vpci, true);
> -    }
> 
>     return 0;
> }
> @@ -976,6 +974,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, 
> libxl_domain_build_info *info,
> 
>     const libxl_version_info *vers;
>     const struct arch_info *ainfo;
> +    libxl_domain_config *d_config = container_of(info, libxl_domain_config, 
> b_info);
> 
>     vers = libxl_get_version_info(CTX);
>     if (vers == NULL) return ERROR_FAIL;
> @@ -1076,7 +1075,7 @@ next_resize:
>         if (info->tee == LIBXL_TEE_TYPE_OPTEE)
>             FDT( make_optee_node(gc, fdt) );
> 
> -        if (libxl_defbool_val(info->arch_arm.vpci))
> +        if (d_config->num_pcidevs)
>             FDT( make_vpci_node(gc, fdt, ainfo, dom) );
> 
>         if (pfdt)