Re: [PATCH v3 4/7] sysctl: Add sysctl interface for querying PCI topology

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

On 10/02/15 14:45, Boris Ostrovsky wrote:
>
> On 02/10/2015 06:13 AM, Andrew Cooper wrote:
>> On 09/02/15 20:04, Boris Ostrovsky wrote:
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky%oracle.com@localhost>
>>> ---
>>>   xen/common/sysctl.c         |   73
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   xen/include/public/sysctl.h |   29 +++++++++++++++++
>>>   2 files changed, 102 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>> index 30c5806..ea6557f 100644
>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>> @@ -384,7 +384,80 @@ long
>>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>>           xfree(cputopo);
>>>       }
>>>       break;
>>> +#ifdef HAS_PCI
>>> +    case XEN_SYSCTL_pcitopoinfo:
>>> +    {
>>> +        xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
>>> +        physdev_pci_device_t *devs;
>>> +        uint8_t *nodes;
>>> +        unsigned int first_dev, i;
>>> +        int num_devs;
>>> +
>>> +        num_devs = ti->num_devs - ti->first_dev;
>>> +
>>> +        if ( guest_handle_is_null(ti->devs) ||
>>> +             guest_handle_is_null(ti->nodes) ||
>>> +             (num_devs <= 0) )
>>> +        {
>>> +            ret = -EINVAL;
>>> +            break;
>>> +        }
>>> +
>>> +        devs = xmalloc_array(physdev_pci_device_t, num_devs);
>>> +        nodes = xmalloc_array(uint8_t, num_devs);
>> You can do all of this without any memory allocation at all, which will
>> simplify your error handling substantially.
>>
>> Something along the lines of
>>
>> for(...)
>> {
>>      copy one physdev_pci_device_t from the guest
>>
>>      do the lookup
>>
>>      copy one node id back to the guest
>> }
>
> I am trying to avoid doing multiple copies. For lots of devices (IIRC,
> you said you had a system with a few thousand), having two copies per
> loop will add up, I think.

copy_to/from_guest() is not expensive.  It is a straight memcpy with an
extable guards for pagefaults.

~Andrew