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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi guys,

On 15.10.2021 09:28, Julien Grall wrote:
> Hi Ian,
> 
> On 14/10/2021 18:54, 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.
> 
> On Arm, c_info->passthrough is also set when assigning platform devives (e.g. 
> a non-PCI network card). At least for now, we don't want to create a node for 
> vCPI in the Device-Tree because we don't enable the emulation. So...
> 
>>
>> 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.
> 
> ... we will need to check d_config->num_pcidevs for the time being.
> 
> Cheers,
> 

Thanks Julien for a good catch.

Do you agree on the following approach:
1. Modify argument of libxl__arch_domain_init_hw_description to 
libxl_domain_config (patch 1)
2. Modify argument of libxl__prepare_dtb to libxl_domain_config (patch 2)
3. Remove arch_arm.vpci completely and in libxl__prepare_dtb add a check (patch 
3):
if (d_config->num_pcidevs)
    FDT( make_vpci_node(gc, fdt, ainfo, dom) );
+ make_vpci_node implementation

Does it sound ok for you?

Cheers,
Michal