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

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

On 11/4/24 06:45, 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?
Correct, and as I hopefully clarified in response to Roger, like 
everything with virtualization, it's turtles all the way down. Try 
having this model in mind,
  +----------------+  +-------------------------------+
  | Supervisor VP  |  | Guest VP                      |
  |                |  |                               |
  | +------------+ |  | +------------+ +------------+ |
  | |    HWDOM   | |  | |    HWDOM   | |    Guest   | |
  | |  +------+  | |  | |  +------+  | |  +------+  | |
  | |  |      |  | |  | |  |      |  | |  |      |  | |
  | |  |  PF  |  | |  | |  |  VF  +--+-+-->  PCI |  | |
  | |  |      |  | |  | |  |      |  | |  |      |  | |
  | |  +------+  | |  | |  +------+  | |  +------+  | |
  | +------------+ |  | +------------+ +------------+ |
  +----------------+  +-------------------------------+


> 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.
Yes, I would agree that is reasonable. Please, I am not trying to make 
this complicated, but just trying to give a more advanced (nested?) 
model to consider. Not something that we have to figure out all the 
details of how to make it all work. I am just saying, consider that a VF 
may have a different label than the PF which allows another domain to be 
able to take actions on the device as if the domain fully owns that 
virtual PCI device. A code comment around the xsm check(s) could state 
that it is expected that a system with access to the PF label must have 
access to all VF labels and the dummy policy should already be wired up 
to only allow the hwdom to do it.
> That would do the clean thing in the common case and be consistent with the
> security policy even with a conflicting policy. The semantics are somewhat more
> complex, but trying to remove a PF before removing the VFs is silly and the
> only sensible thing (imo) is to help out during cleanup _or_ be strict about
> checking.
Yes, and I was not trying to suggest the PF would be removed before the 
VFs. Apologies if it sounds as though I am repeating, myself,, all that 
I was attempting to say is that the VFs should be able to be checked 
separately from the PF. This would allow a domain that was granted 
ownership of a VF, to be able to unplug the VF and only that VF from the 
system. I would leave it to the PCI logic to manage the sequencing of 
the PF and VF operations. In such a scenario, I would expect the hwdom 
to have the facilities to monitor/check the status of the VF to manage 
the case that a guest unplugged it.
v/r,
dps