AMD IOMMU: Hanlde sibling device assignment correctly

PATCH In review Wei Wang2 5 participants 10 messages May 7, 2008 – May 12, 2008

Share & tools

Wei Wang2

author

2008-05-07 09:27 UTC #0 lore raw

PATCH 1/1 AMD IOMMU: Hanlde sibling device assignment correctly

Devices on AMD IOMMU system share a same set of IO page tables if they are indexed by a same device ID in device table. To prevent these sibling devices from being assigned to different domains, a successful device assignment requires all sibling devices are owned by either pciback or the target domain. This is a trial patch with minor modifications in the common interface to handle this situation correctly. VTD code has also been slightly touched please let me know if it breaks any VTD functionality. Thanks!

Wei

Signed-off-by Wei Wang <[email protected]>

Signature · 12 lines

AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

xen/arch/x86/domctl.c +11 -3

diff -r e6f20d5ed5fe xen/arch/x86/domctl.c
@@ -529,15 +529,23 @@ long arch_do_domctl(
     case XEN_DOMCTL_test_assign_device:
     {
         u8 bus, devfn;
+        struct domain *d;
 
         ret = -EINVAL;
         if ( !iommu_enabled )
             break;
+
+        if ( unlikely((d = get_domain_by_id(domctl->domain)) == NULL) )
+        {
+            gdprintk(XENLOG_ERR,
+                "XEN_DOMCTL_assign_device: get_domain_by_id() failed\n");
+            break;
+        }
 
         bus = (domctl->u.assign_device.machine_bdf >> 16) & 0xff;
         devfn = (domctl->u.assign_device.machine_bdf >> 8) & 0xff;
 
-        if ( device_assigned(bus, devfn) )
+        if ( device_assigned(d, bus, devfn) )
         {
             gdprintk(XENLOG_ERR, "XEN_DOMCTL_test_assign_device: "
                      "%x:%x:%x already assigned\n",

@@ -566,7 +574,7 @@ long arch_do_domctl(
         bus = (domctl->u.assign_device.machine_bdf >> 16) & 0xff;
         devfn = (domctl->u.assign_device.machine_bdf >> 8) & 0xff;
 
-        if ( device_assigned(bus, devfn) )
+        if ( device_assigned(d, bus, devfn) )
         {
             gdprintk(XENLOG_ERR, "XEN_DOMCTL_assign_device: "
                      "%x:%x:%x already assigned\n",

@@ -599,7 +607,7 @@ long arch_do_domctl(
         bus = (domctl->u.assign_device.machine_bdf >> 16) & 0xff;
         devfn = (domctl->u.assign_device.machine_bdf >> 8) & 0xff;
 
-        if ( !device_assigned(bus, devfn) )
+        if ( !device_assigned(d, bus, devfn) )
             break;
 
         deassign_device(d, bus, devfn);

xen/arch/x86/setup.c +3 -0

diff -r e6f20d5ed5fe xen/arch/x86/setup.c
@@ -38,6 +38,7 @@
 #include <asm/edd.h>
 #include <xsm/xsm.h>
 #include <asm/tboot.h>
+#include <xen/iommu.h>
 
 #if defined(CONFIG_X86_64)
 #define BOOTSTRAP_DIRECTMAP_END (1UL << 32) /* 4GB */

@@ -1041,6 +1042,8 @@ void __init __start_xen(unsigned long mb
                         cmdline) != 0)
         panic("Could not set up DOM0 guest OS\n");
 
+    iommu_setup_devices(dom0, cmdline);
+
     /* Scrub RAM that is still free and so may go to an unprivileged domain. */
     scrub_heap_pages();

xen/drivers/passthrough/amd/pci_amd_iommu.c +50 -9

84 lines not on this page · raw message

xen/drivers/passthrough/iommu.c +50 -0

54 lines not on this page · raw message

xen/drivers/passthrough/vtd/iommu.c +2 -1

diff -r e6f20d5ed5fe xen/drivers/passthrough/vtd/iommu.c
@@ -1798,7 +1798,7 @@ int intel_vtd_setup(void)
  * If the device isn't owned by dom0, it means it already
  * has been assigned to other domain, or it's not exist.
  */
-int device_assigned(u8 bus, u8 devfn)
+static int intel_iommu_device_assigned(struct domain *d, u8 bus, u8 devfn)
 {
     struct pci_dev *pdev;

@@ -1924,6 +1924,7 @@ struct iommu_ops intel_iommu_ops = {
     .map_page = intel_iommu_map_page,
     .unmap_page = intel_iommu_unmap_page,
     .reassign_device = reassign_device_ownership,
+    .test_assigned = intel_iommu_device_assigned,
 };
 
 /*

xen/include/xen/iommu.h +3 -1

diff -r e6f20d5ed5fe xen/include/xen/iommu.h
@@ -53,9 +53,10 @@ struct iommu {
     struct intel_iommu *intel;
 };
 
+void iommu_setup_devices(struct domain *domain, char *cmdline);
 int iommu_domain_init(struct domain *d);
 void iommu_domain_destroy(struct domain *d);
-int device_assigned(u8 bus, u8 devfn);
+int device_assigned(struct domain*d, u8 bus, u8 devfn);
 int assign_device(struct domain *d, u8 bus, u8 devfn);
 void deassign_device(struct domain *d, u8 bus, u8 devfn);
 void reassign_device_ownership(struct domain *source,

@@ -94,6 +95,7 @@ struct iommu_ops {
     int (*unmap_page)(struct domain *d, unsigned long gfn);
     void (*reassign_device)(struct domain *s, struct domain *t,
                             u8 bus, u8 devfn);
+    int (*test_assigned)(struct domain *d, u8 bus, u8 devfn);
 };
 
 #endif /* _IOMMU_H_ */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Muli Ben-Yehuda

2008-05-07 09:53 UTC #1 lore raw

Quoted 2 lines · On Wed, May 07, 2008 at 11:27:01AM +0200, Wei Wang2 wrote:

Devices on AMD IOMMU system share a same set of IO page tables if they are indexed by a same device ID in device table.

Hi Wei,

Could you please explain in which cases two devices would end up sharing the same device ID? Am I correct in the assumption that this can only happen for devices on the HT fabric, rather than for PCI/PCI-e devices?

Thanks,
Muli

Wei Wang2

author

2008-05-07 10:04 UTC #2 lore raw

Quoted 11 lines · Muli Ben-Yehuda

Quoted 2 lines · On Wed, May 07, 2008 at 11:27:01AM +0200, Wei Wang2 wrote:
Devices on AMD IOMMU system share a same set of IO page tables if they are indexed by a same device ID in device table.
Hi Wei,
Could you please explain in which cases two devices would end up sharing the same device ID? Am I correct in the assumption that this can only happen for devices on the HT fabric, rather than for PCI/PCI-e devices?

I think both could be possible. If IOMMU is implemented per PCI-to-PCI-E bridge, then probably there will be a group of PCI device sharing a same PCI-E requester ID. The device table can only be indexed by either PCI-E requester ID or HT unit ID.

Wei

Quoted 8 lines

Thanks,
Muli
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Muli Ben-Yehuda

2008-05-12 07:55 UTC #3 lore raw

Quoted 4 lines · On Wed, May 07, 2008 at 12:04:51PM +0200, Wei Wang2 wrote:

I think both could be possible. If IOMMU is implemented per PCI-to-PCI-E bridge, then probably there will be a group of PCI device sharing a same PCI-E requester ID. The device table can only be indexed by either PCI-E requester ID or HT unit ID.

Good point. That should be a fairly rare case. It will also apply equally well to VT-d, so this should probably be common functionality.

Cheers,
Muli

Keir Fraser

2008-05-07 09:53 UTC #4 lore raw

In reply to Wei Wang2

Quoted 8 lines · On 7/5/08 10:27, "Wei Wang2" <[email protected]> wrote:

Devices on AMD IOMMU system share a same set of IO page tables if they are indexed by a same device ID in device table. To prevent these sibling devices from being assigned to different domains, a successful device assignment requires all sibling devices are owned by either pciback or the target domain. This is a trial patch with minor modifications in the common interface to handle this situation correctly. VTD code has also been slightly touched please let me know if it breaks any VTD functionality. Thanks!

This patch seems to do more than you suggest, for example adding an extra iommu hook into setup.c for dom0. Also it does get_domain_by_id() with no corresponding put_domain(). And in fact you should be able to rcu_lock_domain_by_id()/rcu_unlock_domain() instead which is faster.

-- Keir

Wei Wang2

author

2008-05-07 10:17 UTC #5 lore raw

Quoted 13 lines · Keir Fraser

Quoted 8 lines · On 7/5/08 10:27, "Wei Wang2" <[email protected]> wrote:
Devices on AMD IOMMU system share a same set of IO page tables if they are indexed by a same device ID in device table. To prevent these sibling devices from being assigned to different domains, a successful device assignment requires all sibling devices are owned by either pciback or the target domain. This is a trial patch with minor modifications in the common interface to handle this situation correctly. VTD code has also been slightly touched please let me know if it breaks any VTD functionality. Thanks!

Quoted 2 lines

This patch seems to do more than you suggest, for example adding an extra iommu hook into setup.c for dom0.

My idea is to let dom0 construct pci device list according to configuration of pciback.hide=(). If a device is not hidden from dom0, it might be in use by dom0, then it could be dangerous to assign any of its siblings to other passthru domain. It is not very clean to hook into setup.c but I failed to find any better way to this :(

Quoted 3 lines

Also it does get_domain_by_id() with no
corresponding put_domain(). And in fact you should be able to rcu_lock_domain_by_id()/rcu_unlock_domain() instead which is faster.

Thanks, I will update that.

Quoted 4 lines

-- Keir

Keir Fraser

2008-05-07 13:09 UTC #6 lore raw

Quoted 7 lines · On 7/5/08 11:17, "Wei Wang2" <[email protected]> wrote:

Quoted 2 lines
This patch seems to do more than you suggest, for example adding an extra iommu hook into setup.c for dom0.
My idea is to let dom0 construct pci device list according to configuration of pciback.hide=(). If a device is not hidden from dom0, it might be in use by dom0, then it could be dangerous to assign any of its siblings to other passthru domain. It is not very clean to hook into setup.c but I failed to find any better way to this :(

I might be confused about how this works. Are you saying that if a domU gets a device passed-thru that is a sibling of a dom0-driven device, then dom0 will mistakenly have its device's DMAs remapped according to the domU mappings that get set up?

Is it possible to do this more dynamically from the tools? I.e, working out sibling groups at the time we build a pass-thru domain, and at that point fail the pass-thru if dom0 or any other domU is driving any device in the group? Seems a nicer level to do the checks and raise errors. You might want to some conflict checking in Xen, of course, just to sanity-check that noone tries to set up two different mappings on the same HT or PCIe identifier.

Basically I think this patch is ugly. :-)

-- Keir

David Edmondson

2008-05-07 13:52 UTC #7 lore raw

Quoted 10 lines · Keir Fraser

Quoted 7 lines · On 7/5/08 11:17, "Wei Wang2" <[email protected]> wrote:
Quoted 2 lines
This patch seems to do more than you suggest, for example adding an extra iommu hook into setup.c for dom0.
My idea is to let dom0 construct pci device list according to configuration of pciback.hide=(). If a device is not hidden from dom0, it might be in use by dom0, then it could be dangerous to assign any of its siblings to other passthru domain. It is not very clean to hook into setup.c but I failed to find any better way to this :(

Quoted 4 lines

I might be confused about how this works. Are you saying that if a domU gets a device passed-thru that is a sibling of a dom0-driven device, then dom0 will mistakenly have its device's DMAs remapped according to the domU mappings that get set up?

If a device is behind a PCI-E to PCI bridge there are cases where transactions from the device are re-written by the bridge to use the requestor id of the bridge. Given that the requestor id is the token used by the IOMMU to determine the domain which initiated the IO, this effectively means that devices behind a PCI-E to PCI bridge are not divisible - they (and the bridge) must all be assigned to the same domain.

At least, that's my understanding :-/

Han, Weidong

2008-05-07 14:03 UTC #8 lore raw

RE: Re: [PATCH] AMD IOMMU: Hanlde sibling deviceassignment correctly

Quoted 25 lines · David Edmondson

Quoted 11 lines · Keir Fraser
Quoted 8 lines · On 7/5/08 11:17, "Wei Wang2" <[email protected]> wrote:
Quoted 2 lines
This patch seems to do more than you suggest, for example adding an extra iommu hook into setup.c for dom0.
My idea is to let dom0 construct pci device list according to configuration of pciback.hide=(). If a device is not hidden from dom0, it might be in use by dom0, then it could be dangerous to assign any of its siblings to other passthru domain. It is not very clean to hook into setup.c but I failed to find any better way to this :(
Quoted 4 lines
I might be confused about how this works. Are you saying that if a domU gets a device passed-thru that is a sibling of a dom0-driven device, then dom0 will mistakenly have its device's DMAs remapped according to the domU mappings that get set up?
If a device is behind a PCI-E to PCI bridge there are cases where transactions from the device are re-written by the bridge to use the requestor id of the bridge. Given that the requestor id is the token used by the IOMMU to determine the domain which initiated the IO, this effectively means that devices behind a PCI-E to PCI bridge are not divisible - they (and the bridge) must all be assigned to the same domain.

Quoted 2 lines

At least, that's my understanding :-/

For VT-d, your understanding is correct. Devices behind PCI-E to PCI/PCI-X bridges can only be collectively assigned to a single domain. But now it's not implemented yet in Xen.

Randy (Weidong)

Wei Wang2

author

2008-05-07 14:12 UTC #9 lore raw

In reply to Keir Fraser

Quoted 10 lines · Keir Fraser

Quoted 7 lines · On 7/5/08 11:17, "Wei Wang2" <[email protected]> wrote:
Quoted 2 lines
This patch seems to do more than you suggest, for example adding an extra iommu hook into setup.c for dom0.
My idea is to let dom0 construct pci device list according to configuration of pciback.hide=(). If a device is not hidden from dom0, it might be in use by dom0, then it could be dangerous to assign any of its siblings to other passthru domain. It is not very clean to hook into setup.c but I failed to find any better way to this :(

Quoted 4 lines

I might be confused about how this works. Are you saying that if a domU gets a device passed-thru that is a sibling of a dom0-driven device, then dom0 will mistakenly have its device's DMAs remapped according to the domU mappings that get set up?

I have encountered a problem on my testing machine. When I assign a usb controller ,which shares the same device ID of a sata controller belong to dom0, dom0 hangs immediately. The current solution is that when hiding a pci device from dom0, its sibling device must also be hidden. Therefore, I need to parse pciback.hide from Xen side..

Quoted 6 lines

Is it possible to do this more dynamically from the tools? I.e, working out sibling groups at the time we build a pass-thru domain, and at that point fail the pass-thru if dom0 or any other domU is driving any device in the group? Seems a nicer level to do the checks and raise errors. You might want to some conflict checking in Xen, of course, just to sanity-check that noone tries to set up two different mappings on the same HT or PCIe identifier.

Yes, that is my first thought. The question is only dom0 knows about pciback.hide information. Current iommu code construct device list no matter whether devices are actually hidden from dom0. Is it better to just allow hidden devices to be added into the device list?

Quoted 1 line

Basically I think this patch is ugly. :-)

Yes, I admit that. the purpose of this patch is maybe just raise a problem for further discussion. ;)

Wei

Quoted 4 lines

-- Keir