Re: [PATCH v4 4/5] [FUTURE] xen/arm: enable vPCI for domUs

[Stewart Hildebrand <stewart.hildebrand@amd.com>]

On 10/31/23 09:17, Julien Grall wrote:
> Hi,
> 
> On 31/10/2023 11:03, Jan Beulich wrote:
>> On 31.10.2023 00:52, Stewart Hildebrand wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1618,6 +1618,16 @@ int iommu_do_pci_domctl(
>>>           bus = PCI_BUS(machine_sbdf);
>>>           devfn = PCI_DEVFN(machine_sbdf);
>>>   +        if ( IS_ENABLED(CONFIG_ARM) &&
>>> +             !is_hardware_domain(d) &&
>>> +             !is_system_domain(d) &&
>>> +             (!IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) || !has_vpci(d)) )
>>
>> I don't think you need the explicit ARM check; that's redundant with
>> checking !HAS_VPCI_GUEST_SUPPORT.

Currently that is true. However, this is allowing for the possibility that we eventually may want to enable PCI passthrough for PVH domU using vPCI (e.g. hyperlaunch, or eliminating qemu backend), in which case we may want to enable CONFIG_HAS_VPCI_GUEST_SUPPORT=y on x86.

>> It's also not really clear why you
>> need to check for the system domain here.

xl pci-assignable-add will assign the device to domIO, which doesn't have vPCI, but it is still a valid assignment. Perhaps an in code comment would be helpful for clarity?

> 
> I might be missing but I wouldn't expect the domain to have vPCI enabled if CONFIG_HAVE_VPCI_GUEST_SUPPORT=n. So why can't this simply be:
> 
> if ( !has_vcpi(d) )
> {
>    ...
> }

Right, the CONFIG_HAVE_VPCI_GUEST_SUPPORT check here is not strictly needed because this case is already caught by the other half of this patch in xen/arch/arm/vpci.c. This simplifies it to:

    if ( IS_ENABLED(CONFIG_ARM) &&
         !is_hardware_domain(d) &&
         !is_system_domain(d) /* !domIO */ &&
         !has_vpci(d) )

On x86, unless I misunderstood something, I think it's valid to assign PCI devices to a domU without has_vpci().

BTW, it's valid for has_vpci() to be true and CONFIG_HAVE_VPCI_GUEST_SUPPORT=n for dom0.