[PATCH] VT-d: fix PCI device detach from virtual machine

[PATCH] VT-d: fix PCI device detach from virtual machine

[Han, Weidong]

[-- Attachment #1: Type: text/plain, Size: 2461 bytes --]

When assign a device behind conventional PCI bridge or PCIe to
PCI/PCI-x bridge to a domain, it must assign its bridge and may
also need to assign secondary interface to the same domain. 

Dependent assignment is already there, but dependent
deassignment is missed when detach device from virtual machine.
This results in conventional PCI device assignment failure after
it has been assigned once. This patch addes dependent
deassignment, and fixes the issue.


Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 drivers/pci/intel-iommu.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index f3f6865..d21a1a5 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2772,6 +2772,33 @@ static int vm_domain_add_dev_info(struct dmar_domain *domain,
 	return 0;
 }
 
+static void iommu_detach_dependent_devices(struct intel_iommu *iommu,
+					   struct pci_dev *pdev)
+{
+	struct pci_dev *tmp, *parent;
+
+	if (!iommu || !pdev)
+		return;
+
+	/* dependent device detach */
+	tmp = pci_find_upstream_pcie_bridge(pdev);
+	/* Secondary interface's bus number and devfn 0 */
+	if (tmp) {
+		parent = pdev->bus->self;
+		while (parent != tmp) {
+			iommu_detach_dev(iommu, parent->bus->number,
+				parent->devfn);
+			parent = parent->bus->self;
+		}
+		if (tmp->is_pcie) /* this is a PCIE-to-PCI bridge */
+			iommu_detach_dev(iommu,
+				tmp->subordinate->number, 0);
+		else /* this is a legacy PCI bridge */
+			iommu_detach_dev(iommu,
+				tmp->bus->number, tmp->devfn);
+	}
+}
+
 static void vm_domain_remove_one_dev_info(struct dmar_domain *domain,
 					  struct pci_dev *pdev)
 {
@@ -2797,6 +2824,7 @@ static void vm_domain_remove_one_dev_info(struct dmar_domain *domain,
 			spin_unlock_irqrestore(&device_domain_lock, flags);
 
 			iommu_detach_dev(iommu, info->bus, info->devfn);
+			iommu_detach_dependent_devices(iommu, pdev);
 			free_devinfo_mem(info);
 
 			spin_lock_irqsave(&device_domain_lock, flags);
@@ -2846,6 +2874,7 @@ static void vm_domain_remove_all_dev_info(struct dmar_domain *domain)
 
 		iommu = device_to_iommu(info->bus, info->devfn);
 		iommu_detach_dev(iommu, info->bus, info->devfn);
+		iommu_detach_dependent_devices(iommu, info->dev);
 
 		/* clear this iommu in iommu_bmp, update iommu count
 		 * and coherency
-- 
1.6.0.4

[-- Attachment #2: 0001-VT-d-fix-PCI-device-detach-from-virtual-machine.patch --]
[-- Type: application/octet-stream, Size: 2609 bytes --]

From 13c25cbfb13c14a41c228c88c2c5bd45e6ed347b Mon Sep 17 00:00:00 2001
From: Weidong Han <weidong.han@intel.com>
Date: Wed, 25 Feb 2009 16:58:44 +0800
Subject: [PATCH] VT-d: fix PCI device detach from virtual machine 

When assign a device behind conventional PCI bridge or PCIe to
PCI/PCI-x bridge to a domain, it must assign its bridge and may
also need to assign secondary interface to the same domain. 

Dependent assignment is already there, but dependent
deassignment is missed when detach device from virtual machine.
This results in conventional PCI device assignment failure after
it has been assigned once. This patch addes dependent
deassignment, and fixes the issue.


Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 drivers/pci/intel-iommu.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index f3f6865..d21a1a5 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2772,6 +2772,33 @@ static int vm_domain_add_dev_info(struct dmar_domain *domain,
 	return 0;
 }
 
+static void iommu_detach_dependent_devices(struct intel_iommu *iommu,
+					   struct pci_dev *pdev)
+{
+	struct pci_dev *tmp, *parent;
+
+	if (!iommu || !pdev)
+		return;
+
+	/* dependent device detach */
+	tmp = pci_find_upstream_pcie_bridge(pdev);
+	/* Secondary interface's bus number and devfn 0 */
+	if (tmp) {
+		parent = pdev->bus->self;
+		while (parent != tmp) {
+			iommu_detach_dev(iommu, parent->bus->number,
+				parent->devfn);
+			parent = parent->bus->self;
+		}
+		if (tmp->is_pcie) /* this is a PCIE-to-PCI bridge */
+			iommu_detach_dev(iommu,
+				tmp->subordinate->number, 0);
+		else /* this is a legacy PCI bridge */
+			iommu_detach_dev(iommu,
+				tmp->bus->number, tmp->devfn);
+	}
+}
+
 static void vm_domain_remove_one_dev_info(struct dmar_domain *domain,
 					  struct pci_dev *pdev)
 {
@@ -2797,6 +2824,7 @@ static void vm_domain_remove_one_dev_info(struct dmar_domain *domain,
 			spin_unlock_irqrestore(&device_domain_lock, flags);
 
 			iommu_detach_dev(iommu, info->bus, info->devfn);
+			iommu_detach_dependent_devices(iommu, pdev);
 			free_devinfo_mem(info);
 
 			spin_lock_irqsave(&device_domain_lock, flags);
@@ -2846,6 +2874,7 @@ static void vm_domain_remove_all_dev_info(struct dmar_domain *domain)
 
 		iommu = device_to_iommu(info->bus, info->devfn);
 		iommu_detach_dev(iommu, info->bus, info->devfn);
+		iommu_detach_dependent_devices(iommu, info->dev);
 
 		/* clear this iommu in iommu_bmp, update iommu count
 		 * and coherency
-- 
1.6.0.4

Re: [PATCH] VT-d: fix PCI device detach from virtual machine

[David Woodhouse]

On Thu, 2009-02-26 at 17:31 +0800, Han, Weidong wrote:
> When assign a device behind conventional PCI bridge or PCIe to
> PCI/PCI-x bridge to a domain, it must assign its bridge and may
> also need to assign secondary interface to the same domain. 
> 
> Dependent assignment is already there, but dependent
> deassignment is missed when detach device from virtual machine.
> This results in conventional PCI device assignment failure after
> it has been assigned once. This patch addes dependent
> deassignment, and fixes the issue. 

Um, this code makes my head hurt.

Why are we doing this in the first place? Because the IOMMU works on the
source-id in PCIe transactions, the pci_find_upstream_pcie_bridge()
function effectively tells us which PCI device our own device will be
masquerading as, for the purposes of DMA.

So why do we bother setting up a context in the IOMMU for the device
itself, when no DMA will ever appear to come from this device? And
likewise why do we bother setting up a context for intermediate PCI
bridges?

Why not just jump straight to the 'DMA proxy' device, and use that
_only_?

We'll have to cope with multiple devices behind the same 'proxy', but it
looks like our handling of that is totally screwed already...  what
happens right now if you have two PCI devices behind the same PCIe-PCI
bridge, and try to attach both of them to different domains... or both
to the _same_ domain, in fact, and then detach just one of them. I think
the answer to the latter question is that your newly-added
iommu_detach_dependent_devices() routine will tear down the mapping on
the 'proxy' device and faults will start happening for the device which
is supposed to still be mapped?

Confused... and tempted to rip it all out and start over.

-- 
dwmw2

Re: [PATCH] VT-d: fix PCI device detach from virtual machine

[Joerg Roedel]

On Mon, Jun 14, 2010 at 07:19:17PM -0400, David Woodhouse wrote:
> Why not just jump straight to the 'DMA proxy' device, and use that
> _only_?

Not sure about Intel chipsets, but on AMD chipset a legacy device can be
seen by the IOMMU with both device-ids, its own and the bridge device.
So the domain needs to be configured for both device-ids in the iommu
driver.

	Joerg

Re: [PATCH] VT-d: fix PCI device detach from virtual machine

[David Woodhouse]

On Tue, 2010-06-15 at 16:10 +0200, Joerg Roedel wrote:
> On Mon, Jun 14, 2010 at 07:19:17PM -0400, David Woodhouse wrote:
> > Why not just jump straight to the 'DMA proxy' device, and use that
> > _only_?
> 
> Not sure about Intel chipsets, but on AMD chipset a legacy device can be
> seen by the IOMMU with both device-ids, its own and the bridge device.
> So the domain needs to be configured for both device-ids in the iommu
> driver.

'can be seen with both' sounds odd to me... you mean that depending on
the phase of the moon, it may show up with one or the other on the
_same_ hardware? Or is your IOMMU sufficiently different that you
actually mean both ids are present at the same time for mapping
purposes?

Or do you just mean that you can't always tell which it'll be, so if
there's a _possibility_ that an upstream bridge will act as a proxy, you
map both of them so that you don't have to worry about false positives
in your "is there a proxy?" algorithm?

That approach may make a lot of sense -- but still I don't see why we'd
set up the mapping for _every_ PCI bridge from the device up to the
suspected proxy... unless we're _really_ unsure about where the
transactions will appear to come from, perhaps?

Have you encountered these Ricoh multi-function devices and observed
them doing DMA transactions from function zero regardless of which
function is actually responsible for it:
03:00.0 SD Host controller: Ricoh Co Ltd Device e822 (rev 03)
03:00.4 FireWire (IEEE 1394): Ricoh Co Ltd Device e832 (rev 03)

Do we need to share a 'quirks' infrastructure for handling such devices?

-- 
dwmw2

Re: [PATCH] VT-d: fix PCI device detach from virtual machine

[Weidong Han]

David Woodhouse wrote:
> On Thu, 2009-02-26 at 17:31 +0800, Han, Weidong wrote:
>   
>> When assign a device behind conventional PCI bridge or PCIe to
>> PCI/PCI-x bridge to a domain, it must assign its bridge and may
>> also need to assign secondary interface to the same domain. 
>>
>> Dependent assignment is already there, but dependent
>> deassignment is missed when detach device from virtual machine.
>> This results in conventional PCI device assignment failure after
>> it has been assigned once. This patch addes dependent
>> deassignment, and fixes the issue. 
>>     
>
> Um, this code makes my head hurt.
>
> Why are we doing this in the first place? Because the IOMMU works on the
> source-id in PCIe transactions, the pci_find_upstream_pcie_bridge()
> function effectively tells us which PCI device our own device will be
> masquerading as, for the purposes of DMA.
>
> So why do we bother setting up a context in the IOMMU for the device
> itself, when no DMA will ever appear to come from this device? And
>   
if the device is behind PCI Express-to-PCI/PCI-X bridge, the source-id 
may be the device bdf or the
source-id provided by the bridge. so it needs to map the device itself.
> likewise why do we bother setting up a context for intermediate PCI
> bridges?
>   
I'm not sure if the intermediate PCI bridges are necessary. need to 
check PCI spec.
> Why not just jump straight to the 'DMA proxy' device, and use that
> _only_?
>   
What's the 'DMA proxy' device? is it the upstream pcie-to-pci bridge?
> We'll have to cope with multiple devices behind the same 'proxy', but it
> looks like our handling of that is totally screwed already...  what
> happens right now if you have two PCI devices behind the same PCIe-PCI
> bridge, and try to attach both of them to different domains... or both
> to the _same_ domain, in fact, and then detach just one of them. I think
> the answer to the latter question is that your newly-added
> iommu_detach_dependent_devices() routine will tear down the mapping on
> the 'proxy' device and faults will start happening for the device which
> is supposed to still be mapped?
>   
all the device behind a pcie-to-pci bridge must be co-assigned to a 
single domain. So it also require users to detach them together.

Regards,
Weidong
> Confused... and tempted to rip it all out and start over.
>
>   

Re: [PATCH] VT-d: fix PCI device detach from virtual machine

[David Woodhouse]

On Thu, 2010-06-17 at 11:35 +0800, Weidong Han wrote:
> David Woodhouse wrote:
> > So why do we bother setting up a context in the IOMMU for the device
> > itself, when no DMA will ever appear to come from this device? And
> >   
> if the device is behind PCI Express-to-PCI/PCI-X bridge, the source-id 
> may be the device bdf or the source-id provided by the bridge. so it
> needs to map the device itself.

Ah, that makes some sense, and matches what Jörg said about the AMD
IOMMU spec. Thanks.

> > likewise why do we bother setting up a context for intermediate PCI
> > bridges?
> >   
> I'm not sure if the intermediate PCI bridges are necessary. need to 
> check PCI spec.

FWIW, the AMD IOMMU doesn't do this; it only sets up the mapping for the
original device and for its 'proxy'.

> > Why not just jump straight to the 'DMA proxy' device, and use that
> > _only_?
> >   
> What's the 'DMA proxy' device? is it the upstream pcie-to-pci bridge?

Yes. Or, in the case of a certain buggy Ricoh multi-function device, it
is function zero -- all other functions do their DMA as if it came from
function zero.

This is why I'm looking at whether we need the whole tree-walking thing,
or whether we can just have a single 'proxy' device (or 'alias' as it's
called in the AMD code).

> all the device behind a pcie-to-pci bridge must be co-assigned to a 
> single domain. So it also require users to detach them together.

Do we even _have_ an API for KVM to assign multiple devices at the same
time? Or an API for KVM to _determine_ which devices are behind the same
'proxy'?

We could even have a new device hotplugged after the assignment has been
done.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] VT-d: fix PCI device detach from virtual machine

[Weidong Han]

David Woodhouse wrote:
> On Thu, 2010-06-17 at 11:35 +0800, Weidong Han wrote:
>   
>> David Woodhouse wrote:
>>     
>>> So why do we bother setting up a context in the IOMMU for the device
>>> itself, when no DMA will ever appear to come from this device? And
>>>   
>>>       
>> if the device is behind PCI Express-to-PCI/PCI-X bridge, the source-id 
>> may be the device bdf or the source-id provided by the bridge. so it
>> needs to map the device itself.
>>     
>
> Ah, that makes some sense, and matches what Jörg said about the AMD
> IOMMU spec. Thanks.
>
>   
>>> likewise why do we bother setting up a context for intermediate PCI
>>> bridges?
>>>   
>>>       
>> I'm not sure if the intermediate PCI bridges are necessary. need to 
>> check PCI spec.
>>     
>
> FWIW, the AMD IOMMU doesn't do this; it only sets up the mapping for the
> original device and for its 'proxy'.
>
>   
>>> Why not just jump straight to the 'DMA proxy' device, and use that
>>> _only_?
>>>   
>>>       
>> What's the 'DMA proxy' device? is it the upstream pcie-to-pci bridge?
>>     
>
> Yes. Or, in the case of a certain buggy Ricoh multi-function device, it
> is function zero -- all other functions do their DMA as if it came from
> function zero.
>
> This is why I'm looking at whether we need the whole tree-walking thing,
> or whether we can just have a single 'proxy' device (or 'alias' as it's
> called in the AMD code).
>
>   
>> all the device behind a pcie-to-pci bridge must be co-assigned to a 
>> single domain. So it also require users to detach them together.
>>     
>
> Do we even _have_ an API for KVM to assign multiple devices at the same
> time? Or an API for KVM to _determine_ which devices are behind the same
> 'proxy'?
>   
No. I think it's better to add some checks in management tool (e.g. 
libvirt) to determine if the devices can be assigned or not before 
device assignment, such as all devices behind same 'proxy' are 
co-assigned (checking command line), and devices can be reset (FLR, 
secondary bus reset, D0->D3 etc.) I think libvirt already has these 
checks for device assignment.


> We could even have a new device hotplugged after the assignment has been
> done.
>   

yes, but for these legacy devices, they only can be assigned and 
de-assigned at the same time.

Regards,
Weidong