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

[Stefano Stabellini <sstabellini@kernel.org>]

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.

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)