[PULL,1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region

Commit Message

Alex Williamson Feb. 3, 2022, 10:35 p.m. UTC

From: Eric Auger <[e-mail]>

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.

Signed-off-by: Eric Auger <[e-mail]>
Tested-by: Stefan Berger <[e-mail]>
Acked-by: Stefan Berger <[e-mail]>
[PMD: Keep tpm_crb.c in meson's softmmu_ss]
Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]
Signed-off-by: Alex Williamson <[e-mail]>
---
 hw/tpm/tpm_crb.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Comments

Igor Mammedov Feb. 4, 2022, 12:08 p.m. UTC | #1

On Thu, 03 Feb 2022 15:35:35 -0700
Alex Williamson <[e-mail]> wrote:

> From: Eric Auger <[e-mail]>
> 
> 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.
> 
> Signed-off-by: Eric Auger <[e-mail]>
> Tested-by: Stefan Berger <[e-mail]>
> Acked-by: Stefan Berger <[e-mail]>
> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]
> Signed-off-by: Alex Williamson <[e-mail]>
> ---
>  hw/tpm/tpm_crb.c |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 58ebd1469c35..be0884ea6031 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, DEVICE(s));

Does it need a compat knob for the case of migrating to older QEMU/machine type,
not to end-up with target aborting migration when it sees unknown section.


>      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);
> +

likewise, should vmstate be unregistered here, before freeing
actually happens?

> +    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;
> 
> 
>

Eric Auger Feb. 7, 2022, 9:23 a.m. UTC | #2

Hi Igor,
On 2/4/22 1:08 PM, Igor Mammedov wrote:
> On Thu, 03 Feb 2022 15:35:35 -0700
> Alex Williamson <[e-mail]> wrote:
>
>> From: Eric Auger <[e-mail]>
>>
>> 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.
>>
>> Signed-off-by: Eric Auger <[e-mail]>
>> Tested-by: Stefan Berger <[e-mail]>
>> Acked-by: Stefan Berger <[e-mail]>
>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>> Link: https://lore.kernel.org/r/[e-mail]
>> Signed-off-by: Alex Williamson <[e-mail]>
>> ---
>>  hw/tpm/tpm_crb.c |   22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> index 58ebd1469c35..be0884ea6031 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, DEVICE(s));
> Does it need a compat knob for the case of migrating to older QEMU/machine type,
> not to end-up with target aborting migration when it sees unknown section.
Hum I did not think about this. I need to double check.

Thank you for the review.

Eric
>
>
>>      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);
>> +
> likewise, should vmstate be unregistered here, before freeing
> actually happens?
>
>> +    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;
>>
>>
>>

Eric Auger Feb. 7, 2022, 1:42 p.m. UTC | #3

Hi Igor,

On 2/4/22 1:08 PM, Igor Mammedov wrote:
> On Thu, 03 Feb 2022 15:35:35 -0700
> Alex Williamson <[e-mail]> wrote:
>
>> From: Eric Auger <[e-mail]>
>>
>> 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.
>>
>> Signed-off-by: Eric Auger <[e-mail]>
>> Tested-by: Stefan Berger <[e-mail]>
>> Acked-by: Stefan Berger <[e-mail]>
>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>> Link: https://lore.kernel.org/r/[e-mail]
>> Signed-off-by: Alex Williamson <[e-mail]>
>> ---
>>  hw/tpm/tpm_crb.c |   22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> index 58ebd1469c35..be0884ea6031 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, DEVICE(s));
> Does it need a compat knob for the case of migrating to older QEMU/machine type,
> not to end-up with target aborting migration when it sees unknown section.

It does not seem to be requested. I am able to migrate between this
version and qemu 6.2, back and forth, using a pc-q35-6.2 machine type.
My guess is, as the amount of RAM that is migrated is the same, it does
not complain. Adding Dave and Juan though.

Thanks

Eric
>
>
>>      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);
>> +
> likewise, should vmstate be unregistered here, before freeing
> actually happens?
>
>> +    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;
>>
>>
>>

Dr. David Alan Gilbert Feb. 23, 2022, 1:02 p.m. UTC | #4

* Eric Auger ([e-mail]) wrote:
> Hi Igor,
> 
> On 2/4/22 1:08 PM, Igor Mammedov wrote:
> > On Thu, 03 Feb 2022 15:35:35 -0700
> > Alex Williamson <[e-mail]> wrote:
> >
> >> From: Eric Auger <[e-mail]>
> >>
> >> 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.
> >>
> >> Signed-off-by: Eric Auger <[e-mail]>
> >> Tested-by: Stefan Berger <[e-mail]>
> >> Acked-by: Stefan Berger <[e-mail]>
> >> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
> >> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> >> Link: https://lore.kernel.org/r/[e-mail]
> >> Signed-off-by: Alex Williamson <[e-mail]>
> >> ---
> >>  hw/tpm/tpm_crb.c |   22 ++++++++++++++++++++--
> >>  1 file changed, 20 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> >> index 58ebd1469c35..be0884ea6031 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, DEVICE(s));
> > Does it need a compat knob for the case of migrating to older QEMU/machine type,
> > not to end-up with target aborting migration when it sees unknown section.
> 
> It does not seem to be requested. I am able to migrate between this
> version and qemu 6.2, back and forth, using a pc-q35-6.2 machine type.
> My guess is, as the amount of RAM that is migrated is the same, it does
> not complain. Adding Dave and Juan though.

I think that should be OK; we just rely on the RAM Block name and size.

Dave

> Thanks
> 
> Eric
> >
> >
> >>      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);
> >> +
> > likewise, should vmstate be unregistered here, before freeing
> > actually happens?
> >
> >> +    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;
> >>
> >>
> >>
>

Patch

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 58ebd1469c35..be0884ea6031 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, 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;