Re: [PATCH v3 2/4] xen/pci: convert pci_find_*cap* to pci_sbdf_t

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 8/22/23 08:53, Jan Beulich wrote:
> On 22.08.2023 03:29, Stewart Hildebrand wrote:
>> @@ -291,12 +291,9 @@ static void msi_set_enable(struct pci_dev *dev, int 
>> enable)
>>  static void msix_set_enable(struct pci_dev *dev, int enable)
>>  {
>>      int pos;
>> -    u16 control, seg = dev->seg;
>> -    u8 bus = dev->bus;
>> -    u8 slot = PCI_SLOT(dev->devfn);
>> -    u8 func = PCI_FUNC(dev->devfn);
>> +    u16 control;
> 
> As you touch such places, it would be nice to also switch to uint16_t at
> the same time. (Similarly when you touch "pos" declarations anyway,
> converting them to unsigned int when they're wrongly plain int would also
> be nice.)

OK. For clarity's sake (and for my own sake), I'll bound the scope of these 
changes to lines/statements/declarations that are being modified anyway as a 
result of the switch to pci_sbdf_t. Also, with the s/int/unsigned int/ changes, 
I will remove "No functional change" from the commit description (not sure it 
should have been there in the first place).

>> @@ -318,17 +315,12 @@ static bool msi_set_mask_bit(struct irq_desc *desc, 
>> bool host, bool guest)
>>  {
>>      struct msi_desc *entry = desc->msi_desc;
>>      struct pci_dev *pdev;
>> -    u16 seg, control;
>> -    u8 bus, slot, func;
>> +    u16 control;
>>      bool flag = host || guest, maskall;
>>
>>      ASSERT(spin_is_locked(&desc->lock));
>>      BUG_ON(!entry || !entry->dev);
>>      pdev = entry->dev;
>> -    seg = pdev->seg;
>> -    bus = pdev->bus;
>> -    slot = PCI_SLOT(pdev->devfn);
>> -    func = PCI_FUNC(pdev->devfn);
>>      switch ( entry->msi_attrib.type )
>>      {
>>      case PCI_CAP_ID_MSI:
> 
> You don't further alter the function, so this looks like an unrelated
> (but still desirable for at least Misra) change.

Right. These instances of -Wunused-but-set-variable warnings were the result of 
a previous pci_sbdf_t conversion. I'll split it into a separate patch, perhaps 
with a fixes tag:
Fixes: 0c38c61aad21 ("pci: switch pci_conf_write32 to use pci_sbdf_t")

>> @@ -685,8 +674,8 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 
>> func, u8 bir, int vf)
>>      {
>>          struct pci_dev *pdev = pci_get_pdev(NULL,
>>                                              PCI_SBDF(seg, bus, slot, func));
> 
> With this, ...
> 
>> -        unsigned int pos = pci_find_ext_capability(seg, bus,
>> -                                                   PCI_DEVFN(slot, func),
>> +        unsigned int pos = pci_find_ext_capability(PCI_SBDF(seg, bus, slot,
>> +                                                            func),
>>                                                     PCI_EXT_CAP_ID_SRIOV);
> 
> ... please use pdev->sbdf here. Albeit I guess some further re-arrangement
> is wanted here, to eliminate all of those PCI_SBDF() (and then also dealing
> with the otherwise necessary NULL check). IOW perhaps fine the way you have
> it, and then to be subject to a follow-on change.

OK. I'll keep it as-is in this patch, then create a follow-on patch that: uses 
pdev->sbdf instead of PCI_SBDF inside this code block, and moves the NULL check 
earlier.

>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -193,11 +193,10 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
>>                     unsigned int devfn, int reg, int len, u32 *value);
>>  int pci_mmcfg_write(unsigned int seg, unsigned int bus,
>>                      unsigned int devfn, int reg, int len, u32 value);
>> -int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, u8 cap);
>> -int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap);
>> -int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
>> -int pci_find_next_ext_capability(int seg, int bus, int devfn, int start,
>> -                                 int cap);
>> +int pci_find_cap_offset(pci_sbdf_t sbdf, u8 cap);
>> +int pci_find_next_cap(pci_sbdf_t sbdf, u8 pos, int cap);
>> +int pci_find_ext_capability(pci_sbdf_t sbdf, int cap);
>> +int pci_find_next_ext_capability(pci_sbdf_t sbdf, int start, int cap);
> 
> The remaining types want converting, too: Neither fixed-width nor plain int
> are appropriate here. (It's a few too many type adjustments to make, for me
> to offer doing so while committing.)

Understood, I'm happy to make the change for v4. Is the following what you'd 
expect it to look like?

unsigned int pci_find_cap_offset(pci_sbdf_t sbdf, unsigned int cap);
unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
                               unsigned int cap);
unsigned int pci_find_ext_capability(pci_sbdf_t sbdf, unsigned int cap);
unsigned int pci_find_next_ext_capability(pci_sbdf_t sbdf, unsigned int start,
                                          unsigned int cap);