Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 14 Oct 2021, Ian Jackson wrote:
> (Replying to both the earlier subthread on v5 and to the new v6
> patch.)
> 
> Bertrand Marquis writes ("Re: [PATCH v5 10/11] arm/libxl: Emulated PCI device 
> tree node in libxl"):
> > Now you suggest to add a new field arm_vpci in libxl__domain_create_state.
> 
> Hi.  I was handwaving, hence "probably" :-).  I hadn't actually looked
> at the existing code to see precisely how it would fit.
> 
> > Once we have done that I will need to access this structure to know if I 
> > need
> > to add the DT part and somehow to give it a value depending something which
> > for now would the number of pcidevs as there will be no user parameter 
> > anymore.
> 
> Right.
> 
> I looked at libxl_arm.c again:
> 
> It seems that the main entrypoint to all this is libxl__prepare_dtb,
> and it is that function you want to do new stuff.  That gets
> libxl_domain_build_info (which is specified by the IDL and comes from
> outside libxl, subject to libxl's default-filling machinery) and
> libxl__domain_build_state (which is the general state struct).
> 
> The information you need is is libxl_domain_create_info.
> libxl__domain_config_setdefault already arranges to set
> c_info->passthrough based on the number of PCI PT devices
> (search for `need_pt` in libxl_create.c).
> 
> That is, as I understand it on ARM vpci should be enabled if
> passthrough is enabled and not otherwise.  That is precisely what
> the variable c_info->passthrough is.
> 
> There is a slight issue because of the distinction between create_info
> and build_info and domain_config (and, the corresponding _state)
> structs.  Currently the DT code ony gets b_info, not the whole
> libxl_domain_config.  These distinctions largely historical nowadays.
> Certainly there is no reason not to pass a pointer to the whole
> libxl_domain_config, rather than just libxl_domain_build_info, into
> libxl__arch_domain_init_hw_description.
> 
> So I think the right approach for this looks something like this:
> 
> 1. Change libxl__arch_domain_init_hw_description to take
>    libxl_domain_config* rather than libxl_domain_build_info*.
>    libxl_domain_config contains libxl_domain_build_info so
>    this is easy.
> 
>    If you put in a convenience alias variable for the
>    libxl_domain_build_info* you can avoid extra typing in the function
>    body.  (If you call the convenience alias `info` you won't need to
>    change the body at all, but maybe `info` isn't the best name so you
>    could rename it to `b_info` maybe; up to you.)
> 
> 2. Make the same change to libxl__prepare_dtb.
> 
> 3. Now you can use d_config->c_info.passthrough to gate the addition
>    of the additional stuff to the DT.  Ie, that is a respin of this
>    patch 3/3.
> 
> 1 and 2 need to be separated out from 3, in a different patch (or two
> different patches) added to this series before what is now 3/3.  Ie
> 1+2, or 1 and 2 separately, should be pure plumbing changes with no
> functional change.
> 
> I hope this is helpful.

The explanation is clear and makes sense to me too.

FYI I suggested something similar and Rahul agreed on it; in fact, he
seemed to have already made the change and tested it:
https://marc.info/?l=xen-devel&m=163362066215155

I am just mentioning it in case Bertrand might be able to find Rahul's
updated patch somewhere and it might be useful as a base for his work.

The rest looks good, including the new changes.