Re: [Xen-devel] [PATCH v2 3/4] sysctl: Add sysctl interface for querying PCI topology

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.01.15 at 03:18, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -365,6 +365,66 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>      }
>      break;
>  

#ifdef HAS_PCI

> +    case XEN_SYSCTL_pcitopoinfo:
> +    {
> +        xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
> +
> +        if ( guest_handle_is_null(ti->pcitopo) ||
> +             (ti->first_dev >= ti->num_devs) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        for ( ; ti->first_dev < ti->num_devs; ti->first_dev++ )
> +        {
> +            xen_sysctl_pcitopo_t pcitopo;
> +            struct pci_dev *pdev;
> +
> +            if ( copy_from_guest_offset(&pcitopo, ti->pcitopo,
> +                                        ti->first_dev, 1) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            spin_lock(&pcidevs_lock);
> +            pdev = pci_get_pdev(pcitopo.pcidev.seg, pcitopo.pcidev.bus,
> +                                pcitopo.pcidev.devfn);
> +            if ( !pdev || (pdev->node == NUMA_NO_NODE) )
> +                pcitopo.node = INVALID_TOPOLOGY_ID;
> +            else
> +                pcitopo.node = pdev->node;

Are hypervisor-internal node numbers really meaningful to the caller?

> +            spin_unlock(&pcidevs_lock);
> +
> +            if ( copy_to_guest_offset(ti->pcitopo, ti->first_dev,

__copy_ty_guest_offset()

> +                                      &pcitopo, 1) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            if ( hypercall_preempt_check() )
> +                break;

You didn't increment ->first_dev yet, i.e. you'd start with the same
index again when continuing later, and in the end you may not make
any forward progress.

> @@ -463,7 +464,7 @@ typedef struct xen_sysctl_lockprof_op 
> xen_sysctl_lockprof_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_lockprof_op_t);
>  
>  /* XEN_SYSCTL_cputopoinfo */
> -#define INVALID_TOPOLOGY_ID  (~0U)
> +#define INVALID_TOPOLOGY_ID  (~0U) /* Also used by pcitopo */

Better extend the preceding comment.

> @@ -492,6 +493,36 @@ 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_pcitopo {
> +    struct physdev_pci_device pcidev;
> +    uint32_t node;
> +};
> +typedef struct xen_sysctl_pcitopo xen_sysctl_pcitopo_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopo_t);
> +
> +struct xen_sysctl_pcitopoinfo {
> +    /* IN: Size of pcitopo array */
> +    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
> +     */
> +    uint32_t first_dev;
> +
> +    /*
> +     * If not NULL, filled with node identifier for each pcidev

The "If not NULL" would be meaningful only if NULL had a special
meaning.

Jan


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