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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan