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

[Alexey Kardashevskiy <aik@amd.com>]

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@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.

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@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.

That was literally you (and I think someone else mentioned it too) ;)

https://lore.kernel.org/all/66d7a10a4d621_3975294ac@dwillia2-xfh.jf.intel.com.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