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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sat, Nov 02, 2024 at 11:18:24AM -0400, 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@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.

The VF getting released before the PF is what we would usually expect?

I'm a bit confused because a guest being torn down doesn't imply that
the device is removed from Xen (iow: a call to pci_remove_device()).
Removing a device is hot-unplugging the PCI device from Xen, not
deassinging from a guest.

I would also be uneasy with assigning a PF to a non-privileged domain,
specially if VFs from that same device are being assigned to trusted
domains.

My assumption is that you generally want the PFs assigned to the
hardware domain, and the VFs assigned to any other domains (trusted or
not).

Thanks, Roger.