From: Stefan Berger <stefanb@linux.ibm.com>
To: Joelle van Dyne <j@getutm.app>, qemu-devel@nongnu.org
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Subject: Re: [PATCH v4 04/14] tpm_crb: use a single read-as-mem/write-as-mmio mapping
Date: Wed, 1 Nov 2023 17:25:41 -0400 [thread overview]
Message-ID: <e7a4c833-3e61-4a7f-ab9e-3921eb581613@linux.ibm.com> (raw)
In-Reply-To: <20231031040021.65582-5-j@getutm.app>
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 */ ?
> + 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().
The size should be CRB_CTRL_CMD_SIZE.
> +}
> +
> +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.
next prev parent reply other threads:[~2023-11-01 21:27 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-31 4:00 [PATCH v4 00/14] tpm: introduce TPM CRB SysBus device Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 01/14] tpm_crb: refactor common code Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 02/14] tpm_crb: CTRL_RSP_ADDR is 64-bits wide Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 03/14] tpm_ppi: refactor memory space initialization Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 04/14] tpm_crb: use a single read-as-mem/write-as-mmio mapping Joelle van Dyne
2023-11-01 21:25 ` Stefan Berger [this message]
2023-11-14 1:37 ` Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 05/14] tpm_crb: move ACPI table building to device interface Joelle van Dyne
2023-11-02 18:50 ` Stefan Berger
2023-11-03 2:37 ` Joelle van Dyne
2023-11-03 12:43 ` Stefan Berger
2023-10-31 4:00 ` [PATCH v4 06/14] tpm-sysbus: add plug handler for TPM on SysBus Joelle van Dyne
2023-10-31 13:19 ` Stefan Berger
2023-10-31 4:00 ` [PATCH v4 07/14] hw/arm/virt: connect TPM to platform bus Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 08/14] hw/loongarch/virt: " Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 09/14] tpm_tis_sysbus: move DSDT AML generation to device Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 10/14] tests: acpi: prepare for TPM CRB tests Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 11/14] tpm_crb_sysbus: introduce TPM CRB SysBus device Joelle van Dyne
2023-10-31 13:25 ` Stefan Berger
2023-10-31 4:00 ` [PATCH v4 12/14] tests: acpi: implement TPM CRB tests for ARM virt Joelle van Dyne
2023-10-31 4:00 ` [PATCH v4 13/14] tests: acpi: updated expected blobs for TPM CRB Joelle van Dyne
2023-10-31 12:43 ` Stefan Berger
2023-10-31 4:00 ` [PATCH v4 14/14] tests: add TPM-CRB sysbus tests for aarch64 Joelle van Dyne
2023-10-31 13:05 ` Stefan Berger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e7a4c833-3e61-4a7f-ab9e-3921eb581613@linux.ibm.com \
--to=stefanb@linux.ibm.com \
--cc=j@getutm.app \
--cc=qemu-devel@nongnu.org \
--cc=stefanb@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).