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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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.ostrovsky@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel