Re: [PATCH 1/5] pci: use pci_sbdf_t in pci_dev

["Jan Beulich" <JBeulich@suse.com>]

>>> On 10.05.19 at 18:10, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -688,8 +688,8 @@ static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data,
>          {
>              gdprintk(XENLOG_ERR,
>                       "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n",
> -                     pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> -                     PCI_FUNC(pdev->devfn), pirq + i, rc);
> +                     pdev->sbdf.seg, pdev->sbdf.bus, pdev->sbdf.dev,
> +                     pdev->sbdf.func, pirq + i, rc);

I assume patch 4 could have been quite a bit smaller, and you could have
avoided touching the same places twice if that one came before the one
here.

> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -289,23 +289,23 @@ void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev,
>      if ( !ats_enabled )
>          return;
>  
> -    if ( !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
> +    if ( !pci_ats_enabled(pdev->sbdf.seg, pdev->sbdf.bus, pdev->sbdf.extfunc) )

Why extfunc and not (as it was before) devfn (same elsewhere)?
There should have been a devfn field from the beginning, even if
it's similarly uint8_t as extfunc is. As the meaning of both is different,
the correct (given context) one should be used. Existing uses of
extfunc should also be inspected and changed if necessary.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -80,9 +80,8 @@ struct pci_dev {
>      struct arch_msix *msix;
>  
>      struct domain *domain;
> -    const u16 seg;
> -    const u8 bus;
> -    const u8 devfn;
> +
> +    const pci_sbdf_t sbdf;

To help the transition, did you consider first making this a union of
the existing fields and the new one, next replacing used in a per
component manner (so that individual maintainers would have to
look at smaller patches each only), and finally dropping the union
and its old fields?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel