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