> From: Jan Beulich
> Sent: Thursday, January 27, 2022 10:50 PM
>
> The VT-d hook can indicate an error, which shouldn't be ignored. Convert
> the hook's return value to a proper error code, and let that bubble up.
>
> Signed-off-by: Jan Beulich
> ---
> I'm not convinced of the XSM related behavior here: It's neither clear
> why the check gets performed on the possible further group members
> instead of on the passed in device, nor - if the former is indeed
> intended behavior - why the loop then simply gets continued instead of
> returning an error. After all in such a case the reported "group" is
> actually incomplete, which can't result in anything good.
>
> I'm further unconvinced that it is correct for the AMD hook to never
> return an error.
I also have this question on the AMD side. In concept that check should
be x86 vendor agnostic.
but this change is good in its context:
Reviewed-by: Kevin Tian
> ---
> v2: New.
>
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1463,6 +1463,8 @@ static int iommu_get_device_group(
> return 0;
>
> group_id = iommu_call(ops, get_device_group_id, seg, bus, devfn);
> +if ( group_id < 0 )
> +return group_id;
>
> pcidevs_lock();
> for_each_pdev( d, pdev )
> @@ -1477,6 +1479,12 @@ static int iommu_get_device_group(
> continue;
>
> sdev_id = iommu_call(ops, get_device_group_id, seg, b, df);
> +if ( sdev_id < 0 )
> +{
> +pcidevs_unlock();
> +return sdev_id;
> +}
> +
> if ( (sdev_id == group_id) && (i < max_sdevs) )
> {
> bdf = (b << 16) | (df << 8);
> @@ -1484,7 +1492,7 @@ static int iommu_get_device_group(
> if ( unlikely(copy_to_guest_offset(buf, i, , 1)) )
> {
> pcidevs_unlock();
> -return -1;
> +return -EFAULT;
> }
> i++;
> }
> @@ -1552,8 +1560,7 @@ int iommu_do_pci_domctl(
> ret = iommu_get_device_group(d, seg, bus, devfn, sdevs, max_sdevs);
> if ( ret < 0 )
> {
> -dprintk(XENLOG_ERR, "iommu_get_device_group() failed!\n");
> -ret = -EFAULT;
> +dprintk(XENLOG_ERR, "iommu_get_device_group() failed: %d\n",
> ret);
> domctl->u.get_device_group.num_sdevs = 0;
> }
> else
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2564,10 +2564,11 @@ static int intel_iommu_assign_device(
> static int intel_iommu_group_id(u16 seg, u8 bus, u8 devfn)
> {
> u8 secbus;
> +
> if ( find_upstream_bridge(seg, , , ) < 0 )
> -return -1;
> -else
> -return PCI_BDF2(bus, devfn);
> +return -ENODEV;
> +
> +return PCI_BDF2(bus, devfn);
> }
>
> static int __must_check vtd_suspend(void)
