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

Eric Auger posted 2 patches 4 months, 1 week ago
Maintainers: Alex Williamson <alex.williamson@redhat.com>, Stefan Berger <stefanb@linux.vnet.ibm.com>
[PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
Posted by Eric Auger 4 months, 1 week ago
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>
---
 hw/tpm/meson.build |  2 +-
 hw/tpm/tpm_crb.c   | 10 ++++++++--
 2 files changed, 9 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..25f8e685e4 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);
-- 
2.26.3


Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
Posted by Stefan Berger 4 months, 1 week ago
On 1/13/22 05:37, 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>
> ---
>   hw/tpm/meson.build |  2 +-
>   hw/tpm/tpm_crb.c   | 10 ++++++++--
>   2 files changed, 9 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..25f8e685e4 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));
> +

Do we need an unrealize function now to qemu_vfree() this memory?


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

Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
Posted by Eric Auger 4 months, 1 week ago
Hi Stefan,

On 1/13/22 3:06 PM, Stefan Berger wrote:
>
> On 1/13/22 05:37, 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>
>> ---
>>   hw/tpm/meson.build |  2 +-
>>   hw/tpm/tpm_crb.c   | 10 ++++++++--
>>   2 files changed, 9 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..25f8e685e4 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));
>> +
>
> Do we need an unrealize function now to qemu_vfree() this memory?
I would say it is needed if the device can be hot-unplugged.
tpmppi->buf is not freeed either.

Thanks

Eric

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


Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
Posted by Stefan Berger 4 months, 1 week ago
On 1/13/22 09:40, Eric Auger wrote:

> Hi Stefan,
>
> On 1/13/22 3:06 PM, Stefan Berger wrote:
>> On 1/13/22 05:37, 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>
>>> ---
>>>    hw/tpm/meson.build |  2 +-
>>>    hw/tpm/tpm_crb.c   | 10 ++++++++--
>>>    2 files changed, 9 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..25f8e685e4 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));
>>> +
>> Do we need an unrealize function now to qemu_vfree() this memory?
> I would say it is needed if the device can be hot-unplugged.
> tpmppi->buf is not freeed either.


Correct about PPI. My main concern would be the CRB related test cases 
that likely currently run without PPI but now could complain about a 
memory leak upon shutdown. I tried to compile with --enable-sanitizers 
and run the tests but it doesn't compile when the sanitizers are enabled.


FAILED: libcommon.fa.p/disas_i386.c.o
cc -m64 -mcx16 -Ilibcommon.fa.p -I../capstone/include/capstone 
-I../dtc/libfdt -I../slirp -I../slirp/src -I/usr/include/pixman-1 
-I/usr/include/p11-kit-1 -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 
-I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 
-fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g 
-isystem /home/stefanb/dev/qemu/linux-headers -isystem linux-headers 
-iquote . -iquote /home/stefanb/dev/qemu -iquote 
/home/stefanb/dev/qemu/include -iquote 
/home/stefanb/dev/qemu/disas/libvixl -iquote 
/home/stefanb/dev/qemu/tcg/i386 -pthread -fsanitize=undefined 
-fsanitize=address -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k 
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs 
-Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 
-Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi 
-fstack-protector-strong -fPIE -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 
-DNCURSES_WIDECHAR=1 -DSTRUCT_IOVEC_DEFINED -MD -MQ 
libcommon.fa.p/disas_i386.c.o -MF libcommon.fa.p/disas_i386.c.o.d -o 
libcommon.fa.p/disas_i386.c.o -c ../disas/i386.c
In file included from /usr/include/string.h:519,
                  from /home/stefanb/dev/qemu/include/qemu/osdep.h:87,
                  from ../disas/i386.c:34:
In function ?strcpy?,
     inlined from ?PNI_Fixup? at ../disas/i386.c:6434:4,
     inlined from ?PNI_Fixup? at ../disas/i386.c:6400:1:
/usr/include/bits/string_fortified.h:79:10: error: ?__builtin_memcpy? 
offset [0, 7] is out of the bounds [0, 0] [-Werror=array-bounds]
    79 |   return __builtin___strcpy_chk (__dest, __src, __glibc_objsize 
(__dest));
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ?strcpy?,
     inlined from ?PNI_Fixup? at ../disas/i386.c:6427:4,
     inlined from ?PNI_Fixup? at ../disas/i386.c:6400:1:
/usr/include/bits/string_fortified.h:79:10: error: ?__builtin_memcpy? 
offset [0, 5] is out of the bounds [0, 0] [-Werror=array-bounds]
    79 |   return __builtin___strcpy_chk (__dest, __src, __glibc_objsize 
(__dest));
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

    Stefan

>
> Thanks
>
> Eric
>
>>
>>>        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);

Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
Posted by Eric Auger 4 months, 1 week ago
Hi Stefan,

On 1/13/22 4:38 PM, Stefan Berger wrote:
> On 1/13/22 09:40, Eric Auger wrote:
>
>> Hi Stefan,
>>
>> On 1/13/22 3:06 PM, Stefan Berger wrote:
>>> On 1/13/22 05:37, 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>
>>>> ---
>>>>    hw/tpm/meson.build |  2 +-
>>>>    hw/tpm/tpm_crb.c   | 10 ++++++++--
>>>>    2 files changed, 9 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..25f8e685e4 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));
>>>> +
>>> Do we need an unrealize function now to qemu_vfree() this memory?
>> I would say it is needed if the device can be hot-unplugged.
>> tpmppi->buf is not freeed either.
>
>
> Correct about PPI. My main concern would be the CRB related test cases
> that likely currently run without PPI but now could complain about a
> memory leak upon shutdown. I tried to compile with --enable-sanitizers
> and run the tests but it doesn't compile when the sanitizers are enabled.
>
>
> FAILED: libcommon.fa.p/disas_i386.c.o
> cc -m64 -mcx16 -Ilibcommon.fa.p -I../capstone/include/capstone
> -I../dtc/libfdt -I../slirp -I../slirp/src -I/usr/include/pixman-1
> -I/usr/include/p11-kit-1 -I/usr/include/glib-2.0
> -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4
> -I/usr/include/libmount -I/usr/include/blkid
> -I/usr/include/gio-unix-2.0 -fdiagnostics-color=auto -Wall
> -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem
> /home/stefanb/dev/qemu/linux-headers -isystem linux-headers -iquote .
> -iquote /home/stefanb/dev/qemu -iquote /home/stefanb/dev/qemu/include
> -iquote /home/stefanb/dev/qemu/disas/libvixl -iquote
> /home/stefanb/dev/qemu/tcg/i386 -pthread -fsanitize=undefined
> -fsanitize=address -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
> -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes
> -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration
> -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
> -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
> -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
> -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi
> -fstack-protector-strong -fPIE -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600
> -DNCURSES_WIDECHAR=1 -DSTRUCT_IOVEC_DEFINED -MD -MQ
> libcommon.fa.p/disas_i386.c.o -MF libcommon.fa.p/disas_i386.c.o.d -o
> libcommon.fa.p/disas_i386.c.o -c ../disas/i386.c
> In file included from /usr/include/string.h:519,
>                  from /home/stefanb/dev/qemu/include/qemu/osdep.h:87,
>                  from ../disas/i386.c:34:
> In function ?strcpy?,
>     inlined from ?PNI_Fixup? at ../disas/i386.c:6434:4,
>     inlined from ?PNI_Fixup? at ../disas/i386.c:6400:1:
> /usr/include/bits/string_fortified.h:79:10: error: ?__builtin_memcpy?
> offset [0, 7] is out of the bounds [0, 0] [-Werror=array-bounds]
>    79 |   return __builtin___strcpy_chk (__dest, __src,
> __glibc_objsize (__dest));
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In function ?strcpy?,
>     inlined from ?PNI_Fixup? at ../disas/i386.c:6427:4,
>     inlined from ?PNI_Fixup? at ../disas/i386.c:6400:1:
> /usr/include/bits/string_fortified.h:79:10: error: ?__builtin_memcpy?
> offset [0, 5] is out of the bounds [0, 0] [-Werror=array-bounds]
>    79 |   return __builtin___strcpy_chk (__dest, __src,
> __glibc_objsize (__dest));
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors

thank you for the tests and above trial. Anyway I can add the unrealize
and deal with the deallocation of both buffers

Eric
>
>    Stefan
>
>>
>> Thanks
>>
>> Eric
>>
>>>
>>>>        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);
>


Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
Posted by Stefan Berger 4 months, 1 week ago
On 1/13/22 05:37, 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>
Suspend/resume tested it:

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


> ---
>   hw/tpm/meson.build |  2 +-
>   hw/tpm/tpm_crb.c   | 10 ++++++++--
>   2 files changed, 9 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..25f8e685e4 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);