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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.01.15 at 15:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 01/07/2015 04:21 AM, Jan Beulich wrote:
>>>>> On 06.01.15 at 03:18, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> +        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?
> 
> 
> This is the same information (pxm -> node mapping ) that we provide in 
> XEN_SYSCTL_topologyinfo (renamed in this series to
> XEN_SYSCTL_cputopoinfo). Given that I expect the two topologies to be 
> used together I think the answer is yes.

Building your argumentation on potentially mis-designed existing
interfaces is bogus. The question is - what use is a Xen internal
node number to a caller of a particular hypercall (other than it
being purely informational, e.g. for printing human readable
output)?

In particular if we were to introduce a new non-sysctl interface,
determining whether the hypervisor internal representation is really
the right one to expose here should be one of the most important
design aspects. I personally think that exposing e.g. the firmware
determined (and hence hopefully stable across reboots) PXM would
be more reasonable.

>>> @@ -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.
> 
> I mentioned it to Wei yesterday that the file is structured (or at least 
> appears to me to be structured) in such a way that these top comments 
> mark sections of definitions for each sysctl. And so I thought that I'd 
> be breaking this convention if I were to extend the comment.

Yeah, I saw that discussion after having replied. Looking more
closely, I think the placement of the INVALID_TOPOLOGY_ID
definition is sub-optimal when you want to use it in a second place.
Move it mode towards the beginning of the header, leave the
current comment as is, and if you feel so ad a new comment ahead
of it explaining which operations are using it.

And then, if we go the non-sysctl route, the definition would likely
need moving into xen.h anyway.

Jan


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