Re: [Xen-devel] [PATCH v3 2/2] VT-d: Fix vt-d flush timeout issue.

["Xu, Quan" <quan.xu@xxxxxxxxx>]

> On 16.12.2015 at 4:08pm, <JBeulich@xxxxxxxx> wrote:
> >>> On 16.12.15 at 04:51, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -1318,6 +1318,25 @@ int iommu_remove_device(struct pci_dev *pdev)
> >      return hd->platform_ops->remove_device(pdev->devfn,
> > pci_to_dev(pdev));  }
> >
> > +int iommu_hide_device(struct pci_dev *pdev) {
> > +    if ( !pdev || !pdev->domain )
> > +        return -EINVAL;
> > +
> > +    spin_lock(&pcidevs_lock);
> > +    pdev->domain = dom_xen;
> > +    list_add(&pdev->domain_list, &dom_xen->arch.pdev_list);
> > +    spin_unlock(&pcidevs_lock);
> > +
> > +    return 0;
> > +}
> 
> This is effectively a copy of pci_hide_device(), and is misnamed (since it 
> takes a
> PCI device as argument). I do not see why you shouldn't be able to use
> pci_hide_device() after removing its __init annotation or a suitably named
> wrapper around _pci_hide_device(). Not specifically that the way you do this
> right now you corrupt the original owning domain's PCI device list - you need 
> to
> remove the device from that list before adding it to dom_xen's (which then 
> will
> naturally entail clearing ->domain, at once satisfying _pci_hide_device()'s 
> early
> check, which is there for the very reason of ensuring not to corrupt any 
> list).
>

You are correct.
As the _pci_hide_device()'s early check, I didn't use it. 
Could I remove the device from that list before adding it to dom_xen's, and 
reuse
pci_hide_device() as below?


+            list_del(&pdev->domain_list);
+            pdev->domain = NULL;
+            if ( pci_hide_device(bus, devfn) )
+            {
+                printk(XENLOG_ERR
+                       "IOMMU hide device %04x:%02x:%02x error.",
+                       seg, bus, devfn);
+                break;
+            }



> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1890,6 +1890,9 @@ static int intel_iommu_add_device(u8 devfn, struct
> pci_dev *pdev)
> >      if ( !pdev->domain )
> >          return -EINVAL;
> >
> > +    if ( pdev->domain == dom_xen )
> > +        return -EACCES;
> 
> I'm not sure about the need for this check, ...
> 
> > @@ -2301,6 +2304,9 @@ static int intel_iommu_assign_device(
> >      if ( list_empty(&acpi_drhd_units) )
> >          return -ENODEV;
> >
> > +    if ( pdev->domain == dom_xen )
> > +        return -EACCES;
> 
> ... and I clearly don't see the need for this one. Please explain, keeping in 
> mind
> that generic IOMMU code should be enforcing things like this (and at least in 
> the
> assign case should already be doing so).

I have verified it. the above 2 checks are excessive protection. I can remove 
it.

((Jan, thanks very much! Sometime it is not just for technology help.))
Quan






_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel