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
