Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl

[Julien Grall <julien@xxxxxxx>]

Hi Rahul,

On 06/10/2021 19:34, Rahul Singh wrote:
> > On 6 Oct 2021, at 12:33 pm, Ian Jackson <iwj@xxxxxxxxxxxxxx> wrote:
> >
> > Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node 
> > in libxl"):
> > > Hi Ian  
> > > > What is wrong with putting it in
> > > > libxl__arch_domain_build_info_setdefault
> > > > which I think exists precisely for this kind of thing ?
> > > As we have to set the arch_arm.vpci to false for x86 and ARM I
> > > thought it is right to move the code to common code to avoid
> > > duplication.
> > >
> > > Are you suggesting to put "
> > > libxl_defbool_setdefault(&b_info->arch_arm.vpci, false)ïżœin
> > > libxl__arch_domain_build_info_setdefault() for x86 and ARM
> > > differently.
> > I've gone back and reread the whole thread, which I probably should
> > have done to start with....
> >
> > So:
> >
> > > > > #if defined(__arm__) || defined(__aarch64__)
> > > > >    /*
> > > > >     * Enable VPCI support for ARM. VPCI support for DOMU guests is not
> > > > >     * supported for x86.
> > > > >     */
> > > > >    if (d_config->num_pcidevs)
> > > > >      libxl_defbool_set(&b_info->arch_arm.vpci, true);
> > > > > #endif
> > I think this logic probably ought to be in libxl, not in xl.
> I will move the code to "libxl_arm.c"to avoid #ifdef in common code and also  
> to avoid setting the vpci for x86
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6e00..2be208b99b 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -101,6 +101,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>           return ERROR_FAIL;
>       }
>   
> +    /* Enable VPCI support. */
> +    if (d_config->num_pcidevs) {
> +        config->flags |= XEN_DOMCTL_CDF_vpci;
> +        libxl_defbool_set(&d_config->b_info.arch_arm.vpci, true);
> +    }
> +
>       return 0;
>   }
>
> >   We try
> > to make the libxl API "do the right thing" by default.  In this case I
> > think that means to enable VPCI (i) on platforms where it's available
> > (ii) if the guest has PCI passthrough devices.  Is that right ?
> Yes you are right VPCI will be enabled for guest when guest has PCI passthrough 
> device
> assigned and VPCI support is available.
> > Sorry to ask these question now, and please forgive my ignorance:
> >
> > Is VPCI inherently an ARM-specific ABI or protocol ?
> As of now VPCI for DOMU guests is only implemented  for ARM.
We need to differentiate between what it is currently implemented and 
how it can be used in the future.
In particular, the layout of b_info is exposed to external toolstack 
(e.g. libvirt). So we can't easily remove an option. In other word, if 
we end up to need it for an other arch then we will have to keep some 
compat code.
In this case, I think this option is not arm specific. So the field 
ought to be outside of arch_arm.
>   
> >   When might an
> > admin want to turn it on explicitly ?
> It will be enabled dynamically when admin assign any PCI device to guest.
>
> > How does this all relate to the (non-arch-specific) "passthrough"
> > option ?
> VPCI will be enabled only when there is any PCI device assigned to guest 
> therefore I used
> "d_config->num_pcidevs” to enable VPCI.
Ok. So we don't expect 'xl' or another toolstack to effectively touch 
the field for the time being. Is that correct?
If so, then I think this option should be hidden from external toolstack 
until we see a use.
Cheers,

--
Julien Grall