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

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

On 11/5/24 04:08, Roger Pau Monné wrote:
> 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.
Apologies for not fully defining how SVP would work. The Supervisor is 
the one of the few domains considered running at the higher trust level.
When I was referring to restricting the VF, for instance, that a VF of 
L:M:N may be assigned label gvp1_pci_t that only allows guest VP 1 
access, while VF X:Y:Z would be labeled gvp2_pci_t that grants guest VP 
2 access only.
At the same time, the Supervisor would be allowed in the policy to 
remove all VFs, in the example above it would have access to 
gvp1/2_pci_t labeled devices. In theory, it would have attempted to have 
the Guest VP tear down (or unplug the VF in a hotplug scenario) the 
virtual device. But in the end, it can't rely on the Guest VP to 
properly shutdown/unplug, and must be able to properly manage the system.
> 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.
Correct, as I mentioned above.

v/r,
dps