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.

The change in meson.build is required to include the
cpu.h header.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>

---

v1 -> v2:
- Add tpm_crb_unrealize
---
 hw/tpm/meson.build |  2 +-
 hw/tpm/tpm_crb.c   | 22 ++++++++++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
index 1c68d81d6a..3e74df945b 100644
--- a/hw/tpm/meson.build
+++ b/hw/tpm/meson.build
@@ -1,8 +1,8 @@
 softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
-softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
 
+specific_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
 specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
 specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
 specific_ss.add(when: 'CONFIG_TPM_SPAPR', if_true: files('tpm_spapr.c'))
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 58ebd1469c..6ec19a9911 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 "cpu.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, DEVICE(s));
 
     memory_region_add_subregion(get_system_memory(),
         TPM_CRB_ADDR_BASE, &s->mmio);
@@ -309,12 +315,24 @@ 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);
+
+    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

On 18/1/22 16:33, Eric Auger 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.
> 
> The change in meson.build is required to include the
> cpu.h header.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> ---
> 
> v1 -> v2:
> - Add tpm_crb_unrealize
> ---
>   hw/tpm/meson.build |  2 +-
>   hw/tpm/tpm_crb.c   | 22 ++++++++++++++++++++--
>   2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> index 1c68d81d6a..3e74df945b 100644
> --- a/hw/tpm/meson.build
> +++ b/hw/tpm/meson.build
> @@ -1,8 +1,8 @@
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
> -softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
>   
> +specific_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))

We don't need to make this file target-specific.

>   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
>   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
>   specific_ss.add(when: 'CONFIG_TPM_SPAPR', if_true: files('tpm_spapr.c'))
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 58ebd1469c..6ec19a9911 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 "cpu.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(ç,
> +                                HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));

HOST_PAGE_ALIGN() and qemu_real_host_page_size() actually belong
to "exec/cpu-common.h".

Alex, could you hold on a few days for this patch? I am going to send
a cleanup series. Otherwise no worry, I will clean this on top too.

Thanks,

Phil.

On Wed, 19 Jan 2022 23:46:19 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 18/1/22 16:33, Eric Auger 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.
> > 
> > The change in meson.build is required to include the
> > cpu.h header.
> > 
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> > 
> > ---
> > 
> > v1 -> v2:
> > - Add tpm_crb_unrealize
> > ---
> >   hw/tpm/meson.build |  2 +-
> >   hw/tpm/tpm_crb.c   | 22 ++++++++++++++++++++--
> >   2 files changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> > index 1c68d81d6a..3e74df945b 100644
> > --- a/hw/tpm/meson.build
> > +++ b/hw/tpm/meson.build
> > @@ -1,8 +1,8 @@
> >   softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
> >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
> >   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
> > -softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> >   
> > +specific_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))  
> 
> We don't need to make this file target-specific.
> 
> >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true: files('tpm_ppi.c'))
> >   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true: files('tpm_ppi.c'))
> >   specific_ss.add(when: 'CONFIG_TPM_SPAPR', if_true: files('tpm_spapr.c'))
> > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > index 58ebd1469c..6ec19a9911 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 "cpu.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(ç,
> > +                                HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));  
> 
> HOST_PAGE_ALIGN() and qemu_real_host_page_size() actually belong
> to "exec/cpu-common.h".
> 
> Alex, could you hold on a few days for this patch? I am going to send
> a cleanup series. Otherwise no worry, I will clean this on top too.

Sure.  Thanks,

Alex