Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On Wed, Dec 18, 2019 at 08:59:18PM -0500, Stefan Berger wrote: > On 12/18/19 8:54 PM, David Gibson wrote: > > On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote: > > > On 12/16/19 7:29 PM, David Gibson wrote: > > > > > > > > > > Since you need to change compatible based on an internal variable, > > > > we'd need to replace the static dt_compatible in the class with a > > > > callback. > > > > > > Why can we not initialize it once we know the version of TPM? From the > > > perspective of SLOF at least this seems to be building the device tree > > > fine > > > since it sees the proper value... > > Because it's a serious layering / isolation violation. You're > > modifying QOM type information from the runtime code of a specific > > instance. You get away with it (now) because there's only one > > instance and the ordering of things happens to let it work, but that's > > assuming way too much about QOM's implementation details. > > > > As a rule, once the QOM classes are set up with their class_init > > function, they should never be modified. > > > If we now add a get_dt_compatible() callback to the class that gets invoked > when dt_compatible is NULL, does this then solve the issue? Yes, that's what I'm suggesting. -- 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: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On Thu, Dec 19, 2019 at 04:13:57PM +1100, David Gibson wrote: > On Wed, Dec 18, 2019 at 08:59:18PM -0500, Stefan Berger wrote: > > On 12/18/19 8:54 PM, David Gibson wrote: > > > On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote: > > > > On 12/16/19 7:29 PM, David Gibson wrote: > > > > > > > > > > > > > Since you need to change compatible based on an internal variable, > > > > > we'd need to replace the static dt_compatible in the class with a > > > > > callback. > > > > > > > > Why can we not initialize it once we know the version of TPM? From the > > > > perspective of SLOF at least this seems to be building the device tree > > > > fine > > > > since it sees the proper value... > > > Because it's a serious layering / isolation violation. You're > > > modifying QOM type information from the runtime code of a specific > > > instance. You get away with it (now) because there's only one > > > instance and the ordering of things happens to let it work, but that's > > > assuming way too much about QOM's implementation details. > > > > > > As a rule, once the QOM classes are set up with their class_init > > > function, they should never be modified. > > > > > > If we now add a get_dt_compatible() callback to the class that gets invoked > > when dt_compatible is NULL, does this then solve the issue? > > Yes, that's what I'm suggesting. Well, almost. Actually I'd suggest the other way around - call the callback method, but if that's NULL, fallback to the static value. -- 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: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On 12/18/19 8:54 PM, David Gibson wrote: On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote: On 12/16/19 7:29 PM, David Gibson wrote: Since you need to change compatible based on an internal variable, we'd need to replace the static dt_compatible in the class with a callback. Why can we not initialize it once we know the version of TPM? From the perspective of SLOF at least this seems to be building the device tree fine since it sees the proper value... Because it's a serious layering / isolation violation. You're modifying QOM type information from the runtime code of a specific instance. You get away with it (now) because there's only one instance and the ordering of things happens to let it work, but that's assuming way too much about QOM's implementation details. As a rule, once the QOM classes are set up with their class_init function, they should never be modified. If we now add a get_dt_compatible() callback to the class that gets invoked when dt_compatible is NULL, does this then solve the issue?
Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote:
> On 12/16/19 7:29 PM, David Gibson wrote:
> > On Fri, Dec 13, 2019 at 08:03:36AM -0500, Stefan Berger wrote:
> > > On 12/13/19 12:34 AM, David Gibson wrote:
> > >
> > > The existing one looks like this:
> > >
> > > typedef struct SpaprVioCrq {
> > > uint64_t qladdr;
> > > uint32_t qsize;
> > > uint32_t qnext;
> > > int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
> > > } SpaprVioCrq;
> > >
> > > I don't seem to find the fields there that we need for vTPM support.
> > Yeah, I can see the difference in the structures. What I'm after is
> > what is the difference in purpose which means they have different
> > content.
> >
> > Having read through the whole series now, I *think* the answer is that
> > the tpm specific structure is one entry in the request queue for the
> > vtpm, whereas the VioCrq structure is a handle on an entire queue.
> >
> > I think the tpm one needs a rename to reflect that a) it's vtpm
> > specific and b) it's not actually a queue, just part of it.
>
>
> v6 has it as TpmCrq. It's local to the file, so from that perspective it's
> specific to (v)TPM.
Ok.
> > > This is a 1:1 copy from the existing TIS driver.
> > Hm, right. Probably not a bad idea to move that out as a helper
> > function then.
>
>
> In V7 then.
Ok.
> > > > > +static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
> > > > > +{
> > > > > +SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> > > > > +SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> > > > > +
> > > > > +switch (s->be_tpm_version) {
> > > > > +case TPM_VERSION_UNSPEC:
> > > > > +assert(false);
> > > > > +break;
> > > > > +case TPM_VERSION_1_2:
> > > > > +k->dt_name = "vtpm";
> > > > > +k->dt_type = "IBM,vtpm";
> > > > > +k->dt_compatible = "IBM,vtpm";
> > > > > +break;
> > > > > +case TPM_VERSION_2_0:
> > > > > +k->dt_name = "vtpm";
> > > > > +k->dt_type = "IBM,vtpm";
> > > > > +k->dt_compatible = "IBM,vtpm20";
> > > > > +break;
> > > > Erk. Updating DeviceClass structures on the fly is hideously ugly.
> > > > We might need to take a different approach for this.
> > > Make a suggestion... Obviously, we can hard-initialize dt_name and dt_type
> > > but dt_compatible can only be set after we have determined the version of
> > > TPM.
> > As you say name and type can just be put into the class statically.
>
>
> I did this in v6.
>
>
> > Since you need to change compatible based on an internal variable,
> > we'd need to replace the static dt_compatible in the class with a
> > callback.
>
>
> Why can we not initialize it once we know the version of TPM? From the
> perspective of SLOF at least this seems to be building the device tree fine
> since it sees the proper value...
Because it's a serious layering / isolation violation. You're
modifying QOM type information from the runtime code of a specific
instance. You get away with it (now) because there's only one
instance and the ordering of things happens to let it work, but that's
assuming way too much about QOM's implementation details.
As a rule, once the QOM classes are set up with their class_init
function, they should never be modified.
--
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: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On 12/16/19 7:29 PM, David Gibson wrote:
On Fri, Dec 13, 2019 at 08:03:36AM -0500, Stefan Berger wrote:
On 12/13/19 12:34 AM, David Gibson wrote:
The existing one looks like this:
typedef struct SpaprVioCrq {
uint64_t qladdr;
uint32_t qsize;
uint32_t qnext;
int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
} SpaprVioCrq;
I don't seem to find the fields there that we need for vTPM support.
Yeah, I can see the difference in the structures. What I'm after is
what is the difference in purpose which means they have different
content.
Having read through the whole series now, I *think* the answer is that
the tpm specific structure is one entry in the request queue for the
vtpm, whereas the VioCrq structure is a handle on an entire queue.
I think the tpm one needs a rename to reflect that a) it's vtpm
specific and b) it's not actually a queue, just part of it.
v6 has it as TpmCrq. It's local to the file, so from that perspective
it's specific to (v)TPM.
This is a 1:1 copy from the existing TIS driver.
Hm, right. Probably not a bad idea to move that out as a helper
function then.
In V7 then.
+static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
+{
+SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
+SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
+
+switch (s->be_tpm_version) {
+case TPM_VERSION_UNSPEC:
+assert(false);
+break;
+case TPM_VERSION_1_2:
+k->dt_name = "vtpm";
+k->dt_type = "IBM,vtpm";
+k->dt_compatible = "IBM,vtpm";
+break;
+case TPM_VERSION_2_0:
+k->dt_name = "vtpm";
+k->dt_type = "IBM,vtpm";
+k->dt_compatible = "IBM,vtpm20";
+break;
Erk. Updating DeviceClass structures on the fly is hideously ugly.
We might need to take a different approach for this.
Make a suggestion... Obviously, we can hard-initialize dt_name and dt_type
but dt_compatible can only be set after we have determined the version of
TPM.
As you say name and type can just be put into the class statically.
I did this in v6.
Since you need to change compatible based on an internal variable,
we'd need to replace the static dt_compatible in the class with a
callback.
Why can we not initialize it once we know the version of TPM? From the
perspective of SLOF at least this seems to be building the device tree
fine since it sees the proper value...
Stefan
Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On Fri, Dec 13, 2019 at 08:03:36AM -0500, Stefan Berger wrote:
> On 12/13/19 12:34 AM, David Gibson wrote:
> > On Thu, Dec 12, 2019 at 03:24:26PM -0500, Stefan Berger wrote:
> > > Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
> > > as a frontend. It can use the tpm_emulator driver backend with the
> > > external
> > > swtpm.
> > >
> > > The Linux vTPM driver for ppc64 works with this emulation.
> > >
> > > This TPM emulator also handles the TPM 2 case.
> > >
> > > Signed-off-by: Stefan Berger
> > > Reviewed-by: David Gibson
> > >
> > > diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> > > index 4c8ee87d67..66a570aac1 100644
> > > --- a/hw/tpm/Kconfig
> > > +++ b/hw/tpm/Kconfig
> > > @@ -22,3 +22,9 @@ config TPM_EMULATOR
> > > bool
> > > default y
> > > depends on TPMDEV
> > > +
> > > +config TPM_SPAPR
> > > +bool
> > > +default n
> > > +select TPMDEV
> > > +depends on PSERIES
> > > diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> > > index de0b85d02a..85eb99ae05 100644
> > > --- a/hw/tpm/Makefile.objs
> > > +++ b/hw/tpm/Makefile.objs
> > > @@ -4,3 +4,4 @@ common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
> > > common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
> > > common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> > > common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
> > > +obj-$(CONFIG_TPM_SPAPR) += tpm_spapr.o
> > > diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> > > new file mode 100644
> > > index 00..c4a67e2403
> > > --- /dev/null
> > > +++ b/hw/tpm/tpm_spapr.c
> > > @@ -0,0 +1,405 @@
> > > +/*
> > > + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System
> > > Emulator
> > > + *
> > > + * PAPR Virtual TPM
> > > + *
> > > + * Copyright (c) 2015, 2017 IBM Corporation.
> > > + *
> > > + * Authors:
> > > + *Stefan Berger
> > > + *
> > > + * This code is licensed under the GPL version 2 or later. See the
> > > + * COPYING file in the top-level directory.
> > > + *
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu/error-report.h"
> > > +#include "qapi/error.h"
> > > +#include "hw/qdev-properties.h"
> > > +#include "migration/vmstate.h"
> > > +
> > > +#include "sysemu/tpm_backend.h"
> > > +#include "tpm_int.h"
> > > +#include "tpm_util.h"
> > > +
> > > +#include "hw/ppc/spapr.h"
> > > +#include "hw/ppc/spapr_vio.h"
> > > +#include "trace.h"
> > > +
> > > +#define DEBUG_SPAPR 0
> > > +
> > > +#define VIO_SPAPR_VTPM(obj) \
> > > + OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
> > > +
> > > +typedef struct VioCRQ {
> > How does this structure relate to the existing SpaprVioCrq?
>
> The existing one looks like this:
>
> typedef struct SpaprVioCrq {
> uint64_t qladdr;
> uint32_t qsize;
> uint32_t qnext;
> int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
> } SpaprVioCrq;
>
> I don't seem to find the fields there that we need for vTPM support.
Yeah, I can see the difference in the structures. What I'm after is
what is the difference in purpose which means they have different
content.
Having read through the whole series now, I *think* the answer is that
the tpm specific structure is one entry in the request queue for the
vtpm, whereas the VioCrq structure is a handle on an entire queue.
I think the tpm one needs a rename to reflect that a) it's vtpm
specific and b) it's not actually a queue, just part of it.
> > Also we're now avoiding exceptions to StudlyCaps, because it causes
> > more confusion even if it is to match other capitalization
> > conventions. So, I'd suggest 'VioCrq', 'TpmSpaprCrq' etc.
>
>
> Will adjust.
>
>
> >
> > > +uint8_t valid; /* 0x80: cmd; 0xc0: init crq */
> > > +/* 0x81-0x83: CRQ message response */
> > > +uint8_t msg;/* see below */
> > > +uint16_t len; /* len of TPM request; len of TPM response */
> > > +uint32_t data; /* rtce_dma_handle when sending TPM request */
> > > +uint64_t reserved;
> > > +} VioCRQ;
> > > +
> > > +typedef union TPMSpaprCRQ {
> > > +VioCRQ s;
> > > +uint8_t raw[sizeof(VioCRQ)];
> > > +} TPMSpaprCRQ;
> > A union just to get raw bytes seems a really weird thing to do (as
> > opposed to just casting to (char *))
>
>
> Ok, I will change it.
>
>
>
> >
> > > +
> > > +#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND 0xC0
> > > +#define SPAPR_VTPM_VALID_COMMAND 0x80
> > > +#define SPAPR_VTPM_MSG_RESULT 0x80
> > > +
> > > +/* msg types for valid = SPAPR_VTPM_VALID_INIT_CRQ */
> > > +#define SPAPR_VTPM_INIT_CRQ_RESULT 0x1
> > > +#define SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT 0x2
> > > +
> > > +/* msg types for valid = SPAPR_VTPM_VALID_CMD */
> > > +#define SPAPR_VTPM_GET_VERSION 0x1
> > > +#define SPAPR_VTPM_TPM_COMMAND 0x2
> > > +#define SPAPR_VTPM_GET_RTCE_BUFFER_SIZE 0x3
> > > +#define SPAPR_VTPM_PREPARE_TO_SUSPEND0x4
> > > +
> > > +/* response error messages
Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On 12/13/19 12:34 AM, David Gibson wrote:
On Thu, Dec 12, 2019 at 03:24:26PM -0500, Stefan Berger wrote:
Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
as a frontend. It can use the tpm_emulator driver backend with the external
swtpm.
The Linux vTPM driver for ppc64 works with this emulation.
This TPM emulator also handles the TPM 2 case.
Signed-off-by: Stefan Berger
Reviewed-by: David Gibson
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 4c8ee87d67..66a570aac1 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -22,3 +22,9 @@ config TPM_EMULATOR
bool
default y
depends on TPMDEV
+
+config TPM_SPAPR
+bool
+default n
+select TPMDEV
+depends on PSERIES
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index de0b85d02a..85eb99ae05 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -4,3 +4,4 @@ common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
+obj-$(CONFIG_TPM_SPAPR) += tpm_spapr.o
diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
new file mode 100644
index 00..c4a67e2403
--- /dev/null
+++ b/hw/tpm/tpm_spapr.c
@@ -0,0 +1,405 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * PAPR Virtual TPM
+ *
+ * Copyright (c) 2015, 2017 IBM Corporation.
+ *
+ * Authors:
+ *Stefan Berger
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+
+#include "sysemu/tpm_backend.h"
+#include "tpm_int.h"
+#include "tpm_util.h"
+
+#include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_vio.h"
+#include "trace.h"
+
+#define DEBUG_SPAPR 0
+
+#define VIO_SPAPR_VTPM(obj) \
+ OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
+
+typedef struct VioCRQ {
How does this structure relate to the existing SpaprVioCrq?
The existing one looks like this:
typedef struct SpaprVioCrq {
uint64_t qladdr;
uint32_t qsize;
uint32_t qnext;
int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
} SpaprVioCrq;
I don't seem to find the fields there that we need for vTPM support.
Also we're now avoiding exceptions to StudlyCaps, because it causes
more confusion even if it is to match other capitalization
conventions. So, I'd suggest 'VioCrq', 'TpmSpaprCrq' etc.
Will adjust.
+uint8_t valid; /* 0x80: cmd; 0xc0: init crq */
+/* 0x81-0x83: CRQ message response */
+uint8_t msg;/* see below */
+uint16_t len; /* len of TPM request; len of TPM response */
+uint32_t data; /* rtce_dma_handle when sending TPM request */
+uint64_t reserved;
+} VioCRQ;
+
+typedef union TPMSpaprCRQ {
+VioCRQ s;
+uint8_t raw[sizeof(VioCRQ)];
+} TPMSpaprCRQ;
A union just to get raw bytes seems a really weird thing to do (as
opposed to just casting to (char *))
Ok, I will change it.
+
+#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND 0xC0
+#define SPAPR_VTPM_VALID_COMMAND 0x80
+#define SPAPR_VTPM_MSG_RESULT 0x80
+
+/* msg types for valid = SPAPR_VTPM_VALID_INIT_CRQ */
+#define SPAPR_VTPM_INIT_CRQ_RESULT 0x1
+#define SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT 0x2
+
+/* msg types for valid = SPAPR_VTPM_VALID_CMD */
+#define SPAPR_VTPM_GET_VERSION 0x1
+#define SPAPR_VTPM_TPM_COMMAND 0x2
+#define SPAPR_VTPM_GET_RTCE_BUFFER_SIZE 0x3
+#define SPAPR_VTPM_PREPARE_TO_SUSPEND0x4
+
+/* response error messages */
+#define SPAPR_VTPM_VTPM_ERROR0xff
+
+/* error codes */
+#define SPAPR_VTPM_ERR_COPY_IN_FAILED0x3
+#define SPAPR_VTPM_ERR_COPY_OUT_FAILED 0x4
+
+#define MAX_BUFFER_SIZE TARGET_PAGE_SIZE
+
+typedef struct {
+SpaprVioDevice vdev;
+
+TPMSpaprCRQ crq; /* track single TPM command */
+
+uint8_t state;
+#define SPAPR_VTPM_STATE_NONE 0
+#define SPAPR_VTPM_STATE_EXECUTION1
+#define SPAPR_VTPM_STATE_COMPLETION 2
I see this field written, but never read. What's up with that?
if (s->state == SPAPR_VTPM_STATE_EXECUTION) {
return H_BUSY;
}
Is this what you mean?
+
+unsigned char buffer[MAX_BUFFER_SIZE];
+
+TPMBackendCmd cmd;
+
+TPMBackend *be_driver;
+TPMVersion be_tpm_version;
+
+size_t be_buffer_size;
+} SPAPRvTPMState;
SpaprVtpmState
Or just SpaprTpmState, since we use just "tpm spapr" rather than
"vtpm" in plenty of other places.
Will adjust.
+
+static void tpm_spapr_show_buffer(const unsigned char *buffer,
+ size_t buffer_size, const char *string)
+{
+size_t len, i;
+char *line_buffer, *p;
+
+len = MIN(tpm_cmd_get_size(buffer), buffer_size);
+
+
Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On Thu, Dec 12, 2019 at 03:24:26PM -0500, Stefan Berger wrote:
> Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
> as a frontend. It can use the tpm_emulator driver backend with the external
> swtpm.
>
> The Linux vTPM driver for ppc64 works with this emulation.
>
> This TPM emulator also handles the TPM 2 case.
>
> Signed-off-by: Stefan Berger
> Reviewed-by: David Gibson
>
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index 4c8ee87d67..66a570aac1 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -22,3 +22,9 @@ config TPM_EMULATOR
> bool
> default y
> depends on TPMDEV
> +
> +config TPM_SPAPR
> +bool
> +default n
> +select TPMDEV
> +depends on PSERIES
> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> index de0b85d02a..85eb99ae05 100644
> --- a/hw/tpm/Makefile.objs
> +++ b/hw/tpm/Makefile.objs
> @@ -4,3 +4,4 @@ common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
> common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
> common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
> +obj-$(CONFIG_TPM_SPAPR) += tpm_spapr.o
> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> new file mode 100644
> index 00..c4a67e2403
> --- /dev/null
> +++ b/hw/tpm/tpm_spapr.c
> @@ -0,0 +1,405 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System
> Emulator
> + *
> + * PAPR Virtual TPM
> + *
> + * Copyright (c) 2015, 2017 IBM Corporation.
> + *
> + * Authors:
> + *Stefan Berger
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +
> +#include "sysemu/tpm_backend.h"
> +#include "tpm_int.h"
> +#include "tpm_util.h"
> +
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_vio.h"
> +#include "trace.h"
> +
> +#define DEBUG_SPAPR 0
> +
> +#define VIO_SPAPR_VTPM(obj) \
> + OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
> +
> +typedef struct VioCRQ {
How does this structure relate to the existing SpaprVioCrq?
Also we're now avoiding exceptions to StudlyCaps, because it causes
more confusion even if it is to match other capitalization
conventions. So, I'd suggest 'VioCrq', 'TpmSpaprCrq' etc.
> +uint8_t valid; /* 0x80: cmd; 0xc0: init crq */
> +/* 0x81-0x83: CRQ message response */
> +uint8_t msg;/* see below */
> +uint16_t len; /* len of TPM request; len of TPM response */
> +uint32_t data; /* rtce_dma_handle when sending TPM request */
> +uint64_t reserved;
> +} VioCRQ;
> +
> +typedef union TPMSpaprCRQ {
> +VioCRQ s;
> +uint8_t raw[sizeof(VioCRQ)];
> +} TPMSpaprCRQ;
A union just to get raw bytes seems a really weird thing to do (as
opposed to just casting to (char *))
> +
> +#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND 0xC0
> +#define SPAPR_VTPM_VALID_COMMAND 0x80
> +#define SPAPR_VTPM_MSG_RESULT 0x80
> +
> +/* msg types for valid = SPAPR_VTPM_VALID_INIT_CRQ */
> +#define SPAPR_VTPM_INIT_CRQ_RESULT 0x1
> +#define SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT 0x2
> +
> +/* msg types for valid = SPAPR_VTPM_VALID_CMD */
> +#define SPAPR_VTPM_GET_VERSION 0x1
> +#define SPAPR_VTPM_TPM_COMMAND 0x2
> +#define SPAPR_VTPM_GET_RTCE_BUFFER_SIZE 0x3
> +#define SPAPR_VTPM_PREPARE_TO_SUSPEND0x4
> +
> +/* response error messages */
> +#define SPAPR_VTPM_VTPM_ERROR0xff
> +
> +/* error codes */
> +#define SPAPR_VTPM_ERR_COPY_IN_FAILED0x3
> +#define SPAPR_VTPM_ERR_COPY_OUT_FAILED 0x4
> +
> +#define MAX_BUFFER_SIZE TARGET_PAGE_SIZE
> +
> +typedef struct {
> +SpaprVioDevice vdev;
> +
> +TPMSpaprCRQ crq; /* track single TPM command */
> +
> +uint8_t state;
> +#define SPAPR_VTPM_STATE_NONE 0
> +#define SPAPR_VTPM_STATE_EXECUTION1
> +#define SPAPR_VTPM_STATE_COMPLETION 2
I see this field written, but never read. What's up with that?
> +
> +unsigned char buffer[MAX_BUFFER_SIZE];
> +
> +TPMBackendCmd cmd;
> +
> +TPMBackend *be_driver;
> +TPMVersion be_tpm_version;
> +
> +size_t be_buffer_size;
> +} SPAPRvTPMState;
SpaprVtpmState
Or just SpaprTpmState, since we use just "tpm spapr" rather than
"vtpm" in plenty of other places.
> +
> +static void tpm_spapr_show_buffer(const unsigned char *buffer,
> + size_t buffer_size, const char *string)
> +{
> +size_t len, i;
> +char *line_buffer, *p;
> +
> +len = MIN(tpm_cmd_get_size(buffer), buffer_size);
> +
> +/*
> + * allocate enough room for 3 chars per buffer entry plus a
> + * newline after every 16 chars and a final null terminator.
> + */
> +line_buffer = g_malloc(len * 3 + (len / 16) + 1);
You can use g_strdup_printf() /
Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On 12/12/19 3:33 PM, Eric Blake wrote: On 12/12/19 2:24 PM, Stefan Berger wrote: Implement support for TPM on ppc64 by implementing the vTPM CRQ interface as a frontend. It can use the tpm_emulator driver backend with the external swtpm. The Linux vTPM driver for ppc64 works with this emulation. This TPM emulator also handles the TPM 2 case. Signed-off-by: Stefan Berger Reviewed-by: David Gibson diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig Odd that your diff doesn't include the usual --- marker or a diffstat. +++ b/hw/tpm/tpm_spapr.c @@ -0,0 +1,405 @@ +/* + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator + * + * PAPR Virtual TPM + * + * Copyright (c) 2015, 2017 IBM Corporation. Do you want to claim 2019? :-)
Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On 12/12/19 2:24 PM, Stefan Berger wrote: Implement support for TPM on ppc64 by implementing the vTPM CRQ interface as a frontend. It can use the tpm_emulator driver backend with the external swtpm. The Linux vTPM driver for ppc64 works with this emulation. This TPM emulator also handles the TPM 2 case. Signed-off-by: Stefan Berger Reviewed-by: David Gibson diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig Odd that your diff doesn't include the usual --- marker or a diffstat. +++ b/hw/tpm/tpm_spapr.c @@ -0,0 +1,405 @@ +/* + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator + * + * PAPR Virtual TPM + * + * Copyright (c) 2015, 2017 IBM Corporation. Do you want to claim 2019? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
as a frontend. It can use the tpm_emulator driver backend with the external
swtpm.
The Linux vTPM driver for ppc64 works with this emulation.
This TPM emulator also handles the TPM 2 case.
Signed-off-by: Stefan Berger
Reviewed-by: David Gibson
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 4c8ee87d67..66a570aac1 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -22,3 +22,9 @@ config TPM_EMULATOR
bool
default y
depends on TPMDEV
+
+config TPM_SPAPR
+bool
+default n
+select TPMDEV
+depends on PSERIES
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index de0b85d02a..85eb99ae05 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -4,3 +4,4 @@ common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
+obj-$(CONFIG_TPM_SPAPR) += tpm_spapr.o
diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
new file mode 100644
index 00..c4a67e2403
--- /dev/null
+++ b/hw/tpm/tpm_spapr.c
@@ -0,0 +1,405 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * PAPR Virtual TPM
+ *
+ * Copyright (c) 2015, 2017 IBM Corporation.
+ *
+ * Authors:
+ *Stefan Berger
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+
+#include "sysemu/tpm_backend.h"
+#include "tpm_int.h"
+#include "tpm_util.h"
+
+#include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_vio.h"
+#include "trace.h"
+
+#define DEBUG_SPAPR 0
+
+#define VIO_SPAPR_VTPM(obj) \
+ OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
+
+typedef struct VioCRQ {
+uint8_t valid; /* 0x80: cmd; 0xc0: init crq */
+/* 0x81-0x83: CRQ message response */
+uint8_t msg;/* see below */
+uint16_t len; /* len of TPM request; len of TPM response */
+uint32_t data; /* rtce_dma_handle when sending TPM request */
+uint64_t reserved;
+} VioCRQ;
+
+typedef union TPMSpaprCRQ {
+VioCRQ s;
+uint8_t raw[sizeof(VioCRQ)];
+} TPMSpaprCRQ;
+
+#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND 0xC0
+#define SPAPR_VTPM_VALID_COMMAND 0x80
+#define SPAPR_VTPM_MSG_RESULT 0x80
+
+/* msg types for valid = SPAPR_VTPM_VALID_INIT_CRQ */
+#define SPAPR_VTPM_INIT_CRQ_RESULT 0x1
+#define SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT 0x2
+
+/* msg types for valid = SPAPR_VTPM_VALID_CMD */
+#define SPAPR_VTPM_GET_VERSION 0x1
+#define SPAPR_VTPM_TPM_COMMAND 0x2
+#define SPAPR_VTPM_GET_RTCE_BUFFER_SIZE 0x3
+#define SPAPR_VTPM_PREPARE_TO_SUSPEND0x4
+
+/* response error messages */
+#define SPAPR_VTPM_VTPM_ERROR0xff
+
+/* error codes */
+#define SPAPR_VTPM_ERR_COPY_IN_FAILED0x3
+#define SPAPR_VTPM_ERR_COPY_OUT_FAILED 0x4
+
+#define MAX_BUFFER_SIZE TARGET_PAGE_SIZE
+
+typedef struct {
+SpaprVioDevice vdev;
+
+TPMSpaprCRQ crq; /* track single TPM command */
+
+uint8_t state;
+#define SPAPR_VTPM_STATE_NONE 0
+#define SPAPR_VTPM_STATE_EXECUTION1
+#define SPAPR_VTPM_STATE_COMPLETION 2
+
+unsigned char buffer[MAX_BUFFER_SIZE];
+
+TPMBackendCmd cmd;
+
+TPMBackend *be_driver;
+TPMVersion be_tpm_version;
+
+size_t be_buffer_size;
+} SPAPRvTPMState;
+
+static void tpm_spapr_show_buffer(const unsigned char *buffer,
+ size_t buffer_size, const char *string)
+{
+size_t len, i;
+char *line_buffer, *p;
+
+len = MIN(tpm_cmd_get_size(buffer), buffer_size);
+
+/*
+ * allocate enough room for 3 chars per buffer entry plus a
+ * newline after every 16 chars and a final null terminator.
+ */
+line_buffer = g_malloc(len * 3 + (len / 16) + 1);
+
+for (i = 0, p = line_buffer; i < len; i++) {
+if (i && !(i % 16)) {
+p += sprintf(p, "\n");
+}
+p += sprintf(p, "%.2X ", buffer[i]);
+}
+trace_tpm_spapr_show_buffer(string, len, line_buffer);
+
+g_free(line_buffer);
+}
+
+/*
+ * Send a request to the TPM.
+ */
+static void tpm_spapr_tpm_send(SPAPRvTPMState *s)
+{
+if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
+tpm_spapr_show_buffer(s->buffer, sizeof(s->buffer), "To TPM");
+}
+
+s->state = SPAPR_VTPM_STATE_EXECUTION;
+s->cmd = (TPMBackendCmd) {
+.locty = 0,
+.in = s->buffer,
+.in_len = MIN(tpm_cmd_get_size(s->buffer), sizeof(s->buffer)),
+.out = s->buffer,
+.out_len = sizeof(s->buffer),
+};
+
+tpm_backend_deliver_request(s->be_driver, >cmd);
+}
+
+static int tpm_spapr_process_cmd(SPAPRvTPMState *s, uint64_t dataptr)
+{
+long
