Re: [Xen-devel] [PATCH v7 2/5] sysctl: Add sysctl interface for querying PCI topology

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.04.15 at 18:59, <boris.ostrovsky@xxxxxxxxxx> wrote:
> Changes in v7:
> * Break from the loop when -ENODEV is encountered

This seems pretty inefficient for the caller. Returning a "bad"
identifier other than XEN_INVALID_NODE_ID would seem better
to me.

> --- a/docs/misc/xsm-flask.txt
> +++ b/docs/misc/xsm-flask.txt
> @@ -121,6 +121,7 @@ __HYPERVISOR_sysctl (xen/include/public/sysctl.h)
>   * XEN_SYSCTL_cpupool_op
>   * XEN_SYSCTL_scheduler_op
>   * XEN_SYSCTL_coverage_op
> + * XEN_SYSCTL_pcitopoinfo

No additions to this list are permitted. Either the new sub-op is
disaggregation safe (which it looks to be), or it can't be accepted.

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -411,6 +411,65 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>          break;
>  #endif
>  
> +#ifdef HAS_PCI
> +    case XEN_SYSCTL_pcitopoinfo:
> +    {
> +        xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
> +        unsigned dev_cnt = 0;
> +
> +        if ( guest_handle_is_null(ti->devs) ||
> +             guest_handle_is_null(ti->nodes) ||
> +             (ti->first_dev > ti->num_devs) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        while ( ti->first_dev < ti->num_devs )
> +        {
> +            physdev_pci_device_t dev;
> +            uint32_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 )
> +            {
> +                ret = -ENODEV;
> +                node = XEN_INVALID_NODE_ID;
> +            }
> +            else if ( 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;
> +            }
> +
> +            ti->first_dev++;
> +
> +            if ( (ret == -ENODEV) ||

If despite the earlier comment you want to stay with this model, I think
you should keep first_dev pointing at the bad slot (but also see below),
leaving it to the caller to increment past it (consider this being function
0 and the next slots being the other functions - in that case it's clear
that the caller would want to skip more than just this one slot).

> +                 ((++dev_cnt > 0x3f) && hypercall_preempt_check()) )
> +                break;
> +        }
> +
> +        if ( (!ret || (ret == -ENODEV)) &&
> +             __copy_field_to_guest(u_sysctl, op, u.pcitopoinfo.first_dev) )
> +            ret = -EFAULT;
> +    }
> +    break;
> +#endif

With the continuation-less model now used I don't think it makes
sense to have first_dev and num_devs - for re-invocation all the
caller needs to do is increment the buffer pointer suitably. I.e.
you can get away with just a count of devices afaict.

Jan


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