Re: [PATCH v2 1/4] pci: Do not ignore device's PXM information

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

On 07/01/15 14:42, Boris Ostrovsky wrote:
> On 01/07/2015 04:06 AM, Jan Beulich wrote:
>>>>> On 06.01.15 at 03:18, <boris.ostrovsky@oracle.com> wrote:
>>> @@ -618,7 +620,22 @@ ret_t do_physdev_op(int cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>           }
>>>           else
>>>               pdev_info.is_virtfn = 0;
>>> -        ret = pci_add_device(add.seg, add.bus, add.devfn, &pdev_info);
>>> +
>>> +        if ( add.flags & XEN_PCI_DEV_PXM )
>>> +        {
>>> +            uint32_t pxm;
>>> +            int optarr_off = offsetof(struct
>>> physdev_pci_device_add, optarr) /
>> unsigned int or size_t.
>>
>>> --- a/xen/include/xen/pci.h
>>> +++ b/xen/include/xen/pci.h
>>> @@ -56,6 +56,8 @@ struct pci_dev {
>>>         u8 phantom_stride;
>>>   +    int node; /* NUMA node */
>> I thought I asked about this on v1 already: Does this really need to be
>> an int, when commonly node numbers are stored in u8/unsigned char?
>> Shrinking the field size would prevent the structure size from
>> growing...
>
>
> I kept this field as an int to be able to store NUMA_NO_NODE which I
> thought to be (int)-1.
>
> But now I see that NUMA_NO_NODE is, in fact, 0xff but is promoted to
> (int)-1 by pxm_to_node(). Given that there is a number of tests for
> NUMA_NO_NODE and not for (int)-1, should we then make pxm_to_node()
> return u8 as well?

I noticed this as well, and found it quite counter intuitive.

I would suggest fixing NUMA_NO_NODE to -1 and removing some of the
type-punning.

~Andrew