Re: [Xen-devel] [PATCH v2] VT-d: don't pass bridge devices to domain_context_mapping_one()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 20, 2020 at 05:15:22PM +0100, Jan Beulich wrote:
> On 20.01.2020 17:07, Roger Pau Monné  wrote:
> > On Mon, Jan 20, 2020 at 04:42:22PM +0100, Jan Beulich wrote:
> >> --- a/xen/drivers/passthrough/vtd/iommu.c
> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> @@ -1493,18 +1493,28 @@ static int domain_context_mapping(struct
> >>          if ( find_upstream_bridge(seg, &bus, &devfn, &secbus) < 1 )
> >>              break;
> >>  
> >> +        /*
> >> +         * Mapping a bridge should, if anything, pass the struct pci_dev 
> >> of
> >> +         * that bridge. Since bridges don't normally get assigned to 
> >> guests,
> >> +         * their owner would be the wrong one. Pass NULL instead.
> >> +         */
> >>          ret = domain_context_mapping_one(domain, drhd->iommu, bus, devfn,
> >> -                                         pci_get_pdev(seg, bus, devfn));
> >> +                                         NULL);
> >>  
> >>          /*
> >>           * Devices behind PCIe-to-PCI/PCIx bridge may generate different
> >>           * requester-id. It may originate from devfn=0 on the secondary 
> >> bus
> >>           * behind the bridge. Map that id as well if we didn't already.
> >> +         *
> >> +         * Somewhat similar as for bridges, we don't want to pass a struct
> >> +         * pci_dev here - there may not even exist one for this 
> >> (secbus,0,0)
> >> +         * tuple. If there is one, without properly working device groups 
> >> it
> >> +         * may again not have the correct owner.
> >>           */
> >>          if ( !ret && pdev_type(seg, bus, devfn) == 
> >> DEV_TYPE_PCIe2PCI_BRIDGE &&
> >>               (secbus != pdev->bus || pdev->devfn != 0) )
> >>              ret = domain_context_mapping_one(domain, drhd->iommu, secbus, 
> >> 0,
> >> -                                             pci_get_pdev(seg, secbus, 
> >> 0));
> >> +                                             NULL);
> > 
> > Isn't it dangerous to map this device to the guest, and that multiple
> > guests might end up with the same device mapped?
> 
> They won't (afaict) - see the checking done by domain_context_mapping_one()
> when it finds an already present context entry. The first one to make such
> a mapping will win.

Right, thanks, I find all this code quite confusing. If the iommu
context is assigned to a domain, won't it make sense to keep the
device in sync and also assign it to that domain?

So that the owner in the iommu context matches the owner on the
pci_dev struct.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel