Re: [PATCH v4 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region

[Dr. David Alan Gilbert]

* Eric Auger (eric.auger@redhat.com) wrote:
> Representing the CRB cmd/response buffer as a standard
> RAM region causes some trouble when the device is used
> with VFIO. Indeed VFIO attempts to DMA_MAP this region
> as usual RAM but this latter does not have a valid page
> size alignment causing such an error report:
> "vfio_listener_region_add received unaligned region".
> To allow VFIO to detect that failing dma mapping
> this region is not an issue, let's use a ram_device
> memory region type instead.

Don't the weird sizes and alignments also cause problems with the RAM
migration code?
Why don't you just align this up to a convenient boundary?

Dave

> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Stefan Berger <stefanb@linux.ibm.com>
> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> ---
> 
> v3 -> v4:
> -  call vmstate_unregister_ram
> ---
>  hw/tpm/tpm_crb.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 58ebd1469c3..668f969b409 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -25,6 +25,7 @@
>  #include "sysemu/tpm_backend.h"
>  #include "sysemu/tpm_util.h"
>  #include "sysemu/reset.h"
> +#include "exec/cpu-common.h"
>  #include "tpm_prop.h"
>  #include "tpm_ppi.h"
>  #include "trace.h"
> @@ -43,6 +44,7 @@ struct CRBState {
>  
>      bool ppi_enabled;
>      TPMPPI ppi;
> +    uint8_t *crb_cmd_buf;
>  };
>  typedef struct CRBState CRBState;
>  
> @@ -291,10 +293,14 @@ static void tpm_crb_realize(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>  
> +    s->crb_cmd_buf = qemu_memalign(qemu_real_host_page_size,
> +                                HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
> +
>      memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s,
>          "tpm-crb-mmio", sizeof(s->regs));
> -    memory_region_init_ram(&s->cmdmem, OBJECT(s),
> -        "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> +    memory_region_init_ram_device_ptr(&s->cmdmem, OBJECT(s), "tpm-crb-cmd",
> +                                      CRB_CTRL_CMD_SIZE, s->crb_cmd_buf);
> +    vmstate_register_ram(&s->cmdmem, dev);
>  
>      memory_region_add_subregion(get_system_memory(),
>          TPM_CRB_ADDR_BASE, &s->mmio);
> @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error 
> **errp)
>      qemu_register_reset(tpm_crb_reset, dev);
>  }
>  
> +static void tpm_crb_unrealize(DeviceState *dev)
> +{
> +    CRBState *s = CRB(dev);
> +
> +    vmstate_unregister_ram(&s->cmdmem, dev);
> +    qemu_vfree(s->crb_cmd_buf);
> +
> +    if (s->ppi_enabled) {
> +        qemu_vfree(s->ppi.buf);
> +    }
> +}
> +
>  static void tpm_crb_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      TPMIfClass *tc = TPM_IF_CLASS(klass);
>  
>      dc->realize = tpm_crb_realize;
> +    dc->unrealize = tpm_crb_unrealize;
>      device_class_set_props(dc, tpm_crb_properties);
>      dc->vmsd  = &vmstate_tpm_crb;
>      dc->user_creatable = true;
> -- 
> 2.26.3
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK