Re: [PATCH v2] x86/msi: fix locking for SR-IOV devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2024 07:20, Stewart Hildebrand wrote:
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -662,7 +662,8 @@ static int msi_capability_init(struct pci_dev *dev,
>      return 0;
>  }
>  
> -static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 func, u8 bir, int 
> vf)
> +static u64 read_pci_mem_bar(struct pci_dev *pdev, u16 seg, u8 bus, u8 slot,
> +                            u8 func, u8 bir, int vf)
>  {

First I thought this was a leftover from the earlier version. But you need
it for accessing the vf_rlen[] field. Yet that's properly misleading,
especially when considering that the fix also wants backporting. What pdev
represents here changes. I think you want to pass in just vf_rlen (if we
really want to go this route; I'm a little wary of this repurposing of the
field, albeit I see no real technical issue).

Of course there's a BUILD_BUG_ON() which we need to get creative with, in
order to now outright drop it (see also below).

> @@ -670,19 +671,15 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, 
> u8 func, u8 bir, int vf)
>  
>      if ( vf >= 0 )
>      {
> -        struct pci_dev *pdev = pci_get_pdev(NULL,
> -                                            PCI_SBDF(seg, bus, slot, func));
> +        pci_sbdf_t pf_sbdf = PCI_SBDF(seg, bus, slot, func);

I think this wants naming just "sbdf" and moving to function scope. There
are more places in the function which, in a subsequent change, could also
benefit from this new local variable.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -654,6 +654,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      const char *type;
>      int ret;
>      bool pf_is_extfn = false;
> +    uint64_t vf_rlen[6] = { 0 };

The type of this variable needs to be tied to that of the struct field
you copy to/from. Otherwise, if the struct field changes type ...

> @@ -664,7 +665,10 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>                              PCI_SBDF(seg, info->physfn.bus,
>                                       info->physfn.devfn));
>          if ( pdev )
> +        {
>              pf_is_extfn = pdev->info.is_extfn;
> +            memcpy(vf_rlen, pdev->vf_rlen, sizeof(pdev->vf_rlen));

... there'll be nothing for the compiler to tell us. Taken together with
the BUILD_BUG_ON() related remark further up, I think you want to
introduce a typedef and/or struct here to make things properly typesafe
(as then you can avoid the use of memcpy()).

Seeing the conditional we're in, what if we take ...

> +        }
>          pcidevs_unlock();
>          if ( !pdev )
>              pci_add_device(seg, info->physfn.bus, info->physfn.devfn,

... this fallback path?

> @@ -700,7 +704,10 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>           * extended function.
>           */
>          if ( pdev->info.is_virtfn )
> +        {
>              pdev->info.is_extfn = pf_is_extfn;
> +            memcpy(pdev->vf_rlen, vf_rlen, sizeof(pdev->vf_rlen));
> +        }
>      }

Similarly here - what if the enclosing if()'s condition is false? Even
if these cases couldn't be properly taken care of, they'd at least need
discussing in the description. In this context note how in a subsequent
invocation of pci_add_device() for the PF the missing data in vf_rlen[]
would actually be populated into the placeholder struct that the
fallback invocation of pci_add_device() would have created. Yet the
previously created VF's struct wouldn't be updated (afaict). This was,
iirc, the main reason to always consult the PF's ->vf_rlen[].

An alternative approach might be to add a link from VF to PF, while
making sure that the PF struct won't be de-allocated until all its VFs
have gone away. That would then also allow to eliminate the problematic
pci_get_pdev().

Jan