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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

> --- 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).

Jan


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