| Message ID | 20230321053001.3886666-3-ninad@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for TPM devices over I2C bus | expand |
On 3/21/23 01:30, Ninad Palsule wrote: > Qemu already supports devices attached to ISA and sysbus. This drop adds > support for the I2C bus attached TPM devices. > > This commit includes changes for the common code. > - Added support for the new checksum registers which are required for > the I2C support. The checksum calculation is handled in the qemu > common code. > - Added wrapper function for read and write data so that I2C code can > call it without MMIO interface. > > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com> > --- > hw/tpm/tpm_tis.h | 2 ++ > hw/tpm/tpm_tis_common.c | 33 +++++++++++++++++++++++++++++++++ > include/hw/acpi/tpm.h | 2 ++ > 3 files changed, 37 insertions(+) > > diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h > index f6b5872ba6..16b7baddd8 100644 > --- a/hw/tpm/tpm_tis.h > +++ b/hw/tpm/tpm_tis.h > @@ -86,5 +86,7 @@ int tpm_tis_pre_save(TPMState *s); > void tpm_tis_reset(TPMState *s); > enum TPMVersion tpm_tis_get_tpm_version(TPMState *s); > void tpm_tis_request_completed(TPMState *s, int ret); > +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size); > +void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size); > > #endif /* TPM_TPM_TIS_H */ > diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c > index 503be2a541..3c82f63179 100644 > --- a/hw/tpm/tpm_tis_common.c > +++ b/hw/tpm/tpm_tis_common.c > @@ -26,6 +26,8 @@ > #include "hw/irq.h" > #include "hw/isa/isa.h" > #include "qapi/error.h" > +#include "qemu/bswap.h" > +#include "qemu/crc-ccitt.h" > #include "qemu/module.h" > > #include "hw/acpi/tpm.h" > @@ -422,6 +424,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, > shift = 0; /* no more adjustments */ > } > break; > + case TPM_TIS_REG_DATA_CSUM_GET: > + val = bswap16(crc_ccitt(0, s->buffer, s->rw_offset)); Should this not rather be cpu_to_be16() so that it would also work on a big endian host (assuming you tested this on a little e endian host)? > + break; > case TPM_TIS_REG_INTERFACE_ID: > val = s->loc[locty].iface_id; > break; > @@ -447,6 +452,15 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, > return val; > } > > +/* > + * A wrapper read function so that it can be directly called without > + * mmio. > + */ > +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size) > +{ > + return tpm_tis_mmio_read(s, addr, size); > +} > + > /* > * Write a value to a register of the TIS interface > * See specs pages 33-63 for description of the registers > @@ -600,6 +614,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, > case TPM_TIS_REG_INT_VECTOR: > /* hard wired -- ignore */ > break; > + case TPM_TIS_REG_DATA_CSUM_ENABLE: > + /* > + * Checksum implemented by common code so no need to set > + * any flags. > + */ > + break; > + case TPM_TIS_REG_DATA_CSUM_GET: > + /* This is readonly register so ignore */ > + break; > case TPM_TIS_REG_INT_STATUS: > if (s->active_locty != locty) { > break; > @@ -703,6 +726,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, > break; > case TPM_TIS_REG_DATA_FIFO: > case TPM_TIS_REG_DATA_XFIFO ... TPM_TIS_REG_DATA_XFIFO_END: > + you can remove this one > /* data fifo */ > if (s->active_locty != locty) { > break; > @@ -767,6 +791,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, > } > } > > +/* > + * A wrapper write function so that it can be directly called without > + * mmio. > + */ > +void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size) > +{ > + tpm_tis_mmio_write(s, addr, val, size); > +}' > + > const MemoryRegionOps tpm_tis_memory_ops = { > .read = tpm_tis_mmio_read, > .write = tpm_tis_mmio_write, > diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h > index 559ba6906c..db12c002f4 100644 > --- a/include/hw/acpi/tpm.h > +++ b/include/hw/acpi/tpm.h > @@ -40,6 +40,8 @@ > #define TPM_TIS_REG_STS 0x18 > #define TPM_TIS_REG_DATA_FIFO 0x24 > #define TPM_TIS_REG_INTERFACE_ID 0x30 > +#define TPM_TIS_REG_DATA_CSUM_ENABLE 0x40 > +#define TPM_TIS_REG_DATA_CSUM_GET 0x44 > #define TPM_TIS_REG_DATA_XFIFO 0x80 > #define TPM_TIS_REG_DATA_XFIFO_END 0xbc > #define TPM_TIS_REG_DID_VID 0xf00 Looks good.
On 3/21/23 6:54 PM, Stefan Berger wrote: > > > On 3/21/23 01:30, Ninad Palsule wrote: >> Qemu already supports devices attached to ISA and sysbus. This drop adds >> support for the I2C bus attached TPM devices. >> >> This commit includes changes for the common code. >> - Added support for the new checksum registers which are required for >> the I2C support. The checksum calculation is handled in the qemu >> common code. >> - Added wrapper function for read and write data so that I2C code can >> call it without MMIO interface. >> >> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com> >> --- >> hw/tpm/tpm_tis.h | 2 ++ >> hw/tpm/tpm_tis_common.c | 33 +++++++++++++++++++++++++++++++++ >> include/hw/acpi/tpm.h | 2 ++ >> 3 files changed, 37 insertions(+) >> >> diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h >> index f6b5872ba6..16b7baddd8 100644 >> --- a/hw/tpm/tpm_tis.h >> +++ b/hw/tpm/tpm_tis.h >> @@ -86,5 +86,7 @@ int tpm_tis_pre_save(TPMState *s); >> void tpm_tis_reset(TPMState *s); >> enum TPMVersion tpm_tis_get_tpm_version(TPMState *s); >> void tpm_tis_request_completed(TPMState *s, int ret); >> +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size); >> +void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, >> uint32_t size); >> #endif /* TPM_TPM_TIS_H */ >> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c >> index 503be2a541..3c82f63179 100644 >> --- a/hw/tpm/tpm_tis_common.c >> +++ b/hw/tpm/tpm_tis_common.c >> @@ -26,6 +26,8 @@ >> #include "hw/irq.h" >> #include "hw/isa/isa.h" >> #include "qapi/error.h" >> +#include "qemu/bswap.h" >> +#include "qemu/crc-ccitt.h" >> #include "qemu/module.h" >> #include "hw/acpi/tpm.h" >> @@ -422,6 +424,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, >> hwaddr addr, >> shift = 0; /* no more adjustments */ >> } >> break; >> + case TPM_TIS_REG_DATA_CSUM_GET: >> + val = bswap16(crc_ccitt(0, s->buffer, s->rw_offset)); > > Should this not rather be cpu_to_be16() so that it would also work on > a big endian host (assuming you tested this on a little e endian host)? Changed code to use cpu_to_be16. Yes, I did not run on big endian host. > >> + break; >> case TPM_TIS_REG_INTERFACE_ID: >> val = s->loc[locty].iface_id; >> break; >> @@ -447,6 +452,15 @@ static uint64_t tpm_tis_mmio_read(void *opaque, >> hwaddr addr, >> return val; >> } >> +/* >> + * A wrapper read function so that it can be directly called without >> + * mmio. >> + */ >> +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size) >> +{ >> + return tpm_tis_mmio_read(s, addr, size); >> +} >> + >> /* >> * Write a value to a register of the TIS interface >> * See specs pages 33-63 for description of the registers >> @@ -600,6 +614,15 @@ static void tpm_tis_mmio_write(void *opaque, >> hwaddr addr, >> case TPM_TIS_REG_INT_VECTOR: >> /* hard wired -- ignore */ >> break; >> + case TPM_TIS_REG_DATA_CSUM_ENABLE: >> + /* >> + * Checksum implemented by common code so no need to set >> + * any flags. >> + */ >> + break; >> + case TPM_TIS_REG_DATA_CSUM_GET: >> + /* This is readonly register so ignore */ >> + break; >> case TPM_TIS_REG_INT_STATUS: >> if (s->active_locty != locty) { >> break; >> @@ -703,6 +726,7 @@ static void tpm_tis_mmio_write(void *opaque, >> hwaddr addr, >> break; >> case TPM_TIS_REG_DATA_FIFO: >> case TPM_TIS_REG_DATA_XFIFO ... TPM_TIS_REG_DATA_XFIFO_END: >> + > > you can remove this one Sorry, I am not clear what you are asking me to remove. > >> /* data fifo */ >> if (s->active_locty != locty) { >> break; >> @@ -767,6 +791,15 @@ static void tpm_tis_mmio_write(void *opaque, >> hwaddr addr, >> } >> } >> +/* >> + * A wrapper write function so that it can be directly called without >> + * mmio. >> + */ >> +void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, >> uint32_t size) >> +{ >> + tpm_tis_mmio_write(s, addr, val, size); >> +}' >> + >> const MemoryRegionOps tpm_tis_memory_ops = { >> .read = tpm_tis_mmio_read, >> .write = tpm_tis_mmio_write, >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h >> index 559ba6906c..db12c002f4 100644 >> --- a/include/hw/acpi/tpm.h >> +++ b/include/hw/acpi/tpm.h >> @@ -40,6 +40,8 @@ >> #define TPM_TIS_REG_STS 0x18 >> #define TPM_TIS_REG_DATA_FIFO 0x24 >> #define TPM_TIS_REG_INTERFACE_ID 0x30 >> +#define TPM_TIS_REG_DATA_CSUM_ENABLE 0x40 >> +#define TPM_TIS_REG_DATA_CSUM_GET 0x44 >> #define TPM_TIS_REG_DATA_XFIFO 0x80 >> #define TPM_TIS_REG_DATA_XFIFO_END 0xbc >> #define TPM_TIS_REG_DID_VID 0xf00 > > Looks good. Thank you for the review! Ninad Palsule
On 3/22/23 07:18, Ninad Palsule wrote: > > On 3/21/23 6:54 PM, Stefan Berger wrote: >>> @@ -703,6 +726,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, >>> break; >>> case TPM_TIS_REG_DATA_FIFO: >>> case TPM_TIS_REG_DATA_XFIFO ... TPM_TIS_REG_DATA_XFIFO_END: >>> + >> >> you can remove this one > Sorry, I am not clear what you are asking me to remove. You added an empty line here. Stefan
On 3/21/23 01:30, Ninad Palsule wrote: > Qemu already supports devices attached to ISA and sysbus. This drop adds > support for the I2C bus attached TPM devices. > @@ -447,6 +452,15 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, > return val; > } > > +/* > + * A wrapper read function so that it can be directly called without > + * mmio. > + */ > +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size) > +{ > + return tpm_tis_mmio_read(s, addr, size); > +} > + > /* > * Write a value to a register of the TIS interface > * See specs pages 33-63 for description of the registers > @@ -600,6 +614,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, > case TPM_TIS_REG_INT_VECTOR: > /* hard wired -- ignore */ > break; > + case TPM_TIS_REG_DATA_CSUM_ENABLE: > + /* > + * Checksum implemented by common code so no need to set > + * any flags. > + */ Can you intercept handling this register on the I2C layer and add a byte for its value so that it can be set correctly? We do want to be able to write bit 0 to it to enable it and allow reading of bit 0 to see what the state is. I don't want this byte of state on the TIS layer since this creates state incompatibilities. And for getting the checksum value it should be also handled on the I2C layer and ask tpm_tis_common.c to run crc_ccitt(0, s->buffer, s->rw_offset) via a function call. Stefan
On 3/22/23 6:24 AM, Stefan Berger wrote: > > > On 3/22/23 07:18, Ninad Palsule wrote: >> >> On 3/21/23 6:54 PM, Stefan Berger wrote: > > >>>> @@ -703,6 +726,7 @@ static void tpm_tis_mmio_write(void *opaque, >>>> hwaddr addr, >>>> break; >>>> case TPM_TIS_REG_DATA_FIFO: >>>> case TPM_TIS_REG_DATA_XFIFO ... TPM_TIS_REG_DATA_XFIFO_END: >>>> + >>> >>> you can remove this one >> Sorry, I am not clear what you are asking me to remove. > > You added an empty line here. > > Stefan Ah, Fixed it. Thank you for the review. Ninad Palsule
On 3/22/23 7:05 AM, Stefan Berger wrote: > > > On 3/21/23 01:30, Ninad Palsule wrote: >> Qemu already supports devices attached to ISA and sysbus. This drop adds >> support for the I2C bus attached TPM devices. > >> @@ -447,6 +452,15 @@ static uint64_t tpm_tis_mmio_read(void *opaque, >> hwaddr addr, >> return val; >> } >> +/* >> + * A wrapper read function so that it can be directly called without >> + * mmio. >> + */ >> +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size) >> +{ >> + return tpm_tis_mmio_read(s, addr, size); >> +} >> + >> /* >> * Write a value to a register of the TIS interface >> * See specs pages 33-63 for description of the registers >> @@ -600,6 +614,15 @@ static void tpm_tis_mmio_write(void *opaque, >> hwaddr addr, >> case TPM_TIS_REG_INT_VECTOR: >> /* hard wired -- ignore */ >> break; >> + case TPM_TIS_REG_DATA_CSUM_ENABLE: >> + /* >> + * Checksum implemented by common code so no need to set >> + * any flags. >> + */ > > Can you intercept handling this register on the I2C layer and add a > byte for its value so that it can be set correctly? We do want to be > able to write bit 0 to it to enable it and allow reading of bit 0 to > see what the state is. I don't want this byte of state on the TIS > layer since this creates state incompatibilities. > > And for getting the checksum value it should be also handled on the > I2C layer and ask tpm_tis_common.c to run crc_ccitt(0, s->buffer, > s->rw_offset) via a function call. > > Stefan Sure, Fix it by handling it in I2C and removed newly defined registers from TIS layer. Thanks for the review. Ninad Palsule
diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h index f6b5872ba6..16b7baddd8 100644 --- a/hw/tpm/tpm_tis.h +++ b/hw/tpm/tpm_tis.h @@ -86,5 +86,7 @@ int tpm_tis_pre_save(TPMState *s); void tpm_tis_reset(TPMState *s); enum TPMVersion tpm_tis_get_tpm_version(TPMState *s); void tpm_tis_request_completed(TPMState *s, int ret); +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size); +void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size); #endif /* TPM_TPM_TIS_H */ diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c index 503be2a541..3c82f63179 100644 --- a/hw/tpm/tpm_tis_common.c +++ b/hw/tpm/tpm_tis_common.c @@ -26,6 +26,8 @@ #include "hw/irq.h" #include "hw/isa/isa.h" #include "qapi/error.h" +#include "qemu/bswap.h" +#include "qemu/crc-ccitt.h" #include "qemu/module.h" #include "hw/acpi/tpm.h" @@ -422,6 +424,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, shift = 0; /* no more adjustments */ } break; + case TPM_TIS_REG_DATA_CSUM_GET: + val = bswap16(crc_ccitt(0, s->buffer, s->rw_offset)); + break; case TPM_TIS_REG_INTERFACE_ID: val = s->loc[locty].iface_id; break; @@ -447,6 +452,15 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, return val; } +/* + * A wrapper read function so that it can be directly called without + * mmio. + */ +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size) +{ + return tpm_tis_mmio_read(s, addr, size); +} + /* * Write a value to a register of the TIS interface * See specs pages 33-63 for description of the registers @@ -600,6 +614,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, case TPM_TIS_REG_INT_VECTOR: /* hard wired -- ignore */ break; + case TPM_TIS_REG_DATA_CSUM_ENABLE: + /* + * Checksum implemented by common code so no need to set + * any flags. + */ + break; + case TPM_TIS_REG_DATA_CSUM_GET: + /* This is readonly register so ignore */ + break; case TPM_TIS_REG_INT_STATUS: if (s->active_locty != locty) { break; @@ -703,6 +726,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, break; case TPM_TIS_REG_DATA_FIFO: case TPM_TIS_REG_DATA_XFIFO ... TPM_TIS_REG_DATA_XFIFO_END: + /* data fifo */ if (s->active_locty != locty) { break; @@ -767,6 +791,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, } } +/* + * A wrapper write function so that it can be directly called without + * mmio. + */ +void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size) +{ + tpm_tis_mmio_write(s, addr, val, size); +} + const MemoryRegionOps tpm_tis_memory_ops = { .read = tpm_tis_mmio_read, .write = tpm_tis_mmio_write, diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h index 559ba6906c..db12c002f4 100644 --- a/include/hw/acpi/tpm.h +++ b/include/hw/acpi/tpm.h @@ -40,6 +40,8 @@ #define TPM_TIS_REG_STS 0x18 #define TPM_TIS_REG_DATA_FIFO 0x24 #define TPM_TIS_REG_INTERFACE_ID 0x30 +#define TPM_TIS_REG_DATA_CSUM_ENABLE 0x40 +#define TPM_TIS_REG_DATA_CSUM_GET 0x44 #define TPM_TIS_REG_DATA_XFIFO 0x80 #define TPM_TIS_REG_DATA_XFIFO_END 0xbc #define TPM_TIS_REG_DID_VID 0xf00
Qemu already supports devices attached to ISA and sysbus. This drop adds support for the I2C bus attached TPM devices. This commit includes changes for the common code. - Added support for the new checksum registers which are required for the I2C support. The checksum calculation is handled in the qemu common code. - Added wrapper function for read and write data so that I2C code can call it without MMIO interface. Signed-off-by: Ninad Palsule <ninad@linux.ibm.com> --- hw/tpm/tpm_tis.h | 2 ++ hw/tpm/tpm_tis_common.c | 33 +++++++++++++++++++++++++++++++++ include/hw/acpi/tpm.h | 2 ++ 3 files changed, 37 insertions(+)