Re: [PATCH v4 04/14] tpm_crb: use a single read-as-mem/write-as-mmio mapping

[Joelle van Dyne]

On Wed, Nov 1, 2023 at 2:25 PM Stefan Berger <stefanb@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 = &regs[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, &regs[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(&regs[R_CRB_DATA_BUFFER], saved_cmdmem, A_CRB_DATA_BUFFER);
> > +}
>
> Same comments; size seems wrong.