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:41:17PM +0100, Jan Beulich wrote:
> On 20.01.2020 17:37, Roger Pau Monné wrote:
> > 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.
> 
> For bridges - no, I don't think so. For these "fake" (possibly phantom,
> possibly real) devices at (secbus,0,0) I don't know for sure, but - as
> the comment I'm adding says - I think this should be taken care of when
> we gain properly working device groups (at which point if this "fake"
> device is actually a real one, it should be put into the same group).

Yes, that's true. Also assigning the pci_dev to the guest could allow
the guest to actually interact with it, which is not what we actually
want.

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.

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