From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 1/4] pci: Do not ignore device's PXM information Date: Wed, 03 Dec 2014 10:19:27 -0500 Message-ID: <547F29FF.5040300@oracle.com> References: <1417556050-23364-1-git-send-email-boris.ostrovsky@oracle.com> <1417556050-23364-2-git-send-email-boris.ostrovsky@oracle.com> <547F25CC.80200@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <547F25CC.80200@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , jbeulich@suse.com, keir@xen.org, ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, ian.campbell@citrix.com, wei.liu2@citrix.com Cc: dario.faggioli@citrix.com, ufimtseva@gmail.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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