[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 2/3] xen/pci: introduce PF<->VF links
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@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. > > 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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |