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

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

Alexey Kardashevskiy wrote:
[..]
> >>> +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 :)

Hey, what's kernel development without little side-arguments about
whitespace? Will leave it alone for now.

> >>> +	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)

Effectively, yes. In the non-TDISP case the driver handles the MSE+BME
transition, in the TDISP case the driver also effectively handles the
same as BME+MSE are superseded by the LOCKED state.

So TVM userspace is responsible for marking the device "accepted" and the
driver checks that state before enabling the device (LOCKED -> RUN).

This also allows for kernel debug overrides of the acceptance policy,
because, in the end, the Linux kernel trusts drivers. If the TVM owner
loads a driver that ignores the "accepted" bit, that is the owner's
prerogative. If the TVM owner does not trust a driver there are multiple
knobs under the TVM's control to mitigate that mistrust.

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

My heartburn with that is that there is an indefinite amount of time
whereby a device is MSE + BME active without any driver to deal with the
consequences. For example, what if the device needs some form of reset /
re-initialization to quiet an engine or silence an interrupt that
immediately starts firing upon the LOCKED -> RUN transition. Userspace
is not in a good position to make judgements about the state of the
device outside of the Interface Report.

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

Yes, I now think entry into "RUN" needs to be a driver triggered event
to maintain parity with the safety of the non-TDISP case.

[..]
> >>> @@ -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/

Ugh, yes, it seems that joke: "debugging is a murder mystery where you
find out you were the killer the whole time." can also be true for patch
review.

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

Now that is a problem independent of the ops unification question. The
'struct pci_tsm_pf0' data-type should not be used for guest devices. I
will rework that to be a separate data-structure, but still keep
'pci_tsm_ops' unified since the signatures are identical.

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

Here is the reason my thinking evolved from that comment. A primary goal
of drivers/pci/tsm.c is to give one "Device Security" lifetime model to
the PCI core. That means TSM driver discovery (host or guest) lights up
TEE I/O capabilities in the PCI topology. That supports "pci_tsm_ops +
mode flag" vs separate registration mechanisms for different ops.

I also am not perceiving the need for guest-specific ops beyond
->accept(), as part of what drove my reaction to that RFC proposal was
the quantity of proposed ops.

So today's "good reason" is the useful programming pattern of "push
complexity from core-to-leaf". Where the low-level TSM driver needs to
be "mode" aware for some operations.