Re: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.09.11 at 16:20, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> On some systems, requests devices behind a PCIe-to-PCI bridge all
> appear to the IOMMU as though they come from from slot 0, function
> 0 on that device; so the mapping code much punch a hole for X:0.0
> in the IOMMU for such devices.  When punching the hole, if that device
> has already been mapped once, we simply need to check ownership to
> make sure it's legal.  To do so, domain_context_mapping_one() will look
> up the device for the mapping with pci_get_pdev() and look for the owner.
> 
> However, if there is no device in X:0.0, this look up will fail.

Was it really that there was no device at all at X:0.0, or rather that
Xen just didn't know about the device (because Dom0 failed to notify
Xen, as could happen in the 2.6.18-derived trees up to pretty
recently)?

Also, didn't we sort of agree that creating a phantom device would
be more elegant (or at least much smaller a change)?

Jan

> Rather than returning -ENODEV in this situation (causing a failure in
> mapping the device), try to get the domain ownership from the iommu context
> mapping itself.
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> diff -r 4a4882df5649 -r ede81b0552be xen/drivers/passthrough/vtd/iommu.c
> --- a/xen/drivers/passthrough/vtd/iommu.c     Wed Aug 31 15:23:49 2011 +0100
> +++ b/xen/drivers/passthrough/vtd/iommu.c     Thu Sep 01 15:18:18 2011 +0100
> @@ -113,6 +113,27 @@ static int context_set_domain_id(struct 
>      return 0;
>  }
>  
> +static int context_get_domain_id(struct context_entry *context,
> +                                 struct iommu *iommu)
> +{
> +    unsigned long dom_index, nr_dom;
> +    int domid = -1;
> +
> +    if (iommu && context)
> +    {
> +        nr_dom = cap_ndoms(iommu->cap);
> +
> +        dom_index = context_domain_id(*context);
> +
> +        if ( dom_index < nr_dom && iommu->domid_map)
> +            domid = iommu->domid_map[dom_index];
> +        else
> +            dprintk(XENLOG_DEBUG VTDPREFIX, "%s: dom_index %lu exceeds 
> nr_dom %lu or iommu has no domid_map\n",
> +                    __func__, dom_index, nr_dom);
> +    }
> +    return domid;
> +}
> +
>  static struct intel_iommu *__init alloc_intel_iommu(void)
>  {
>      struct intel_iommu *intel;
> @@ -1237,7 +1258,6 @@ int domain_context_mapping_one(
>      struct hvm_iommu *hd = domain_hvm_iommu(domain);
>      struct context_entry *context, *context_entries;
>      u64 maddr, pgd_maddr;
> -    struct pci_dev *pdev = NULL;
>      int agaw;
>  
>      ASSERT(spin_is_locked(&pcidevs_lock));
> @@ -1249,12 +1269,45 @@ int domain_context_mapping_one(
>      if ( context_present(*context) )
>      {
>          int res = 0;
> +        struct pci_dev *pdev = NULL;
>  
> +        /* First try to get domain ownership from device structure.  If 
> that's
> +         * not available, try to read it from the context itself. */
>          pdev = pci_get_pdev(bus, devfn);
> -        if (!pdev)
> -            res = -ENODEV;
> -        else if (pdev->domain != domain)
> -            res = -EINVAL;
> +        if ( pdev )
> +        {
> +            if ( pdev->domain != domain )
> +            {
> +                dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x owned 
> by d%d!",
> +                        domain->domain_id, 
> +                        bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> +                        (pdev->domain)
> +                        ? pdev->domain->domain_id : -1);
> +                res = -EINVAL;
> +            }
> +        }
> +        else
> +        {
> +            int cdomain;
> +            cdomain = context_get_domain_id(context, iommu);
> +            
> +            if ( cdomain < 0 )
> +            {
> +                dprintk(VTDPREFIX, "d%d: bdf = %x:%x.%x mapped, but can't 
> find owner!\n",
> +                        domain->domain_id, 
> +                        bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> +                res = -EINVAL;
> +            }
> +            else if ( cdomain != domain->domain_id )
> +            {
> +                dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x already 
> mapped to d%d!",
> +                        domain->domain_id, 
> +                        bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> +                        cdomain);
> +                res = -EINVAL;
> +            }
> +        }
> +
>          unmap_vtd_domain_page(context_entries);
>          spin_unlock(&iommu->lock);
>          return res;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx 
> http://lists.xensource.com/xen-devel 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel