[PATCH v3 0/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region

[PATCH v3 0/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region

[Philippe Mathieu-Daudé via]

This is a respin of Eric's work, but not making tpm_crb.c target
specific.

Based-on: <20220120000836.229419-1-f4bug@amsat.org>
"exec/cpu: Make host pages variables / macros 'target agnostic'"
https://lore.kernel.org/qemu-devel/20220120000836.229419-1-f4bug@amsat.org/

--

Eric's v2 cover:

This series aims at removing a spurious error message we get when
launching a guest with a TPM-CRB device and VFIO-PCI devices.

The CRB command buffer currently is a RAM MemoryRegion and given
its base address alignment, it causes an error report on
vfio_listener_region_add(). This series proposes to use a ram-device
region instead which helps in better assessing the dma map error
failure on VFIO side.

Eric Auger (2):
  tpm: CRB: Use ram_device for "tpm-crb-cmd" region
  hw/vfio/common: Silence ram device offset alignment error traces

 hw/tpm/tpm_crb.c     | 22 ++++++++++++++++++++--
 hw/vfio/common.c     | 15 ++++++++++++++-
 hw/vfio/trace-events |  1 +
 3 files changed, 35 insertions(+), 3 deletions(-)

-- 
2.34.1

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

[Philippe Mathieu-Daudé via]

From: Eric Auger <eric.auger@redhat.com>

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 <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>
---
 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 58ebd1469c3..be0884ea603 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;
-- 
2.34.1

[PATCH v3 2/2] hw/vfio/common: Silence ram device offset alignment error traces

[Philippe Mathieu-Daudé via]

From: Eric Auger <eric.auger@redhat.com>

Failing to DMA MAP a ram_device should not cause an error message.
This is currently happening with the TPM CRB command region and
this is causing confusion.

We may want to keep the trace for debug purpose though.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Acked-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/vfio/common.c     | 15 ++++++++++++++-
 hw/vfio/trace-events |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 080046e3f51..9caa560b078 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -884,7 +884,20 @@ static void vfio_listener_region_add(MemoryListener *listener,
     if (unlikely((section->offset_within_address_space &
                   ~qemu_real_host_page_mask) !=
                  (section->offset_within_region & ~qemu_real_host_page_mask))) {
-        error_report("%s received unaligned region", __func__);
+        if (memory_region_is_ram_device(section->mr)) { /* just debug purpose */
+            trace_vfio_listener_region_add_bad_offset_alignment(
+                memory_region_name(section->mr),
+                section->offset_within_address_space,
+                section->offset_within_region, qemu_real_host_page_size);
+        } else { /* error case we don't want to be fatal */
+            error_report("%s received unaligned region %s iova=0x%"PRIx64
+                         " offset_within_region=0x%"PRIx64
+                         " qemu_real_host_page_mask=0x%"PRIx64,
+                         __func__, memory_region_name(section->mr),
+                         section->offset_within_address_space,
+                         section->offset_within_region,
+                         qemu_real_host_page_mask);
+        }
         return;
     }
 
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index 0ef1b5f4a65..ccd9d7610d6 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -100,6 +100,7 @@ vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add
 vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d"
 vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add [iommu] 0x%"PRIx64" - 0x%"PRIx64
 vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]"
+vfio_listener_region_add_bad_offset_alignment(const char *name, uint64_t iova, uint64_t offset_within_region, uint64_t page_size) "Region \"%s\" @0x%"PRIx64", offset_within_region=0x%"PRIx64", qemu_real_host_page_mask=0x%"PRIx64 " cannot be mapped for DMA"
 vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA"
 vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
 vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64
-- 
2.34.1

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

[Eric Auger]

Hi Philippe,

On 1/20/22 1:12 AM, Philippe Mathieu-Daudé wrote:
> From: Eric Auger <eric.auger@redhat.com>
>
> 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 <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>
> ---
>  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 58ebd1469c3..be0884ea603 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"
OK thank you for the change. That's definitively better to keep

tpm_crb.c in meson's softmmu_ss


Thanks

Eric
>  #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;

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

[Alex Williamson]

On Thu, 20 Jan 2022 01:12:40 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> This is a respin of Eric's work, but not making tpm_crb.c target
> specific.
> 
> Based-on: <20220120000836.229419-1-f4bug@amsat.org>
> "exec/cpu: Make host pages variables / macros 'target agnostic'"
> https://lore.kernel.org/qemu-devel/20220120000836.229419-1-f4bug@amsat.org/
> 
> --
> 
> Eric's v2 cover:
> 
> This series aims at removing a spurious error message we get when
> launching a guest with a TPM-CRB device and VFIO-PCI devices.
> 
> The CRB command buffer currently is a RAM MemoryRegion and given
> its base address alignment, it causes an error report on
> vfio_listener_region_add(). This series proposes to use a ram-device
> region instead which helps in better assessing the dma map error
> failure on VFIO side.
> 
> Eric Auger (2):
>   tpm: CRB: Use ram_device for "tpm-crb-cmd" region
>   hw/vfio/common: Silence ram device offset alignment error traces
> 
>  hw/tpm/tpm_crb.c     | 22 ++++++++++++++++++++--
>  hw/vfio/common.c     | 15 ++++++++++++++-
>  hw/vfio/trace-events |  1 +
>  3 files changed, 35 insertions(+), 3 deletions(-)
> 

Unfortunately, FTB:

../hw/tpm/tpm_crb.c: In function ‘tpm_crb_realize’:
../hw/tpm/tpm_crb.c:297:33: warning: implicit declaration of function ‘HOST_PAGE_ALIGN’ [-Wimplicit-function-declaration]
  297 |                                 HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
      |                                 ^~~~~~~~~~~~~~~
../hw/tpm/tpm_crb.c:297:33: warning: nested extern declaration of ‘HOST_PAGE_ALIGN’ [-Wnested-externs]

This is a regression since Eric's v2.  Thanks,

Alex

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

[Philippe Mathieu-Daudé via]

Hi Alex,

On 27/1/22 00:51, Alex Williamson wrote:
> On Thu, 20 Jan 2022 01:12:40 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> This is a respin of Eric's work, but not making tpm_crb.c target
>> specific.
>>
>> Based-on: <20220120000836.229419-1-f4bug@amsat.org>
>> "exec/cpu: Make host pages variables / macros 'target agnostic'"
>> https://lore.kernel.org/qemu-devel/20220120000836.229419-1-f4bug@amsat.org/

[*]

>> --
>>
>> Eric's v2 cover:
>>
>> This series aims at removing a spurious error message we get when
>> launching a guest with a TPM-CRB device and VFIO-PCI devices.
>>
>> The CRB command buffer currently is a RAM MemoryRegion and given
>> its base address alignment, it causes an error report on
>> vfio_listener_region_add(). This series proposes to use a ram-device
>> region instead which helps in better assessing the dma map error
>> failure on VFIO side.
>>
>> Eric Auger (2):
>>    tpm: CRB: Use ram_device for "tpm-crb-cmd" region
>>    hw/vfio/common: Silence ram device offset alignment error traces
>>
>>   hw/tpm/tpm_crb.c     | 22 ++++++++++++++++++++--
>>   hw/vfio/common.c     | 15 ++++++++++++++-
>>   hw/vfio/trace-events |  1 +
>>   3 files changed, 35 insertions(+), 3 deletions(-)
>>
> 
> Unfortunately, FTB:
> 
> ../hw/tpm/tpm_crb.c: In function ‘tpm_crb_realize’:
> ../hw/tpm/tpm_crb.c:297:33: warning: implicit declaration of function ‘HOST_PAGE_ALIGN’ [-Wimplicit-function-declaration]
>    297 |                                 HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
>        |                                 ^~~~~~~~~~~~~~~
> ../hw/tpm/tpm_crb.c:297:33: warning: nested extern declaration of ‘HOST_PAGE_ALIGN’ [-Wnested-externs]
> 
> This is a regression since Eric's v2.  Thanks,

This series is based on another patch that Paolo already queued
(see [*] earlier).

Next time I'll try to make it more explicit.

Regards,

Phil.

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

[Alex Williamson]

On Thu, 27 Jan 2022 08:51:15 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Hi Alex,
> 
> On 27/1/22 00:51, Alex Williamson wrote:
> > On Thu, 20 Jan 2022 01:12:40 +0100
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> This is a respin of Eric's work, but not making tpm_crb.c target
> >> specific.
> >>
> >> Based-on: <20220120000836.229419-1-f4bug@amsat.org>
> >> "exec/cpu: Make host pages variables / macros 'target agnostic'"
> >> https://lore.kernel.org/qemu-devel/20220120000836.229419-1-f4bug@amsat.org/  
> 
> [*]
> 
> >> --
> >>
> >> Eric's v2 cover:
> >>
> >> This series aims at removing a spurious error message we get when
> >> launching a guest with a TPM-CRB device and VFIO-PCI devices.
> >>
> >> The CRB command buffer currently is a RAM MemoryRegion and given
> >> its base address alignment, it causes an error report on
> >> vfio_listener_region_add(). This series proposes to use a ram-device
> >> region instead which helps in better assessing the dma map error
> >> failure on VFIO side.
> >>
> >> Eric Auger (2):
> >>    tpm: CRB: Use ram_device for "tpm-crb-cmd" region
> >>    hw/vfio/common: Silence ram device offset alignment error traces
> >>
> >>   hw/tpm/tpm_crb.c     | 22 ++++++++++++++++++++--
> >>   hw/vfio/common.c     | 15 ++++++++++++++-
> >>   hw/vfio/trace-events |  1 +
> >>   3 files changed, 35 insertions(+), 3 deletions(-)
> >>  
> > 
> > Unfortunately, FTB:
> > 
> > ../hw/tpm/tpm_crb.c: In function ‘tpm_crb_realize’:
> > ../hw/tpm/tpm_crb.c:297:33: warning: implicit declaration of function ‘HOST_PAGE_ALIGN’ [-Wimplicit-function-declaration]
> >    297 |                                 HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
> >        |                                 ^~~~~~~~~~~~~~~
> > ../hw/tpm/tpm_crb.c:297:33: warning: nested extern declaration of ‘HOST_PAGE_ALIGN’ [-Wnested-externs]
> > 
> > This is a regression since Eric's v2.  Thanks,  
> 
> This series is based on another patch that Paolo already queued
> (see [*] earlier).
> 
> Next time I'll try to make it more explicit.

Sorry I missed that.  Since Paolo now has that pending in a pull
request I'll try again next week.  Thanks,

Alex

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

[Eric Auger]

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