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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Nov 04, 2024 at 11:45:05AM +0000, Alejandro Vallejo wrote:
> On Sat Nov 2, 2024 at 3:18 PM GMT, Daniel P. Smith wrote:
> > 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@amd.com>
> > >>> ---
> > >>> 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.
> 
> Wouldn't it stand to reason then to act conditionally on the authority of the
> caller?
> 
> i.e: If the caller has the (XSM-checked) authority to remove _BOTH_ PF and
> VFs, remove all. If it doesn't have authority to remove the VFs then early exit
> with an error, leaving the PF behind as well.

I'm unsure if it makes sense to have an entity that's allowed to issue
a pci_remove_device() against a PF, but not against the VFs of the
device.

The owner of the PF should be capable of disabling SR-IOV, at which
point all the VFs disappear from the PCI config space.  If such entity
is capable of controlling the SR-IOV capability, it should also be
able to issue pci_remove_device() calls against the VFs.

Thanks, Roger.