Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
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 signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
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. > > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57 > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/link-id > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-size > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/label > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/compatible > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/status > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/reg > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/phandle > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-base > > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/name > > > > > > > > > I'll go ahead and give that a shot, assuming it seems reasonable to you. > > > > > > > > > > > > > > > By default, no tpm-device-file is defined and hcalls will return > > > > > > H_RESOURCE. > > > > > > > > > > Wouldn't H_FUNCTION make more sense? > > > > > > > > Yes, for this case it probably would. > > > > > > > > Thanks for the suggestions! > > > > > > > > > > > > > > > > > > > > > The full specification for this hypercall can be found in > > > > > > docs/specs/ppc-spapr-uv-hcalls.txt > > > > > > > > > > > > Signed-off-by: Michael Roth > > > > > --- > > > > > > hw/ppc/Makefile.objs | 1 + > > > > > > hw/ppc/spapr.c | 27 > > > > > > hw/ppc/spapr_hcall_tpm.c | 135 > > > > > > +++ > > > > > > hw/ppc/trace-events | 4 ++ > > >
Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
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. > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57 > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/link-id > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-size > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/label > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/compatible > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/status > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/reg > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/phandle > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-base > ./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/name > > > > > > I'll go ahead and give that a shot, assuming it seems reasonable to you. > > > > > > > > > > > > By default, no tpm-device-file is defined and hcalls will return > > > > > H_RESOURCE. > > > > > > > > Wouldn't H_FUNCTION make more sense? > > > > > > Yes, for this case it probably would. > > > > > > Thanks for the suggestions! > > > > > > > > > > > > > > > > > The full specification for this hypercall can be found in > > > > > docs/specs/ppc-spapr-uv-hcalls.txt > > > > > > > > > > Signed-off-by: Michael Roth > > > > --- > > > > > hw/ppc/Makefile.objs | 1 + > > > > > hw/ppc/spapr.c | 27 > > > > > hw/ppc/spapr_hcall_tpm.c | 135 > > > > > +++ > > > > > hw/ppc/trace-events | 4 ++ > > > > > include/hw/ppc/spapr.h | 7 +- > > > > > 5 files changed, 173 insertions(+), 1 deletion(-) > > > > > create mode 100644 hw/ppc/spapr_hcall_tpm.c > > > > > > > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs > > > > > index 9da93af905..5aa120cae6 100644 > > > > > --- a/hw/ppc/Makefile.objs > > > > > +++ b/hw/ppc/Makefile.objs > > > > > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o > > > > > spapr_vio.o spapr_events.o > > > > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o > > > > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o > > > > > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o > > > > > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o > > > > > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o > > > > > # IBM PowerNV > > > > > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o > > > > > pnv_psi.o pnv_occ.o pnv_bmc.o > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > > index 821f0d4a49..eb3421673b 100644 > > >
Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
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
>
> How does the TPM appear in the device tree?
Nothing in the guest, on the host it appears as:
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/link-id
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-size
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/label
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/compatible
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/status
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/reg
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/phandle
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-base
./xscom@603fc/i2cm@a2000/i2c-bus@0/tpm@57/name
>
> > I'll go ahead and give that a shot, assuming it seems reasonable to you.
> >
> > >
> > > > By default, no tpm-device-file is defined and hcalls will return
> > > > H_RESOURCE.
> > >
> > > Wouldn't H_FUNCTION make more sense?
> >
> > Yes, for this case it probably would.
> >
> > Thanks for the suggestions!
> >
> > >
> > > >
> > > > The full specification for this hypercall can be found in
> > > > docs/specs/ppc-spapr-uv-hcalls.txt
> > > >
> > > > Signed-off-by: Michael Roth > > > ---
> > > > hw/ppc/Makefile.objs | 1 +
> > > > hw/ppc/spapr.c | 27
> > > > hw/ppc/spapr_hcall_tpm.c | 135 +++
> > > > hw/ppc/trace-events | 4 ++
> > > > include/hw/ppc/spapr.h | 7 +-
> > > > 5 files changed, 173 insertions(+), 1 deletion(-)
> > > > create mode 100644 hw/ppc/spapr_hcall_tpm.c
> > > >
> > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > index 9da93af905..5aa120cae6 100644
> > > > --- a/hw/ppc/Makefile.objs
> > > > +++ b/hw/ppc/Makefile.objs
> > > > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o
> > > > spapr_vio.o spapr_events.o
> > > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > > > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> > > > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> > > > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> > > > # IBM PowerNV
> > > > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o
> > > > pnv_psi.o pnv_occ.o pnv_bmc.o
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 821f0d4a49..eb3421673b 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState
> > > > *machine)
> > > > */
> > > > object_child_foreach_recursive(object_get_root(),
> > > > spapr_reset_drcs, NULL);
> > > >
> > > > +if (spapr->tpm_device_file) {
> > > > +spapr_hcall_tpm_reset();
> > > > +}
> > > > +
> > > > spapr_clear_pending_events(spapr);
> > > >
> > > > /*
> > > >
Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
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.
How does the TPM appear in the device tree?
> I'll go ahead and give that a shot, assuming it seems reasonable to you.
>
> >
> > > By default, no tpm-device-file is defined and hcalls will return
> > > H_RESOURCE.
> >
> > Wouldn't H_FUNCTION make more sense?
>
> Yes, for this case it probably would.
>
> Thanks for the suggestions!
>
> >
> > >
> > > The full specification for this hypercall can be found in
> > > docs/specs/ppc-spapr-uv-hcalls.txt
> > >
> > > Signed-off-by: Michael Roth > > ---
> > > hw/ppc/Makefile.objs | 1 +
> > > hw/ppc/spapr.c | 27
> > > hw/ppc/spapr_hcall_tpm.c | 135 +++
> > > hw/ppc/trace-events | 4 ++
> > > include/hw/ppc/spapr.h | 7 +-
> > > 5 files changed, 173 insertions(+), 1 deletion(-)
> > > create mode 100644 hw/ppc/spapr_hcall_tpm.c
> > >
> > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > index 9da93af905..5aa120cae6 100644
> > > --- a/hw/ppc/Makefile.objs
> > > +++ b/hw/ppc/Makefile.objs
> > > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o
> > > spapr_events.o
> > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> > > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> > > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> > > # IBM PowerNV
> > > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o
> > > pnv_psi.o pnv_occ.o pnv_bmc.o
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 821f0d4a49..eb3421673b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState
> > > *machine)
> > > */
> > > object_child_foreach_recursive(object_get_root(), spapr_reset_drcs,
> > > NULL);
> > >
> > > +if (spapr->tpm_device_file) {
> > > +spapr_hcall_tpm_reset();
> > > +}
> > > +
> > > spapr_clear_pending_events(spapr);
> > >
> > > /*
> > > @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj,
> > > const char *value, Error **errp)
> > > spapr->host_serial = g_strdup(value);
> > > }
> > >
> > > +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> > > +{
> > > +SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > +
> > > +return g_strdup(spapr->tpm_device_file);
> > > +}
> > > +
> > > +static void spapr_set_tpm_device_file(Object *obj, const char *value,
> > > Error **errp)
> > > +{
> > > +SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > +
> > > +g_free(spapr->tpm_device_file);
> > > +spapr->tpm_device_file = g_strdup(value);
> > > +}
> > > +
> > > static void spapr_instance_init(Object *obj)
> > > {
> > > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> > > _abort);
> > > object_property_set_description(obj, "host-serial",
> > > "Host serial number to advertise in guest device tree",
> > > _abort);
> > > +object_property_add_str(obj, "tpm-device-file",
> > > +
Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
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=
I'll go ahead and give that a shot, assuming it seems reasonable to you.
>
> > By default, no tpm-device-file is defined and hcalls will return
> > H_RESOURCE.
>
> Wouldn't H_FUNCTION make more sense?
Yes, for this case it probably would.
Thanks for the suggestions!
>
> >
> > The full specification for this hypercall can be found in
> > docs/specs/ppc-spapr-uv-hcalls.txt
> >
> > Signed-off-by: Michael Roth > ---
> > hw/ppc/Makefile.objs | 1 +
> > hw/ppc/spapr.c | 27
> > hw/ppc/spapr_hcall_tpm.c | 135 +++
> > hw/ppc/trace-events | 4 ++
> > include/hw/ppc/spapr.h | 7 +-
> > 5 files changed, 173 insertions(+), 1 deletion(-)
> > create mode 100644 hw/ppc/spapr_hcall_tpm.c
> >
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index 9da93af905..5aa120cae6 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o
> > spapr_events.o
> > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> > # IBM PowerNV
> > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o
> > pnv_occ.o pnv_bmc.o
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 821f0d4a49..eb3421673b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState
> > *machine)
> > */
> > object_child_foreach_recursive(object_get_root(), spapr_reset_drcs,
> > NULL);
> >
> > +if (spapr->tpm_device_file) {
> > +spapr_hcall_tpm_reset();
> > +}
> > +
> > spapr_clear_pending_events(spapr);
> >
> > /*
> > @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const
> > char *value, Error **errp)
> > spapr->host_serial = g_strdup(value);
> > }
> >
> > +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> > +{
> > +SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > +return g_strdup(spapr->tpm_device_file);
> > +}
> > +
> > +static void spapr_set_tpm_device_file(Object *obj, const char *value,
> > Error **errp)
> > +{
> > +SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > +g_free(spapr->tpm_device_file);
> > +spapr->tpm_device_file = g_strdup(value);
> > +}
> > +
> > static void spapr_instance_init(Object *obj)
> > {
> > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> > _abort);
> > object_property_set_description(obj, "host-serial",
> > "Host serial number to advertise in guest device tree",
> > _abort);
> > +object_property_add_str(obj, "tpm-device-file",
> > +spapr_get_tpm_device_file,
> > +spapr_set_tpm_device_file, _abort);
> > +object_property_set_description(obj, "tpm-device-file",
> > + "Specifies the path to the TPM character device file to
> > use"
> > + " for TPM communication via hypercalls (usually a TPM"
> > + " resource manager)",
> > + _abort);
> > }
> >
> >
Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
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.
> By default, no tpm-device-file is defined and hcalls will return
> H_RESOURCE.
Wouldn't H_FUNCTION make more sense?
>
> The full specification for this hypercall can be found in
> docs/specs/ppc-spapr-uv-hcalls.txt
>
> Signed-off-by: Michael Roth ---
> hw/ppc/Makefile.objs | 1 +
> hw/ppc/spapr.c | 27
> hw/ppc/spapr_hcall_tpm.c | 135 +++
> hw/ppc/trace-events | 4 ++
> include/hw/ppc/spapr.h | 7 +-
> 5 files changed, 173 insertions(+), 1 deletion(-)
> create mode 100644 hw/ppc/spapr_hcall_tpm.c
>
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 9da93af905..5aa120cae6 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o
> spapr_events.o
> obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> # IBM PowerNV
> obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o
> pnv_occ.o pnv_bmc.o
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 821f0d4a49..eb3421673b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState *machine)
> */
> object_child_foreach_recursive(object_get_root(), spapr_reset_drcs,
> NULL);
>
> +if (spapr->tpm_device_file) {
> +spapr_hcall_tpm_reset();
> +}
> +
> spapr_clear_pending_events(spapr);
>
> /*
> @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const
> char *value, Error **errp)
> spapr->host_serial = g_strdup(value);
> }
>
> +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> +{
> +SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +return g_strdup(spapr->tpm_device_file);
> +}
> +
> +static void spapr_set_tpm_device_file(Object *obj, const char *value, Error
> **errp)
> +{
> +SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +g_free(spapr->tpm_device_file);
> +spapr->tpm_device_file = g_strdup(value);
> +}
> +
> static void spapr_instance_init(Object *obj)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> _abort);
> object_property_set_description(obj, "host-serial",
> "Host serial number to advertise in guest device tree",
> _abort);
> +object_property_add_str(obj, "tpm-device-file",
> +spapr_get_tpm_device_file,
> +spapr_set_tpm_device_file, _abort);
> +object_property_set_description(obj, "tpm-device-file",
> + "Specifies the path to the TPM character device file to use"
> + " for TPM communication via hypercalls (usually a TPM"
> + " resource manager)",
> + _abort);
> }
>
> static void spapr_machine_finalizefn(Object *obj)
> diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
> new file mode 100644
> index 00..75e2b6d594
> --- /dev/null
> +++ b/hw/ppc/spapr_hcall_tpm.c
> @@ -0,0 +1,135 @@
> +/*
> + * SPAPR TPM Hypercall
> + *
> + * Copyright IBM Corp. 2019
> + *
> + * Authors:
> + * Michael Roth
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "cpu.h"
> +#include "hw/ppc/spapr.h"
> +#include "trace.h"
> +
> +#define TPM_SPAPR_BUFSIZE 4096
> +
> +enum {
> +TPM_COMM_OP_EXECUTE = 1,
> +TPM_COMM_OP_CLOSE_SESSION = 2,
> +};
> +
> +static int tpm_devfd = -1;
A global? Really? You can do better.
> +
> +static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
> +{
> +uint64_t data_in = ppc64_phys_to_real(args[1]);
> +target_ulong data_in_size = args[2];
> +uint64_t data_out = ppc64_phys_to_real(args[3]);
> +target_ulong data_out_size = args[4];
> +
[Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
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
By default, no tpm-device-file is defined and hcalls will return
H_RESOURCE.
The full specification for this hypercall can be found in
docs/specs/ppc-spapr-uv-hcalls.txt
Signed-off-by: Michael Roth tpm_device_file) {
+spapr_hcall_tpm_reset();
+}
+
spapr_clear_pending_events(spapr);
/*
@@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const
char *value, Error **errp)
spapr->host_serial = g_strdup(value);
}
+static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
+{
+SpaprMachineState *spapr = SPAPR_MACHINE(obj);
+
+return g_strdup(spapr->tpm_device_file);
+}
+
+static void spapr_set_tpm_device_file(Object *obj, const char *value, Error
**errp)
+{
+SpaprMachineState *spapr = SPAPR_MACHINE(obj);
+
+g_free(spapr->tpm_device_file);
+spapr->tpm_device_file = g_strdup(value);
+}
+
static void spapr_instance_init(Object *obj)
{
SpaprMachineState *spapr = SPAPR_MACHINE(obj);
@@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
_abort);
object_property_set_description(obj, "host-serial",
"Host serial number to advertise in guest device tree", _abort);
+object_property_add_str(obj, "tpm-device-file",
+spapr_get_tpm_device_file,
+spapr_set_tpm_device_file, _abort);
+object_property_set_description(obj, "tpm-device-file",
+ "Specifies the path to the TPM character device file to use"
+ " for TPM communication via hypercalls (usually a TPM"
+ " resource manager)",
+ _abort);
}
static void spapr_machine_finalizefn(Object *obj)
diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
new file mode 100644
index 00..75e2b6d594
--- /dev/null
+++ b/hw/ppc/spapr_hcall_tpm.c
@@ -0,0 +1,135 @@
+/*
+ * SPAPR TPM Hypercall
+ *
+ * Copyright IBM Corp. 2019
+ *
+ * Authors:
+ * Michael Roth
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "cpu.h"
+#include "hw/ppc/spapr.h"
+#include "trace.h"
+
+#define TPM_SPAPR_BUFSIZE 4096
+
+enum {
+TPM_COMM_OP_EXECUTE = 1,
+TPM_COMM_OP_CLOSE_SESSION = 2,
+};
+
+static int tpm_devfd = -1;
+
+static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
+{
+uint64_t data_in = ppc64_phys_to_real(args[1]);
+target_ulong data_in_size = args[2];
+uint64_t data_out = ppc64_phys_to_real(args[3]);
+target_ulong data_out_size = args[4];
+uint8_t buf_in[TPM_SPAPR_BUFSIZE];
+uint8_t buf_out[TPM_SPAPR_BUFSIZE];
+ssize_t ret;
+
+trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
+
+if (data_in_size > TPM_SPAPR_BUFSIZE) {
+error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
+ data_in_size);
+return H_P3;
+}
+
+if (data_out_size < TPM_SPAPR_BUFSIZE) {
+error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
+ data_out_size);
+return H_P5;
+}
+
+if (tpm_devfd == -1) {
+tpm_devfd = open(spapr->tpm_device_file, O_RDWR);
+if (tpm_devfd == -1) {
+error_report("failed to open TPM device %s: %d",
+ spapr->tpm_device_file, errno);
+return H_RESOURCE;
+}
+}
+
+cpu_physical_memory_read(data_in, buf_in, data_in_size);
+
+do {
+ret = write(tpm_devfd, buf_in, data_in_size);
+if (ret > 0) {
+data_in_size -= ret;
+}
+} while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
+
+if (ret == -1) {
+error_report("failed to write to TPM device %s: %d",
+ spapr->tpm_device_file, errno);
+return H_RESOURCE;
+}
+
+do {
+ret = read(tpm_devfd, buf_out, data_out_size);
+} while (ret == 0 || (ret == -1 && errno == EINTR));
+
+if (ret == -1) {
+error_report("failed to read from TPM device %s: %d",
+ spapr->tpm_device_file, errno);
+return H_RESOURCE;
+}
+
+cpu_physical_memory_write(data_out, buf_out, ret);
+args[0] = ret;
+
+return H_SUCCESS;
+}
+
+static target_ulong h_tpm_comm(PowerPCCPU *cpu,
+ SpaprMachineState *spapr,
+ target_ulong opcode,
+ target_ulong
