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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

Hi Jan,

Thanks for the feedback.

On 8/7/24 11:21, Jan Beulich wrote:
> 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).

I like your idea below of using a struct, so I'll pass a pointer to the
new struct.

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

I suppose this BUILD_BUG_ON() is redundant with the one in
pci_add_device()...

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

Will do.

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

Here's what I'm thinking:

struct vf_info {
    uint64_t vf_rlen[PCI_SRIOV_NUM_BARS];
    unsigned int refcnt;
};

struct pci_dev {
    ...
    struct vf_info *vf_info;
    ...
};

> 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?

It seems we need another call to pci_get_pdev() here to obtain a
reference to the newly allocated vf_info from the PF's pdev.

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

Right. If info is NULL, either it's a PF in the fallback case, or the
toolstack invoked PHYSDEVOP_manage_pci_add, in which case we treat it as
a PF or non-SR-IOV device. Using PHYSDEVOP_manage_pci_add for a VF is
not a case we handle. We only know if it's a VF if the toolstack has
told us so.

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

I think I can add a link to a new reference-counted struct with just the
info needed (see the proposed struct above).