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.
Is there a reason to make it a sysbus device instead of a
bus-less device, then?
--
Eduardo
