Re: [Xen-devel] [PATCH 1/4] pci: Do not ignore device's PXM information

[Boris Ostrovsky]

On 12/03/2014 10:01 AM, Andrew Cooper wrote:
On 02/12/14 21:34, Boris Ostrovsky wrote:
If ACPI provides PXM data for IO devices then dom0 will pass it to
hypervisor during PHYSDEVOP_pci_device_add call. This information,
however, is currently ignored.
We should remember it (in the form of nodeID). We will also print it
when user requests device information dump.

Signed-off-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
---
  xen/arch/x86/physdev.c        | 20 +++++++++++++++++---
  xen/drivers/passthrough/pci.c | 13 +++++++++----
  xen/include/xen/pci.h         |  5 ++++-
  3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 6b3201b..7775f80 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -565,7 +565,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
arg)
          if ( copy_from_guest(&manage_pci, arg, 1) != 0 )
              break;
  
-        ret = pci_add_device(0, manage_pci.bus, manage_pci.devfn, NULL);
+        ret = pci_add_device(0, manage_pci.bus, manage_pci.devfn,
+                             NULL, NUMA_NO_NODE);
          break;
      }
  
@@ -597,13 +598,14 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
          pdev_info.physfn.devfn = manage_pci_ext.physfn.devfn;
          ret = pci_add_device(0, manage_pci_ext.bus,
                               manage_pci_ext.devfn,
-                             &pdev_info);
+                             &pdev_info, NUMA_NO_NODE);
          break;
      }
  
      case PHYSDEVOP_pci_device_add: {
          struct physdev_pci_device_add add;
          struct pci_dev_info pdev_info;
+        int node;
  
          ret = -EFAULT;
          if ( copy_from_guest(&add, arg, 1) != 0 )
@@ -618,7 +620,19 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
arg)
          }
          else
              pdev_info.is_virtfn = 0;
-        ret = pci_add_device(add.seg, add.bus, add.devfn, &pdev_info);
+
+        if ( add.flags & XEN_PCI_DEV_PXM ) {
+            int optarr_off = offsetof(struct physdev_pci_device_add, optarr) /
+                             sizeof(add.optarr[0]);
+
+            if ( copy_from_guest_offset(&add.optarr[0], arg, optarr_off, 1) )
+                break;
This will clobber the hypervisor stack, attempting to put the PXM
information into what is probably pdev_info.

Sigh... I actually fixed this on Linux side and now introduced this same 
bug here. (I guess I was lucky here that compiler must have inserted a 
word between add and pdev_info for alignment).


How is one expected to use XEN_PCI_DEV_PXM ? There is currently no
specification of how to use the variable length structure.

I don't think this is explicitly specified anywhere but if this flag is 
set then optarr[0] is supposed to store uint32_t pxm. I will add a 
comment to this effect to physdev.h

Thanks.
-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel