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

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

On 11/5/24 04:03, Roger Pau Monné wrote:
> 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 was providing a use-case that was crafted, just not implemented. I 
have not looked at SRIOV in some time and not at the level necessary to 
determine this, but I would be uneasy thinking a VF could just be 
released from a guest to be reassigned either to the host hardware 
domain, aka Supervisor VP, or another guest. Perhaps a reset of the VF 
is enough or it might need more, I have not looked at the details at 
this point since we are still ages away from Xen being exactly capable 
of really doing a true implementation of the SVP architecture.
> 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.
As mentioned in the other response, the PF is in a higher trusted domain 
level than where the VFs would be assigned.
> 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).
In the SVP architecture, there is more than one "hardware domain".

v/r,
dps