Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Eduardo Habkost]

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  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  
> >> 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
> >> >>  wrote:
> >> >> > On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:
> >> >> >>
> >> >> >> Hi
> >> >> >>
> >> >> >> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
> >> >> >>  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 
> >> >> > Signed-off-by: Stefan Berger 
> >> >> > ---
> >> >> >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   0xFED4
> >> >> >#define 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-André Lureau]

Hi

On Mon, Jan 22, 2018 at 6:47 PM, Eduardo Habkost  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  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
>> >>  wrote:
>> >> > On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:
>> >> >>
>> >> >> Hi
>> >> >>
>> >> >> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
>> >> >>  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 
>> >> > Signed-off-by: Stefan Berger 
>> >> > ---
>> >> >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   0xFED4
>> >> >#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;
>> >> > + 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Eduardo Habkost]

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  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
> >>  wrote:
> >> > On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:
> >> >>
> >> >> Hi
> >> >>
> >> >> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
> >> >>  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 
> >> > Signed-off-by: Stefan Berger 
> >> > ---
> >> >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   0xFED4
> >> >#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 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-André Lureau]

Hi

On Mon, Jan 22, 2018 at 6:25 PM, Eduardo Habkost  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
>>  wrote:
>> > On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:
>> >>
>> >> Hi
>> >>
>> >> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
>> >>  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 
>> > Signed-off-by: Stefan Berger 
>> > ---
>> >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   0xFED4
>> >#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;
>> > +   

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Eduardo Habkost]

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
>  wrote:
> > On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:
> >>
> >> Hi
> >>
> >> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
> >>  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 
> > Signed-off-by: Stefan Berger 
> > ---
> >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   0xFED4
> >#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?

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-André Lureau]

On Mon, Jan 22, 2018 at 4:47 PM, Stefan Berger
 wrote:
> On 01/22/2018 10:08 AM, Marc-Andre Lureau wrote:
>>
>> Hi
>>
>> On Sun, Jan 21, 2018 at 11:01 PM, Stefan Berger
>>  wrote:
>>>
>>> On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:

 Hi

 On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
  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 
>>> Signed-off-by: Stefan Berger 
>>> ---
>>> 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   0xFED4
>>> #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;
>>

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Stefan Berger]

On 01/22/2018 10:08 AM, Marc-Andre Lureau wrote:

Hi

On Sun, Jan 21, 2018 at 11:01 PM, Stefan Berger
 wrote:

On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:

Hi

On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
 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 
Signed-off-by: Stefan Berger 
---
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   0xFED4
#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?


That is the question. It would be a pain for users to have to choose the 
interface. Basically TIS and CRB can be implemented in a 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-Andre Lureau]

Hi

On Sun, Jan 21, 2018 at 11:01 PM, Stefan Berger
 wrote:
> On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:
>>
>> Hi
>>
>> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
>>  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 
> Signed-off-by: Stefan Berger 
> ---
>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   0xFED4
>#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:
>>>
>>>
>>> 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Philippe Mathieu-Daudé]

On 01/21/2018 02:46 AM, Stefan Berger 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 
>>> Signed-off-by: Stefan Berger 
>>> ---
>>>   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   0xFED4
>>>   #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

Thanks!

Looking at your tree, I recommend you to have a look at the
"hw/registerfields.h" API, you might find it simpler when it
comes to add bitfields #defines and checks, moreover when you
have specs available in a parsable form, since you can easily
script and auto-generate.

Alistair, thinking about it I'm not sure you already have a such
template available, but if you do we might add it in the
scripts/modules/ directory, then I can add doc 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Stefan Berger]

On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:

Hi

On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
 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 
Signed-off-by: Stefan Berger 
---
   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   0xFED4
   #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 
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:


https://github.com/stefanberger/qemu-tpm/commit/08c61bd666f82084c42621f4285bcac08fb4f713

Stefan

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-Andre Lureau]

Hi

On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
 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 
>>> Signed-off-by: Stefan Berger 
>>> ---
>>>   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   0xFED4
>>>   #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.


> Regards,
> Stefan
>
>
>
>>
>>> +} loc_sts;
>>> +uint8_t reserved2[32];
>>> +union {
>>> +uint64_t reg;
>>> +struct {
>>> +unsigned type:4;
>>> +unsigned version:4;
>>> +unsigned cap_locality:1;
>>> +  

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Stefan Berger]

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 
Signed-off-by: Stefan Berger 
---
  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   0xFED4
  #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

Regards,
Stefan





+} loc_sts;
+uint8_t reserved2[32];
+union {
+uint64_t reg;
+struct {
+unsigned type:4;
+unsigned version:4;
+unsigned cap_locality:1;
+unsigned cap_crb_idle_bypass:1;
+unsigned reserved1:1;
+unsigned cap_data_xfer_size_support:2;
+unsigned cap_fifo:1;
+unsigned cap_crb:1;
+unsigned cap_if_res:2;
+unsigned if_selector:2;
+unsigned if_selector_lock:1;
+unsigned reserved2:4;
+unsigned rid:8;
+unsigned vid:16;
+unsigned did:16;
+} bits;
+} intf_id;
+uint64_t ctrl_ext;
+
+uint32_t ctrl_req;
+union {
+uint32_t reg;
+struct {
+unsigned tpm_sts:1;
+unsigned tpm_idle:1;
+unsigned reserved:30;

Oh here you 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Philippe Mathieu-Daudé]

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 
> Signed-off-by: Stefan Berger 
> ---
>  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   0xFED4
>  #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;

> +} loc_sts;
> +uint8_t reserved2[32];
> +union {
> +uint64_t reg;
> +struct {
> +unsigned type:4;
> +unsigned version:4;
> +unsigned cap_locality:1;
> +unsigned cap_crb_idle_bypass:1;
> +unsigned reserved1:1;
> +unsigned cap_data_xfer_size_support:2;
> +unsigned cap_fifo:1;
> +unsigned cap_crb:1;
> +unsigned cap_if_res:2;
> +unsigned if_selector:2;
> +unsigned if_selector_lock:1;
> +unsigned reserved2:4;
> +unsigned rid:8;
> +unsigned vid:16;
> +unsigned did:16;
> +} bits;
> +} intf_id;
> +uint64_t ctrl_ext;
> +
> +uint32_t ctrl_req;
> +union {
> +uint32_t reg;
> +struct {
> +unsigned tpm_sts:1;
> +unsigned tpm_idle:1;
> +unsigned reserved:30;

Oh here you use 'reserved' to left align.

> +} bits;
> +} 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Eduardo Habkost]

On Fri, Jan 19, 2018 at 04:56:31PM -0500, Stefan Berger wrote:
> On 01/19/2018 01:42 PM, Eduardo Habkost wrote:
> > On Fri, Jan 19, 2018 at 12:10:03PM -0500, Stefan Berger wrote:
> > > On 01/19/2018 09: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)
> > > I removed this last sentence now. It's at the right location.
> > > 
> > > > 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.
> > > I think this has to be solved. So I remove these last 2 sentences. I'll 
> > > have
> > > to wait until that other patch series from Eduard is merged since it 
> > > doesn't
> > > start yet.
> > The series was just merged to master.  It's possible to make a
> > machine accept the new device using
> > machine_class_allow_dynamic_sysbus_dev(), now.
> 
> I saw that.
> > 
> > However, is it really necessary to make it a sysbus device?
> > Having bus-less devices was not possible in the past, but it is
> > possible today.
> > 
> What I don't like about it is the fact that I would have to use q35 if we
> only extend that machine type to allow this sysbus device. What is the
> reason that dynamic sysbus devices have to explicitly be allowed? If we
> don't need to limit this device to a certain machine type that may be more
> user friendly.

Most sysbus devices don't work unless they have additional
supporting machine code to wire them at the right addresses.
Devices that work without extra machine code (like *-iommu) are
the exception.

I think the last time I saw an explanation for this was at:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg439549.html

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Stefan Berger]

On 01/19/2018 01:42 PM, Eduardo Habkost wrote:

On Fri, Jan 19, 2018 at 12:10:03PM -0500, Stefan Berger wrote:

On 01/19/2018 09: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)

I removed this last sentence now. It's at the right location.


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.

I think this has to be solved. So I remove these last 2 sentences. I'll have
to wait until that other patch series from Eduard is merged since it doesn't
start yet.

The series was just merged to master.  It's possible to make a
machine accept the new device using
machine_class_allow_dynamic_sysbus_dev(), now.


I saw that.


However, is it really necessary to make it a sysbus device?
Having bus-less devices was not possible in the past, but it is
possible today.

What I don't like about it is the fact that I would have to use q35 if 
we only extend that machine type to allow this sysbus device. What is 
the reason that dynamic sysbus devices have to explicitly be allowed? If 
we don't need to limit this device to a certain machine type that may be 
more user friendly.

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Eduardo Habkost]

On Fri, Jan 19, 2018 at 12:10:03PM -0500, Stefan Berger wrote:
> On 01/19/2018 09: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)
> 
> I removed this last sentence now. It's at the right location.
> 
> > 
> > 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.
> 
> I think this has to be solved. So I remove these last 2 sentences. I'll have
> to wait until that other patch series from Eduard is merged since it doesn't
> start yet.

The series was just merged to master.  It's possible to make a
machine accept the new device using
machine_class_allow_dynamic_sysbus_dev(), now.

However, is it really necessary to make it a sysbus device?
Having bus-less devices was not possible in the past, but it is
possible today.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Stefan Berger]

On 01/19/2018 09: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)


I removed this last sentence now. It's at the right location.



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.


I think this has to be solved. So I remove these last 2 sentences. I'll 
have to wait until that other patch series from Eduard is merged since 
it doesn't start yet.


   Stefan

[Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-André Lureau]

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 
Signed-off-by: Stefan Berger 
---
 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   0xFED4
 #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;
+} loc_state;
+uint32_t reserved1;
+uint32_t loc_ctrl;
+union {
+uint32_t reg;
+struct {
+unsigned granted:1;
+unsigned been_seized:1;
+} bits;
+} loc_sts;
+uint8_t reserved2[32];
+union {
+uint64_t reg;
+struct {
+unsigned type:4;
+unsigned version:4;
+unsigned cap_locality:1;
+unsigned cap_crb_idle_bypass:1;
+unsigned reserved1:1;
+unsigned cap_data_xfer_size_support:2;
+unsigned cap_fifo:1;
+unsigned cap_crb:1;
+unsigned cap_if_res:2;
+unsigned if_selector:2;
+unsigned if_selector_lock:1;
+unsigned reserved2:4;
+unsigned rid:8;
+unsigned vid:16;
+unsigned did:16;
+} bits;
+} intf_id;
+uint64_t ctrl_ext;
+
+uint32_t ctrl_req;
+union {
+uint32_t reg;
+struct {
+unsigned tpm_sts:1;
+unsigned tpm_idle:1;
+unsigned reserved:30;
+} bits;
+} ctrl_sts;
+uint32_t ctrl_cancel;
+uint32_t ctrl_start;
+uint32_t ctrl_int_enable;
+uint32_t ctrl_int_sts;
+uint32_t ctrl_cmd_size;
+uint32_t ctrl_cmd_pa_low;
+uint32_t ctrl_cmd_pa_high;
+uint32_t ctrl_rsp_size;
+uint64_t ctrl_rsp_pa;
+uint8_t reserved3[0x10];
+} QEMU_PACKED;
+
+#define TPM_CRB_ADDR_BASE   0xFED4
+#define TPM_CRB_ADDR_SIZE   0x1000
+#define TPM_CRB_ADDR_CTRL \
+(TPM_CRB_ADDR_BASE + offsetof(struct crb_regs, ctrl_req))
+
 #define TPM_LOG_AREA_MINIMUM_SIZE   (64 * 1024)
 
 #define TPM_TCPA_ACPI_CLASS_CLIENT  0
@@ -30,5 +101,6 @@
 #define TPM2_ACPI_CLASS_SERVER  1
 
 #define TPM2_START_METHOD_MMIO  6