Re: [PATCH v3 02/18] pci: Use pci_sbdf_t in pci_add_device()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.06.2026 14:19, Teddy Astie wrote:
> Le 30/06/2026 à 13:44, Jan Beulich a écrit :
>> On 29.06.2026 19:21, Teddy Astie wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -662,12 +662,11 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, 
>>> unsigned int pos,
>>>       return is64bits ? 2 : 1;
>>>   }
>>>   
>>> -int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>> -                   const struct pci_dev_info *info, nodeid_t node)
>>> +int pci_add_device(pci_sbdf_t sbdf, const struct pci_dev_info *info, 
>>> nodeid_t node)
>>
>> Nit: Overlong line (like already pointed out for v2).
>>
>>>   {
>>>       struct pci_seg *pseg;
>>>       struct pci_dev *pdev;
>>> -    unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
>>> +    unsigned int slot = sbdf.devfn, func = sbdf.fn;
>>
>> Andrew pointed out an issue here for v2, and you addressed only half of his
>> comment.
> 
> Ah, I see, I used devfn instead of device.
> I guess it would be better to avoid slot vs dev wording as it's easy to 
> get wrong.
> In this case, I think it would be even better to drop this variable as 
> it's barely used (only used at the end for display as a alias for sbdf.dev).

Indeed (and I think another comment of mine was precisely about that one use).

>>> @@ -729,14 +727,14 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>               if ( !pdev->ext_cfg )
>>>                   printk(XENLOG_WARNING
>>>                          "%pp: VF without extended config space?\n",
>>> -                       &pdev->sbdf);
>>> +                       &sbdf);
>>>           }
>>>       }
>>>   
>>>       if ( !pdev->info.is_virtfn && !pdev->physfn.vf_rlen[0] )
>>>       {
>>>           unsigned int pos = pci_find_ext_capability(pdev, 
>>> PCI_EXT_CAP_ID_SRIOV);
>>> -        uint16_t ctrl = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_CTRL);
>>> +        uint16_t ctrl = pci_conf_read16(sbdf, pos + PCI_SRIOV_CTRL);
>>
>> Quoting from my v2 reply:
>>
>> "Are changes like these two actually worthwhile to make? sbdf, being a 
>> function
>>   parameter, can be modified in the course of the function. pdev->sbdf, otoh,
>>   cannot (for being in a const struct field). If further sbdf, throughout the
>>   function, never had its address taken, the compiler may be able to produce
>>   better code."
>>
> 
> I missed it; though I'm not convinced that would work. While the sbdf 
> field of struct pci_dev is const, the pointer to it isn't, so in the 
> same regard, the compiler can assume that the pdev pointer changes thus 
> can't rely on the const property of the pci_dev::sbdf field.

If the pointer is a local variable of the function which never has its
address taken, the compiler can see all assignments. Hence it can deduce
whether (and when) the pointer potentially changes.

> A better alternative would be to mark the sbdf function param const.

Not really, we don't normally use const this way.

Jan