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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan

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