On 21/5/25 07:11, Dan Williams wrote:
Alexey Kardashevskiy wrote:
On 16/5/25 15:47, Dan Williams wrote:
From: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
Co-developed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
---
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.
Do not really care as much :)
+ 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.
So your plan it to modify driver to switch to the secure mode on the go? (not arguing, just asking for now)
The alternative is - let TSM do the attestation and acceptance and then "modprobe tdispawaredriver tdisp=on" and change the driver to assume that BME and MSE are already enabled.
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@xxxxxxx
[..]
@@ -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.
That was literally you (and I think someone else mentioned it too) ;)
https://lore.kernel.org/all/66d7a10a4d621_3975294ac@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch/
"Lets not mix HV and VM hooks in the same ops without good reason" and I do not see a good reason here yet.
More to the point, the host and guest have very little in common to have one ops struct for both and then deal with questions "do we execute the code related to PF0 in the guest", etc.
My life definitely got easier with 2 separate structures and my split to virt/coco/...(tsm-host.c|tsm-guest.c|tsm.c) + pci/tsm.c.
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".
I see "connect" more like "set up ssh connection with some ports forwarded"
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?
... and these "accept" as connecting via ssh-forwarded ports. Not the same thing. But not really an issue here. Thanks,
--
Alexey
