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);
