| Message ID | 998f6587-d64a-7336-a44b-d05ca486b4bc@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | (mainly) IOMMU hook adjustments | expand |
On 27/01/2022 14:49, Jan Beulich wrote: > 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@suse.com> Reviewed-by: Paul Durrant <paul@xen.org>
> From: Jan Beulich <jbeulich@suse.com> > 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@suse.com> > --- > 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@intel.com> > --- > 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)
--- 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)
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@suse.com> --- 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. --- v2: New.