On Wed, Nov 1, 2023 at 2:25 PM Stefan Berger <stef...@linux.ibm.com> wrote:
>
>
>
> On 10/31/23 00:00, Joelle van Dyne wrote:
> > On Apple Silicon, when Windows performs a LDP on the CRB MMIO space,
> > the exception is not decoded by hardware and we cannot trap the MMIO
> > read. This led to the idea from @agraf to use the same mapping type as
> > ROM devices: namely that reads should be seen as memory type and
> > writes should trap as MMIO.
> >
> > Once that was done, the second memory mapping of the command buffer
> > region was redundent and was removed.
> >
> > A note about the removal of the read trap for `CRB_LOC_STATE`:
> > The only usage was to return the most up-to-date value for
> > `tpmEstablished`. However, `tpmEstablished` is only cleared when a
> > TPM2_HashStart operation is called which only exists for locality 4.
> > We do not handle locality 4. Indeed, the comment for the write handler
> > of `CRB_LOC_CTRL` makes the same argument for why it is not calling
> > the backend to reset the `tpmEstablished` bit (to 1).
> > As this bit is unused, we do not need to worry about updating it for
> > reads.
> >
> > In order to maintain migration compatibility with older versions of
> > QEMU, we store a copy of the register data and command data which is
> > used only during save/restore.
> >
> > Signed-off-by: Joelle van Dyne <j...@getutm.app>
> > ---
>
> > diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
> > index bee0b71fee..605e8576e9 100644
> > --- a/hw/tpm/tpm_crb_common.c
> > +++ b/hw/tpm/tpm_crb_common.c
> > @@ -31,31 +31,12 @@
> > #include "qom/object.h"
> > #include "tpm_crb.h"
> >
> > -static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
> > - unsigned size)
> > +static uint8_t tpm_crb_get_active_locty(TPMCRBState *s, uint32_t *regs)
> > {
> > - TPMCRBState *s = opaque;
> > - void *regs = (void *)&s->regs + (addr & ~3);
> > - unsigned offset = addr & 3;
> > - uint32_t val = *(uint32_t *)regs >> (8 * offset);
> > -
> > - switch (addr) {
> > - case A_CRB_LOC_STATE:
> > - val |= !tpm_backend_get_tpm_established_flag(s->tpmbe);
> > - break;
> > - }
> > -
> > - trace_tpm_crb_mmio_read(addr, size, val);
> > -
> > - return val;
> > -}
> > -
> > -static uint8_t tpm_crb_get_active_locty(TPMCRBState *s)
> > -{
> > - if (!ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, locAssigned)) {
> > + if (!ARRAY_FIELD_EX32(regs, CRB_LOC_STATE, locAssigned)) {
> > return TPM_CRB_NO_LOCALITY;
> > }
> > - return ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, activeLocality);
> > + return ARRAY_FIELD_EX32(regs, CRB_LOC_STATE, activeLocality);
> > }
> >
> > static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
> > @@ -63,35 +44,47 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr
> > addr,
> > {
> > TPMCRBState *s = opaque;
> > uint8_t locty = addr >> 12;
> > + uint32_t *regs;
> > + void *mem;
> >
> > trace_tpm_crb_mmio_write(addr, size, val);
> > + regs = memory_region_get_ram_ptr(&s->mmio);
> > + mem = ®s[R_CRB_DATA_BUFFER];
> > + assert(regs);
> > +
> > + if (addr >= A_CRB_DATA_BUFFER) {
>
>
> Can you write here /* receive TPM command bytes */ ?
Will do.
>
>
> > + assert(addr + size <= TPM_CRB_ADDR_SIZE);
> > + assert(size <= sizeof(val));
> > + memcpy(mem + addr - A_CRB_DATA_BUFFER, &val, size);
>
> > + memory_region_set_dirty(&s->mmio, addr, size);
> > + return;
> > + }
> >
> > switch (addr) {
> > case A_CRB_CTRL_REQ:
> > switch (val) {
> > case CRB_CTRL_REQ_CMD_READY:
> > - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> > + ARRAY_FIELD_DP32(regs, CRB_CTRL_STS,
> > tpmIdle, 0);
> > break;
> > case CRB_CTRL_REQ_GO_IDLE:
> > - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> > + ARRAY_FIELD_DP32(regs, CRB_CTRL_STS,
> > tpmIdle, 1);
> > break;
> > }
> > break;
> > case A_CRB_CTRL_CANCEL:
> > if (val == CRB_CANCEL_INVOKE &&
> > - s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
> > + regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
> > tpm_backend_cancel_cmd(s->tpmbe);
> > }
> > break;
> > case A_CRB_CTRL_START:
> > if (val == CRB_START_INVOKE &&
> > - !(s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) &&
> > - tpm_crb_get_active_locty(s) == locty) {
> > - void *mem = memory_region_get_ram_ptr(&s->cmdmem);
> > + !(regs[R_CRB_CTRL_START] & CRB_START_INVOKE) &&
> > + tpm_crb_get_active_locty(s, regs) == locty) {
> >
> > - s->regs[R_CRB_CTRL_START] |= CRB_START_INVOKE;
> > + regs[R_CRB_CTRL_START] |= CRB_START_INVOKE;
> > s->cmd = (TPMBackendCmd) {
> > .in = mem,
> > .in_len = MIN(tpm_cmd_get_size(mem), s->be_buffer_size),
> > @@ -108,26 +101,27 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr
> > addr,
> > /* not loc 3 or 4 */
> > break;
> > case CRB_LOC_CTRL_RELINQUISH:
> > - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> > + ARRAY_FIELD_DP32(regs, CRB_LOC_STATE,
> > locAssigned, 0);
> > - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> > + ARRAY_FIELD_DP32(regs, CRB_LOC_STS,
> > Granted, 0);
> > break;
> > case CRB_LOC_CTRL_REQUEST_ACCESS:
> > - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> > + ARRAY_FIELD_DP32(regs, CRB_LOC_STS,
> > Granted, 1);
> > - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> > + ARRAY_FIELD_DP32(regs, CRB_LOC_STS,
> > beenSeized, 0);
> > - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> > + ARRAY_FIELD_DP32(regs, CRB_LOC_STATE,
> > locAssigned, 1);
> > break;
> > }
> > break;
> > }
> > +
> > + memory_region_set_dirty(&s->mmio, 0, A_CRB_DATA_BUFFER);
> > }
> >
> > const MemoryRegionOps tpm_crb_memory_ops = {
> > - .read = tpm_crb_mmio_read,
> > .write = tpm_crb_mmio_write,
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > .valid = {
> > @@ -138,12 +132,16 @@ const MemoryRegionOps tpm_crb_memory_ops = {
> >
> > void tpm_crb_request_completed(TPMCRBState *s, int ret)
> > {
> > - s->regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
> > + uint32_t *regs = memory_region_get_ram_ptr(&s->mmio);
> > +
> > + assert(regs);
> > + regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
> > if (ret != 0) {
> > - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> > + ARRAY_FIELD_DP32(regs, CRB_CTRL_STS,
> > tpmSts, 1); /* fatal error */
> > }
> > - memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
> > +
> > + memory_region_set_dirty(&s->mmio, 0, TPM_CRB_ADDR_SIZE);
> > }
> >
> > enum TPMVersion tpm_crb_get_version(TPMCRBState *s)
> > @@ -160,45 +158,50 @@ int tpm_crb_pre_save(TPMCRBState *s)
> >
> > void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr)
> > {
> > + uint32_t *regs = memory_region_get_ram_ptr(&s->mmio);
> > +
> > + assert(regs);
> > if (s->ppi_enabled) {
> > tpm_ppi_reset(&s->ppi);
> > }
> > tpm_backend_reset(s->tpmbe);
> >
> > - memset(s->regs, 0, sizeof(s->regs));
> > + memset(regs, 0, TPM_CRB_ADDR_SIZE);
> >
> > - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> > + ARRAY_FIELD_DP32(regs, CRB_LOC_STATE,
> > tpmRegValidSts, 1);
> > - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> > + ARRAY_FIELD_DP32(regs, CRB_LOC_STATE,
> > + tpmEstablished, 1);
> > + ARRAY_FIELD_DP32(regs, CRB_CTRL_STS,
> > tpmIdle, 1);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > InterfaceVersion, CRB_INTF_VERSION_CRB);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > CapLocality, CRB_INTF_CAP_LOCALITY_0_ONLY);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > CapCRBIdleBypass, CRB_INTF_CAP_IDLE_FAST);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > CapDataXferSizeSupport, CRB_INTF_CAP_XFER_SIZE_64);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > CapFIFO, CRB_INTF_CAP_FIFO_NOT_SUPPORTED);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > CapCRB, CRB_INTF_CAP_CRB_SUPPORTED);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
> > RID, 0b0000);
> > - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2,
> > + ARRAY_FIELD_DP32(regs, CRB_INTF_ID2,
> > VID, PCI_VENDOR_ID_IBM);
> >
> > baseaddr += A_CRB_DATA_BUFFER;
> > - s->regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
> > - s->regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr;
> > - s->regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32);
> > - s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
> > - s->regs[R_CRB_CTRL_RSP_LADDR] = (uint32_t)baseaddr;
> > - s->regs[R_CRB_CTRL_RSP_HADDR] = (uint32_t)(baseaddr >> 32);
> > + regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
> > + regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr;
> > + regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32);
> > + regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
> > + regs[R_CRB_CTRL_RSP_LADDR] = (uint32_t)baseaddr;
> > + regs[R_CRB_CTRL_RSP_HADDR] = (uint32_t)(baseaddr >> 32);
> >
> > s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
> > CRB_CTRL_CMD_SIZE);
> > @@ -206,15 +209,33 @@ void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr)
> > if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) {
> > exit(1);
> > }
> > +
> > + memory_region_rom_device_set_romd(&s->mmio, true);
> > + memory_region_set_dirty(&s->mmio, 0, TPM_CRB_ADDR_SIZE);
> > }
> >
> > void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp)
> > {
> > - memory_region_init_io(&s->mmio, obj, &tpm_crb_memory_ops, s,
> > - "tpm-crb-mmio", sizeof(s->regs));
> > - memory_region_init_ram(&s->cmdmem, obj,
> > - "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> > + memory_region_init_rom_device_nomigrate(&s->mmio, obj,
> > &tpm_crb_memory_ops,
> > + s, "tpm-crb-mem", TPM_CRB_ADDR_SIZE, errp);
> > if (s->ppi_enabled) {
> > tpm_ppi_init_memory(&s->ppi, obj);
> > }
> > }
> > +
> > +void tpm_crb_mem_save(TPMCRBState *s, uint32_t *saved_regs, void
> > *saved_cmdmem)
> > +{
> > + uint32_t *regs = memory_region_get_ram_ptr(&s->mmio);
> > +
> > + memcpy(saved_regs, regs, TPM_CRB_R_MAX);
> > + memcpy(saved_cmdmem, ®s[R_CRB_DATA_BUFFER], A_CRB_DATA_BUFFER);
>
> I find it confusing that this function is here rather than in
> tpm_crb_non_pre_save().
I factored it that way to be consistent with the other callbacks in
-common. The idea is that the -common code has no visibility into the
instance specific data structure and therefore needs to get that info
(saved_regs, saved_cmdmem) from the concrete instance.
>
> The size should be CRB_CTRL_CMD_SIZE.
Good catch, will fix.
>
> > +}
> > +
> > +void tpm_crb_mem_load(TPMCRBState *s, const uint32_t *saved_regs,
> > + const void *saved_cmdmem)
> > +{
> > + uint32_t *regs = memory_region_get_ram_ptr(&s->mmio);
> > +
> > + memcpy(regs, saved_regs, TPM_CRB_R_MAX);
> > + memcpy(®s[R_CRB_DATA_BUFFER], saved_cmdmem, A_CRB_DATA_BUFFER);
> > +}
>
> Same comments; size seems wrong.
