Re: [PATCH v3 13/13] PCI/TSM: Add Guest TSM Support

[Dan Williams <dan.j.williams@intel.com>]

Alexey Kardashevskiy wrote:
> 
> 
> On 16/5/25 15:47, Dan Williams wrote:
> > From: Xu Yilun <yilun.xu@linux.intel.com>
> > 
> > Enable PCI TSM/TSM core to support assigned device authentication in
> > CoCo VM. The main changes are:
> > 
> >   - Add an ->accept() operation which also flags whether the TSM driver is
> >     host or guest context.
> >   - Re-purpose the 'connect' sysfs attribute in guest to lock down the
> >     private configuration for the device.
> >   - Add the 'accept' sysfs attribute for guest to accept the private
> >     device into its TEE.
> >   - Skip DOE setup/transfer for guest TSM managed devices.
> > 
> > All private capable assigned PCI devices (TDI) start as shared. CoCo VM
> > should authenticate some of these devices and accept them in its TEE for
> > private memory access. TSM supports this authentication in 3 steps:
> > Connect, Attest and Accept.
> > 
> > On Connect, CoCo VM requires hypervisor to finish all private
> > configurations to the device and put the device in TDISP CONFIG_LOCKED
> > state. Please note this verb has different meaning from host context. On
> > host, Connect means establish secure physical link (e.g. PCI IDE).
> > 
> > On Attest, CoCo VM retrieves evidence from device and decide if the
> > device is good for accept. The CoCo VM kernel provides evidence,
> > userspace decides if the evidence is good based on its own strategy.
> > 
> > On Accept, userspace has acknowledged the evidence and requires CoCo VM
> > kernel to enable private MMIO & DMA access. Usually it ends up by put
> > the device in TDISP RUN state.
> > 
> > Currently only implement Connect & Accept to enable a minimum flow for
> > device shared <-> private conversion. There is no evidence retrieval
> > interfaces, only to assume the device evidences are always good without
> > attestation.
> > 
> > The shared -> private conversion:
> > 
> >    echo 1 > /sys/bus/pci/devices/<...>/tsm/connect
> >    echo 1 > /sys/bus/pci/devices/<...>/tsm/accept
> > 
> > The private -> shared conversion:
> > 
> >    echo 0 > /sys/bus/pci/devices/<...>/tsm/connect
> > 
> > Since the device's MMIO & DMA are all blocked after Connect & before
> > Accept, device drivers are not considered workable in this intermediate
> > state. The Connect and Accept transitions only proceed while the driver is
> > detached. Note this can be relaxed later with a callback to an enlightened
> > driver to coordinate the transition, but for now, require detachment.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >   drivers/pci/tsm.c       | 160 +++++++++++++++++++++++++++++++++++-----
> >   include/linux/pci-tsm.h |  15 +++-
> >   2 files changed, 152 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > index 219e40c5d4e7..794de2f258c3 100644
> > --- a/drivers/pci/tsm.c
> > +++ b/drivers/pci/tsm.c
> > @@ -124,6 +124,29 @@ static int pci_tsm_disconnect(struct pci_dev *pdev)
> >   	return 0;
> >   }
> >   
> > +/*
> > + * TDISP locked state temporarily makes the device inaccessible, do not
> > + * surprise live attached drivers
> > + */
> > +static int __driver_idle_connect(struct pci_dev *pdev)
> 
> Do not need "__"...

I am ok to drop. The thought was to make this nuance stand-out more, as
one more level of indirection than typically necessary, but no need to
push that preference.

> > +{
> > +	guard(device)(&pdev->dev);
> > +	if (pdev->dev.driver)
> > +		return -EBUSY;
> 
> > +	return tsm_ops->connect(pdev);
> > +}
> > +
> > +/*
> > + * When the registered ops support accept it indicates that this is a
> > + * TVM-side (guest) TSM operations structure. In this mode ->connect()
> > + * arranges for the TDI to enter TDISP LOCKED state, and ->accept()
> > + * transitions the device to RUN state.
> > + */
> > +static bool tvm_mode(void)
> > +{
> > +	return !!tsm_ops->accept;
> 
> tsm_ops->accept != NULL

Yeah, that is a bit more idiomatic.

> > +}
> > +
> >   static int pci_tsm_connect(struct pci_dev *pdev)
> >   {
> >   	struct pci_tsm_pf0 *tsm = to_pci_tsm_pf0(pdev->tsm);
> > @@ -138,13 +161,47 @@ static int pci_tsm_connect(struct pci_dev *pdev)
> >   	if (tsm->state >= PCI_TSM_CONNECT)
> >   		return 0;
> >   
> > -	rc = tsm_ops->connect(pdev);
> > +	if (tvm_mode())
> > +		rc = __driver_idle_connect(pdev);
> 
> ... or just open code it here?

I will do with dropping the "__" and keeping the helper with the
comment to keep this function less busy.

> > +	else
> > +		rc = tsm_ops->connect(pdev);
> >   	if (rc)
> >   		return rc;
> >   	tsm->state = PCI_TSM_CONNECT;
> >   	return 0;
> >   }
> >   
> > +static int pci_tsm_accept(struct pci_dev *pdev)
> > +{
> > +	struct pci_tsm_pf0 *tsm = to_pci_tsm_pf0(pdev->tsm);
> > +	int rc;
> > +
> > +	struct mutex *lock __free(tsm_ops_unlock) = tsm_ops_lock(tsm);
> 
> Add an empty line.

I think we, as a community, are still figuring out the coding-style
around scope-based cleanup declarations, but I would argue, no empty
line required after mid-function variable declarations. Now, in this
case it is arguably not "mid-function", but all the other occurrences of
tsm_ops_lock() are checking the result on the immediate next line.

> > +	if (!lock)
> > +		return -EINTR;
> > +
> > +	if (tsm->state < PCI_TSM_CONNECT)
> > +		return -ENXIO;
> > +	if (tsm->state >= PCI_TSM_ACCEPT)
> > +		return 0;
> > +
> > +	/*
> > +	 * "Accept" transitions a device to the run state, it is only suitable
> > +	 * to make that transition from a known DMA-idle (no active mappings)
> > +	 * state.  The "driver detached" state is a coarse way to assert that
> > +	 * requirement.
> 
> And then the userspace will modprobe the driver, which will enable BME
> and MSE which in turn will render the ERROR state, what is the plan
> here?

Right, so the notifier proposal [1] gave me pause because of perceived
complexity and my general reluctance to rely on the magic of notifiers
when an explicit sequence is easier to read/maintain. The proposal is
that drivers switch to TDISP aware setup helpers that understand that
BME and MSE were handled at LOCK. Becauase it is not just
pci_enable_device() / pci_set_master() awareness that is needed but also
pci_disable_device() / pci_clear_master() flows that need consideration
for avoiding/handling the TDISP ERROR state.

I.e. support for TDISP-unaware drivers is not a goal.

There are still details to work out like supporting drivers that want to
stay loaded over the UNLOCKED->LOCKED->RUN transitions, and whether the
"accept" UAPI triggers entry into "RUN" or still requires a driver to
perform that.

[1]: http://lore.kernel.org/20250218111017.491719-20-aik@amd.com

[..]
> > @@ -558,11 +675,11 @@ int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u64 tdi_id)
> >   	if (!pdev->tsm)
> >   		return -EINVAL;
> >   
> > -	struct pci_dev *pf0_dev __free(pci_dev_put) = tsm_pf0_get(pdev);
> > -	if (!pf0_dev)
> > +	struct pci_dev *dsm_dev __free(pci_dev_put) = dsm_dev_get(pdev);
> > +	if (!dsm_dev)
> >   		return -EINVAL;
> 
> A bunch of changes like this one belong to 12/13.

Whoops, yes, missed that before sending.

[..]
> > @@ -135,6 +141,8 @@ struct pci_tsm_guest_req_info {
> >    * @bind: establish a secure binding with the TVM
> >    * @unbind: teardown the secure binding
> >    * @guest_req: handle the vendor specific requests from TVM when bound
> > + * @accept: TVM-only operation to confirm that system policy allows
> > + *	    device to access private memory and be mapped with private mmio.
> >    *
> >    * @probe and @remove run in pci_tsm_rwsem held for write context. All
> >    * other ops run under the @pdev->tsm->lock mutex and pci_tsm_rwsem held
> > @@ -150,6 +158,7 @@ struct pci_tsm_ops {
> >   	void (*unbind)(struct pci_tdi *tdi);
> >   	int (*guest_req)(struct pci_dev *pdev,
> >   			 struct pci_tsm_guest_req_info *info);
> > +	int (*accept)(struct pci_dev *pdev);
> 
> 
> When I posted my v1, I got several comments to not put host and guest
> callbacks together which makes sense (as only really "connect" and
> "status" are shared, and I am not sure I like dual use of "connect")
> and so did I in v2 and yet you are pushing for one struct for all?

Frankly, I missed that feedback and was focused on how to simply extend
PCI to understand TSM semantics.

Part of the motivation is reduce the number of details that
drivers/pci/tsm.c needs to consider. I.e. there is only one ops object
to manage. Can you share the lore links for the comments that convinced
you to change course? Maybe those folks can chime in again here, but I
might have been too focused my tdi_dev object model concerns to notice
those previously.

As for repurposing "connect" that also comes back to question of whether
userspace needs to see or care about that nuance. In the end "connect"
is "prepare this device for follow-on TSM + TDISP security operations".
If the "accept" attribute is present userspace can infer that it has
more work to get the device operational in the TEE. So the interface can
indeed be more verbose... but to what end?