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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2024 17:02, Stewart Hildebrand wrote:
> 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?

Have the PF have a linked list of its VFs, and de-allocate the PF struct
only when that list is empty. (When putting in a VF->PF link, I was
taking it as obvious that then we also want a link the other way around,
i.e. a linked list attached to the PF's struct.) For non-PF devices that
list (if we need to instantiate it in all cases in the first place) will
always be empty.

Jan