Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]

On Wed, Jul 17, 2019 at 03:53:47PM -0500, Michael Roth wrote:
> Quoting David Gibson (2019-07-16 20:29:12)
> > On Tue, Jul 16, 2019 at 11:30:01AM -0500, Michael Roth wrote:
> > > Quoting David Gibson (2019-07-14 21:25:24)
> > > > On Fri, Jul 12, 2019 at 09:34:46AM -0500, Michael Roth wrote:
> > > > > Quoting David Gibson (2019-07-12 01:46:19)
> > > > > > On Thu, Jul 11, 2019 at 08:19:34PM -0500, Michael Roth wrote:
> > > > > > > This implements the H_TPM_COMM hypercall, which is used by an
> > > > > > > Ultravisor to pass TPM commands directly to the host's TPM device, or
> > > > > > > a TPM Resource Manager associated with the device.
> > > > > > > 
> > > > > > > This also introduces a new pseries machine option which is used to
> > > > > > > configure what TPM device to pass commands to, for example:
> > > > > > > 
> > > > > > >   -machine pseries,...,tpm-device-file=/dev/tmprm0
> > > > > > 
> > > > > > Bolting this into yet another machine parameter seems kind of ugly.
> > > > > > Wouldn't it make more sense to treat this as an virtual device (say
> > > > > > "spapr-vtpm").  Adding that device would enable the hcall, and would
> > > > > > have properties for the back end host device.
> > > > > 
> > > > > That does sound nicer.
> > > > > 
> > > > > Originally I had SpaprMachineClass implement the TYPE_TPM_IF interface so
> > > > > we could define a TPM backend via -tpmdev passthrough,path=..., but after
> > > > > some discussion with the TPM maintainer it didn't quite work for the main
> > > > > use-case of passing through a TPM Resource Manager since it isn't suitable
> > > > > for full vTPM front-ends (since multiple guests can interfere with each
> > > > > other's operations when running the full gamut of TPM functionality).
> > > > > 
> > > > > I hadn't consider a stand-alone -device implementation though. It's not
> > > > > a proper VIO or PCI device so there's no proper bus to attach it to. I
> > > > > guess we would just make it a direct child of SpaprMachineState (sort
> > > > > of like SpaprDrcClass), then we could define it via something like
> > > > > -object spapr-tpm-proxy,path=....
> > > > 
> > > > It should be -device not -object, but otherwise that looks ok.
> > > 
> > > Ok, for some reason I thought -device needed either a specific bus or
> > > needed to be a SysBusDevice to attach to main-system-bus, but maybe that
> > > was just for qdev-managed reset handling. I've re-worked the series to
> > > allow configuration via:
> > > 
> > >   -device spapr-tpm-proxy,host_path=/dev/tpmrmX
> > 
> > That looks good.
> > 
> > > > How does the TPM appear in the device tree?
> > > 
> > > Nothing in the guest, on the host it appears as:
> > 
> > Hrm.  That seems unwise.  I mean, I guess its being treated as a
> > hypervisor facility rather than a device per se, but what if we ever
> > need to advertise more metadata about it.
> 
> It's a little bit awkward using a device tree in this case since it's
> generally the ultravisor that will be making this hcall on behalf of
> a guest requesting switch-over to SVM mode. The TPM device itself has
> a GetCapabilities command that seems like it would cover most of the
> metadata we might need though:
> 
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-3-Commands-01.38.pdf
> (page 340)
> 
> and if we need to add a layer of metadata on top of that there's also
> the option of introducing support for an additional operation in
> H_TPM_COMM itself, e.g. TPM_COMM_OP_GET_CAPABILITIES. Unsupported or
> invalid operations have a unique H_PARAMETER return code so callers
> should be able to reliably probe for it in the future if they need
> more information.

Yeah, ok.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]