RE: [PATCH v2 4/4] IOMMU/PCI: propagate get_device_group_id() failure

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich <jbeulich@xxxxxxxx>
> 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 <jbeulich@xxxxxxxx>
> ---
> 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 <kevin.tian@xxxxxxxxx>

> ---
> 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, &bdf, 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, &bus, &devfn, &secbus) < 0 )
> -        return -1;
> -    else
> -        return PCI_BDF2(bus, devfn);
> +        return -ENODEV;
> +
> +    return PCI_BDF2(bus, devfn);
>  }
> 
>  static int __must_check vtd_suspend(void)