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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 6 Oct 2021, at 6:53 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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.

As we are discussing whether we need this field or not If we reach to the 
conclusion the we need this field I will move this 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?

Yes it is correct. Only use of this field is used to create the DOMU emulated 
PCI device tree node when PCI device is assigned to guest ( 
d_config->num_pcidevs != NULL )
> 
> If so, then I think this option should be hidden from external toolstack 
> until we see a use.

Stefano suggested in another email how we can remove this field. I will work on 
to remove this field.

Regards,
Rahul 
> 
> Cheers,
> 
> -- 
> Julien Grall