Re: [PATCH 03/11] tpm_ppi: refactor memory space initialization

[Stefan Berger <stefanb@linux.ibm.com>]

On 7/12/23 23:51, Joelle van Dyne wrote:
> Instead of calling `memory_region_add_subregion` directly, we defer to
> the caller to do it. This allows us to re-use the code for a SysBus
> device.
> 
> Signed-off-by: Joelle van Dyne <j@getutm.app>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   hw/tpm/tpm_ppi.h        | 10 +++-------
>   hw/tpm/tpm_crb.c        |  4 ++--
>   hw/tpm/tpm_crb_common.c |  3 +++
>   hw/tpm/tpm_ppi.c        |  5 +----
>   hw/tpm/tpm_tis_isa.c    |  5 +++--
>   5 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> index bf5d4a300f..30863c6438 100644
> --- a/hw/tpm/tpm_ppi.h
> +++ b/hw/tpm/tpm_ppi.h
> @@ -20,17 +20,13 @@ typedef struct TPMPPI {
>   } TPMPPI;
> 
>   /**
> - * tpm_ppi_init:
> + * tpm_ppi_init_memory:
>    * @tpmppi: a TPMPPI
> - * @m: the address-space / MemoryRegion to use
> - * @addr: the address of the PPI region
>    * @obj: the owner object
>    *
> - * Register the TPM PPI memory region at @addr on the given address
> - * space for the object @obj.
> + * Creates the TPM PPI memory region.
>    **/
> -void tpm_ppi_init(TPMPPI *tpmppi, MemoryRegion *m,
> -                  hwaddr addr, Object *obj);
> +void tpm_ppi_init_memory(TPMPPI *tpmppi, Object *obj);
> 
>   /**
>    * tpm_ppi_reset:
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 3ef4977fb5..598c3e0161 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -107,8 +107,8 @@ static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
>           TPM_CRB_ADDR_BASE + sizeof(s->state.regs), &s->state.cmdmem);
> 
>       if (s->state.ppi_enabled) {
> -        tpm_ppi_init(&s->state.ppi, get_system_memory(),
> -                     TPM_PPI_ADDR_BASE, OBJECT(s));
> +        memory_region_add_subregion(get_system_memory(),
> +            TPM_PPI_ADDR_BASE, &s->state.ppi.ram);
>       }
> 
>       if (xen_enabled()) {
> diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
> index 228e2d0faf..e56e910670 100644
> --- a/hw/tpm/tpm_crb_common.c
> +++ b/hw/tpm/tpm_crb_common.c
> @@ -216,4 +216,7 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp)
>           "tpm-crb-mmio", sizeof(s->regs));
>       memory_region_init_ram(&s->cmdmem, obj,
>           "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> +    if (s->ppi_enabled) {
> +        tpm_ppi_init_memory(&s->ppi, obj);
> +    }
>   }
> diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> index 7f74e26ec6..40cab59afa 100644
> --- a/hw/tpm/tpm_ppi.c
> +++ b/hw/tpm/tpm_ppi.c
> @@ -44,14 +44,11 @@ void tpm_ppi_reset(TPMPPI *tpmppi)
>       }
>   }
> 
> -void tpm_ppi_init(TPMPPI *tpmppi, MemoryRegion *m,
> -                  hwaddr addr, Object *obj)
> +void tpm_ppi_init_memory(TPMPPI *tpmppi, Object *obj)
>   {
>       tpmppi->buf = qemu_memalign(qemu_real_host_page_size(),
>                                   HOST_PAGE_ALIGN(TPM_PPI_ADDR_SIZE));
>       memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
>                                         TPM_PPI_ADDR_SIZE, tpmppi->buf);
>       vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
> -
> -    memory_region_add_subregion(m, addr, &tpmppi->ram);
>   }
> diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
> index 91e3792248..7cd7415f30 100644
> --- a/hw/tpm/tpm_tis_isa.c
> +++ b/hw/tpm/tpm_tis_isa.c
> @@ -134,8 +134,9 @@ static void tpm_tis_isa_realizefn(DeviceState *dev, Error **errp)
>                                   TPM_TIS_ADDR_BASE, &s->mmio);
> 
>       if (s->ppi_enabled) {
> -        tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
> -                     TPM_PPI_ADDR_BASE, OBJECT(dev));
> +        tpm_ppi_init_memory(&s->ppi, OBJECT(dev));
> +        memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
> +                                    TPM_PPI_ADDR_BASE, &s->ppi.ram);
>       }
>   }
>