Re: [PATCH v4 5/9] sysctl: Add sysctl interface for querying PCI topology

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 10/03/15 02:27, Boris Ostrovsky wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>
> Changes in v4:
> * No buffer allocation in sysctl, copy data device-by-device
> * Replace INVALID_TOPOLOGY_ID with XEN_INVALID_NODE_ID
>
>  xen/common/sysctl.c         |   59 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/sysctl.h |   29 +++++++++++++++++++++
>  2 files changed, 88 insertions(+), 0 deletions(-)
>
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 2d11a76..9cd6321 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -387,7 +387,66 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>          }
>      }
>      break;

Blank line here.

> +#ifdef HAS_PCI
> +    case XEN_SYSCTL_pcitopoinfo:

Please try and keep the SYSCTL implementations in numerical order, which
should put this new block below the pcr and coverage blocks.

> +    {
> +        xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
> +
> +        if ( guest_handle_is_null(ti->devs) ||
> +             guest_handle_is_null(ti->nodes) ||
> +             (ti->first_dev > ti->num_devs) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        for ( ; ti->first_dev < ti->num_devs; ti->first_dev++ )
> +        {
> +            physdev_pci_device_t dev;
> +            uint8_t node;
> +            struct pci_dev *pdev;
> +
> +            if ( copy_from_guest_offset(&dev, ti->devs, ti->first_dev, 1) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            spin_lock(&pcidevs_lock);
> +            pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
> +            if ( !pdev || (pdev->node == NUMA_NO_NODE) )
> +                node = XEN_INVALID_NODE_ID;
> +            else
> +                node = pdev->node;
> +            spin_unlock(&pcidevs_lock);
> +
> +            if ( copy_to_guest_offset(ti->nodes, ti->first_dev, &node, 1) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
>  
> +            if ( hypercall_preempt_check() )
> +                break;
> +        }
> +
> +        if ( !ret )
> +        {
> +            ti->first_dev++;
> +
> +            if ( __copy_field_to_guest(u_sysctl, op, u.pcitopoinfo.first_dev) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            if ( ti->first_dev < ti->num_devs )
> +                ret = hypercall_create_continuation(__HYPERVISOR_sysctl,
> +                                                    "h", u_sysctl);
> +        }
> +    }
> +    break;
> +#endif

And here.


>  #ifdef TEST_COVERAGE
>      case XEN_SYSCTL_coverage_op:
>          ret = sysctl_coverage_op(&op->u.coverage_op);
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index c544c76..a224951 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -33,6 +33,7 @@
>  
>  #include "xen.h"
>  #include "domctl.h"
> +#include "physdev.h"
>  
>  #define XEN_SYSCTL_INTERFACE_VERSION 0x0000000B
>  
> @@ -494,6 +495,32 @@ struct xen_sysctl_cputopoinfo {
>  typedef struct xen_sysctl_cputopoinfo xen_sysctl_cputopoinfo_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cputopoinfo_t);
>  
> +/* XEN_SYSCTL_pcitopoinfo */
> +struct xen_sysctl_pcitopoinfo {
> +    /* IN: Size of pcitopo array */

Number of elements (as the two arrays are different sized structures) of
both the devs and nodes array.  It is not sensible for a caller to ever
pass arrays of different lengths.

> +    uint32_t num_devs;
> +
> +    /*
> +     * IN/OUT: First element of pcitopo array that needs to be processed by
> +     * hypervisor.
> +     * This is used primarily by hypercall continuations and callers will
> +     * typically set it to zero.

I would word this more strongly and state that callers must set this to
0 and that it is an internal implementation detail of Xen.

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +     */
> +    uint32_t first_dev;
> +
> +    /* IN: list of devices */
> +    XEN_GUEST_HANDLE_64(physdev_pci_device_t) devs;
> +
> +    /*
> +     * OUT: node identifier for each device.
> +     * If information for a particular device is not avalable then set
> +     * to XEN_INVALID_NODE_ID.
> +     */
> +    XEN_GUEST_HANDLE_64(uint8) nodes;
> +};
> +typedef struct xen_sysctl_pcitopoinfo xen_sysctl_pcitopoinfo_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
> +
>  /* XEN_SYSCTL_numainfo */
>  #define XEN_INVALID_MEM_SZ     (~0U)
>  #define XEN_INVALID_NODE_DIST  ((uint8_t)~0)
> @@ -694,12 +721,14 @@ struct xen_sysctl {
>  #define XEN_SYSCTL_scheduler_op                  19
>  #define XEN_SYSCTL_coverage_op                   20
>  #define XEN_SYSCTL_psr_cmt_op                    21
> +#define XEN_SYSCTL_pcitopoinfo                   22
>      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>      union {
>          struct xen_sysctl_readconsole       readconsole;
>          struct xen_sysctl_tbuf_op           tbuf_op;
>          struct xen_sysctl_physinfo          physinfo;
>          struct xen_sysctl_cputopoinfo       cputopoinfo;
> +        struct xen_sysctl_pcitopoinfo       pcitopoinfo;
>          struct xen_sysctl_numainfo          numainfo;
>          struct xen_sysctl_sched_id          sched_id;
>          struct xen_sysctl_perfc_op          perfc_op;