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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> @@ -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.

> @@ -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.

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

Jan