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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 8/9/24 09:05, Jan Beulich wrote:
> On 09.08.2024 06:09, Stewart Hildebrand wrote:
>> 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()...
> 
> "Redundant" can be positive or negative. Your response sounds as if you
> thought one could be dropped, yet I think we want them in both places.
> 
>>>> --- 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;
>>     ...
>> };
> 
> I don't (yet) see the need for refcnt, and I also don't see why it wouldn't
> continue to be embedded in struct pci_dev. Specifically ...
> 
>>> 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).
> 
> ... I think having a link from VF to its PF may turn out helpful in the
> future for other purposes, too.

Continue to embed in struct pci_dev: okay.

Link from VF to PF: assuming you mean a link to the PF's
struct pci_dev *, okay.

Ensuring the PF's struct pci_dev * won't be de-allocated until the VFs
are gone: I don't think we want to impose any sort of ordering on
whether the toolstack removes VFs or PFs first. So, if not reference
counting (i.e. how many VFs are referring back to the PF), how else
would we make sure that the PF struct won't be de-allocated until all
its VFs have gone away?