On Mon, Jan 22, 2018 at 07:15:01PM +0100, Marc-André Lureau wrote: > Hi > > On Mon, Jan 22, 2018 at 6:47 PM, Eduardo Habkost <ehabk...@redhat.com> wrote: > > On Mon, Jan 22, 2018 at 06:32:37PM +0100, Marc-André Lureau wrote: > >> Hi > >> > >> On Mon, Jan 22, 2018 at 6:25 PM, Eduardo Habkost <ehabk...@redhat.com> > >> wrote: > >> > On Mon, Jan 22, 2018 at 04:08:30PM +0100, Marc-Andre Lureau wrote: > >> >> Hi > >> >> > >> >> On Sun, Jan 21, 2018 at 11:01 PM, Stefan Berger > >> >> <stef...@linux.vnet.ibm.com> wrote: > >> >> > On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote: > >> >> >> > >> >> >> Hi > >> >> >> > >> >> >> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger > >> >> >> <stef...@linux.vnet.ibm.com> wrote: > >> >> >>> > >> >> >>> On 01/20/2018 07:54 AM, Philippe Mathieu-Daudé wrote: > >> >> >>>> > >> >> >>>> On 01/19/2018 11:11 AM, Marc-André Lureau wrote: > >> >> >>>>> > >> >> >>>>> tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB) > >> >> >>>>> Interface as defined in TCG PC Client Platform TPM Profile (PTP) > >> >> >>>>> Specification Family “2.0” Level 00 Revision 01.03 v22. > >> >> >>>>> > >> >> >>>>> The PTP allows device implementation to switch between TIS and CRB > >> >> >>>>> model at run time, but given that CRB is a simpler device to > >> >> >>>>> implement, I chose to implement it as a different device. > >> >> >>>>> > >> >> >>>>> The device doesn't implement other locality than 0 for now (my > >> >> >>>>> laptop > >> >> >>>>> TPM doesn't either, so I assume this isn't so bad) > >> >> >>>>> > >> >> >>>>> The command/reply memory region is statically allocated after the > >> >> >>>>> CRB > >> >> >>>>> registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I > >> >> >>>>> wonder if the BIOS could or should allocate it instead, or what > >> >> >>>>> size > >> >> >>>>> to use, again this seems to fit well expectations) > >> >> >>>>> > >> >> >>>>> The PTP doesn't specify a particular bus to put the device. So I > >> >> >>>>> added > >> >> >>>>> it on the system bus directly, so it could hopefully be used > >> >> >>>>> easily on > >> >> >>>>> a different platform than x86. Currently, it fails to init on > >> >> >>>>> piix, > >> >> >>>>> because error_on_sysbus_device() check. The check may be changed > >> >> >>>>> in a > >> >> >>>>> near future, see discussion on the qemu-devel ML. > >> >> >>>>> > >> >> >>>>> Tested with some success with Linux upstream and Windows 10, > >> >> >>>>> seabios & > >> >> >>>>> modified ovmf. The device is recognized and correctly transmit > >> >> >>>>> command/response with passthrough & emu. However, we are missing > >> >> >>>>> PPI > >> >> >>>>> ACPI part atm. > >> >> >>>>> > >> >> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > >> >> >>>>> Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com> > >> >> >>>>> --- > >> >> >>>>> qapi/tpm.json | 5 +- > >> >> >>>>> include/hw/acpi/tpm.h | 72 ++++++++ > >> >> >>>>> include/sysemu/tpm.h | 3 + > >> >> >>>>> hw/i386/acpi-build.c | 34 +++- > >> >> >>>>> hw/tpm/tpm_crb.c | 327 > >> >> >>>>> +++++++++++++++++++++++++++++++++++++ > >> >> >>>>> default-configs/i386-softmmu.mak | 1 + > >> >> >>>>> default-configs/x86_64-softmmu.mak | 1 + > >> >> >>>>> hw/tpm/Makefile.objs | 1 + > >> >> >>>>> 8 files changed, 434 insertions(+), 10 deletions(-) > >> >> >>>>> create mode 100644 hw/tpm/tpm_crb.c > >> >> >>>>> > >> >> >>>>> diff --git a/qapi/tpm.json b/qapi/tpm.json > >> >> >>>>> index 7093f268fb..d50deef5e9 100644 > >> >> >>>>> --- a/qapi/tpm.json > >> >> >>>>> +++ b/qapi/tpm.json > >> >> >>>>> @@ -11,10 +11,11 @@ > >> >> >>>>> # An enumeration of TPM models > >> >> >>>>> # > >> >> >>>>> # @tpm-tis: TPM TIS model > >> >> >>>>> +# @tpm-crb: TPM CRB model (since 2.12) > >> >> >>>>> # > >> >> >>>>> # Since: 1.5 > >> >> >>>>> ## > >> >> >>>>> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis' ] } > >> >> >>>>> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] } > >> >> >>>>> ## > >> >> >>>>> # @query-tpm-models: > >> >> >>>>> @@ -28,7 +29,7 @@ > >> >> >>>>> # Example: > >> >> >>>>> # > >> >> >>>>> # -> { "execute": "query-tpm-models" } > >> >> >>>>> -# <- { "return": [ "tpm-tis" ] } > >> >> >>>>> +# <- { "return": [ "tpm-tis", "tpm-crb" ] } > >> >> >>>>> # > >> >> >>>>> ## > >> >> >>>>> { 'command': 'query-tpm-models', 'returns': ['TpmModel'] } > >> >> >>>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h > >> >> >>>>> index 6d516c6a7f..b0048515fa 100644 > >> >> >>>>> --- a/include/hw/acpi/tpm.h > >> >> >>>>> +++ b/include/hw/acpi/tpm.h > >> >> >>>>> @@ -16,11 +16,82 @@ > >> >> >>>>> #ifndef HW_ACPI_TPM_H > >> >> >>>>> #define HW_ACPI_TPM_H > >> >> >>>>> +#include "qemu/osdep.h" > >> >> >>>>> + > >> >> >>>>> #define TPM_TIS_ADDR_BASE 0xFED40000 > >> >> >>>>> #define TPM_TIS_ADDR_SIZE 0x5000 > >> >> >>>>> #define TPM_TIS_IRQ 5 > >> >> >>>>> +struct crb_regs { > >> >> >>>>> + union { > >> >> >>>>> + uint32_t reg; > >> >> >>>>> + struct { > >> >> >>>>> + unsigned tpm_established:1; > >> >> >>>>> + unsigned loc_assigned:1; > >> >> >>>>> + unsigned active_locality:3; > >> >> >>>>> + unsigned reserved:2; > >> >> >>>>> + unsigned tpm_reg_valid_sts:1; > >> >> >>>>> + } bits; > >> >> >>>> > >> >> >>>> I suppose this is little-endian layout, so this won't work on > >> >> >>>> big-endian > >> >> >>>> hosts. > >> >> >>>> > >> >> >>>> Can you add a qtest? > >> >> >>>> > >> >> >>>>> + } loc_state; > >> >> >>>>> + uint32_t reserved1; > >> >> >>>>> + uint32_t loc_ctrl; > >> >> >>>>> + union { > >> >> >>>>> + uint32_t reg; > >> >> >>>>> + struct { > >> >> >>>>> + unsigned granted:1; > >> >> >>>>> + unsigned been_seized:1; > >> >> >>>>> + } bits; > >> >> >>>> > >> >> >>>> > >> >> >>>> This is unclear where you expect those bits (right/left aligned). > >> >> >>>> > >> >> >>>> Can you add an unnamed field to be more explicit? > >> >> >>>> > >> >> >>>> i.e. without using struct if left alignment expected: > >> >> >>>> > >> >> >>>> unsigned granted:1, been_seized:1, :30; > >> >> >>> > >> >> >>> > >> >> >>> > >> >> >>> I got rid of all the bitfields and this patch here makes it work on > >> >> >>> a > >> >> >>> ppc64 > >> >> >>> big endian and also x86_64 host: > >> >> >>> > >> >> >>> > >> >> >>> https://github.com/stefanberger/qemu-tpm/commit/28fc07f0d9314168986190effd6d72d9fd3972dd > >> >> >>> > >> >> >> Thank you Stefan! I am all for squashing this fix to the patch. You > >> >> >> should then add your signed-off to the commit. > >> >> > > >> >> > > >> >> > I'll do that. > >> >> > > >> >> > The TIS is an ISA Device and the CRB is similar. Considering the > >> >> > >> >> How much similarity is there between TIS and CRB is there? The two > >> >> devices look quite different to me, CRB is way simpler it seems. Or is > >> >> the CRB implementation just lacking many bells and whistles that TIS > >> >> has? Should we consider merging CRB in TIS? > >> >> > >> >> > complications with the sysbus devices where one has to explicitly > >> >> > allow it > >> >> > for a certain machine type, I would advocate to convert the CRB to an > >> >> > ISA > >> >> > device. A patch that does that is this one: > >> >> > >> >> If it's only for that reason (an explicit enable), I would rather keep > >> >> it on the system bus. Or should it be on an LPC bus? > >> >> > >> >> Eduardo, what do you think? > >> > > >> > Everything about sysbus is exceptional and confusing, so I would > >> > prefer to avoid using sysbus every time we have an alternative. > >> > If tpm-tis is already an ISA device, what are the reasons to not > >> > use ISA for tpm-crb too? > >> > >> I was hoping this would make the device more portable (especially on > >> arm) and avoid using legacy bus or resources. > >> > >> Putting it on ISA doesn't reflect better what a real hardware is like, > >> or does it? > > > > I don't know what the hardware interface for those devices look > > like, the documentation I found for TPM-CRB seem to be purely for > > the software interface. > > > > It has a MMIO region at 0xFED4_xxxx, and it can be on LPC or SPI bus > as far as I understand (8.2.2): > > https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_Specification_Family_2.0_Revision_1.3v22.pdf
Thanks. It looks like the actual hardware interface doesn't matter for QEMU and for guest software, though? > > > Is there a reason to make it a sysbus device instead of a > > bus-less device, then? > > The calls to > sysbus_init_mmio(sbd, &s->mmio); > sysbus_mmio_map(sbd, 0, TPM_CRB_ADDR_BASE); > > Could perhaps be replaced by lower-level memory_region calls? > > I'll give it a try If the code ends up uglier, then this sounds like a good reason to use sysbus, at least until we make the sysbus I/O APIs available to the rest of devices. I would document this in a comment above the ".parent = TYPE_SYSBUS_DEVICE" line, though. -- Eduardo