Re: [PATCH v6 2/3] xen/pci: introduce PF<->VF links

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 11/1/24 16:16, Stewart Hildebrand wrote:
> +Daniel (XSM mention)
>
> On 10/28/24 13:02, Jan Beulich wrote:
> > On 18.10.2024 22:39, Stewart Hildebrand wrote:
> > > Add links between a VF's struct pci_dev and its associated PF struct
> > > pci_dev. Move the calls to pci_get_pdev()/pci_add_device() down to avoid
> > > dropping and re-acquiring the pcidevs_lock().
> > >
> > > During PF removal, unlink VF from PF and mark the VF broken. As before,
> > > VFs may exist without a corresponding PF, although now only with
> > > pdev->broken = true.
> > >
> > > The hardware domain is expected to remove the associated VFs before
> > > removing the PF. Print a warning in case a PF is removed with associated
> > > VFs still present.
> > >
> > > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> > > ---
> > > Candidate for backport to 4.19 (the next patch depends on this one)
> > >
> > > v5->v6:
> > > * move printk() before ASSERT_UNREACHABLE()
> > > * warn about PF removal with VFs still present
> > Hmm, maybe I didn't make this clear enough when commenting on v5: I wasn't
> > just after an adjustment to the commit message. I'm instead actively
> > concerned of the resulting behavior. Question is whether we can reasonably
> > do something about that.
> >
> > Jan
> Right. My suggestion then is to go back to roughly how it was done in
> v4 [0]:
>
> * Remove the VFs right away during PF removal, so that we don't end up
> with stale VFs. Regarding XSM, assume that a domain with permission to
> remove the PF is also allowed to remove the VFs. We should probably also
> return an error from pci_remove_device in the case of removing the PF
> with VFs still present (and still perform the removals despite returning
> an error). Subsequent attempts by a domain to remove the VFs would
> return an error (as they have already been removed), but that's expected
> since we've taken a stance that PF-then-VF removal order is invalid
> anyway.
I am not confident this is a safe assumption. It will likely be safe for 
probably 99% of the implementations. Apologies for not following 
closely, and correct me if I am wrong here, but from a resource 
perspective each VF can appear to the system as its own unique BDF and 
so I am fairly certain it would be possible to uniquely label each VF. 
For instance in the SVP architecture, the VF may be labeled to restrict 
control to a hardware domain within a Guest Virtual Platform while the 
PF may be restricted to the Supervisor Virtual Platform. In this 
scenario, the Guest would be torn down before the Supervisor so the VF 
should get released before the PF. But it's all theoretical, so I have 
no real implementation to point at that this could be checked/confirmed.
I am only raising this for awareness and not as an objection. If people 
want to punt that theoretical use case down the road until someone 
actually attempts it, I would not be opposed.
v/r,
dps