* [PATCH 0/2] TPM-CRB: Remove spurious error report when used with VFIO @ 2022-01-13 10:37 Eric Auger 2022-01-13 10:37 ` [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region Eric Auger 2022-01-13 10:37 ` [PATCH 2/2] hw/vfio/common: Silence ram device offset alignment error traces Eric Auger 0 siblings, 2 replies; 9+ messages in thread From: Eric Auger @ 2022-01-13 10:37 UTC (permalink / raw) To: eric.auger.pro, eric.auger, stefanb, qemu-devel, alex.williamson Cc: cohuck, david 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 severity on VFIO side. Best Regards Eric This series can be found at: https://github.com/eauger/qemu/tree/tpm-crb-ram-device-v1 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/meson.build | 2 +- hw/tpm/tpm_crb.c | 10 ++++++++-- hw/vfio/common.c | 15 ++++++++++++++- hw/vfio/trace-events | 1 + 4 files changed, 24 insertions(+), 4 deletions(-) -- 2.26.3 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region 2022-01-13 10:37 [PATCH 0/2] TPM-CRB: Remove spurious error report when used with VFIO Eric Auger @ 2022-01-13 10:37 ` Eric Auger 2022-01-13 14:06 ` Stefan Berger 2022-01-13 16:20 ` Stefan Berger 2022-01-13 10:37 ` [PATCH 2/2] hw/vfio/common: Silence ram device offset alignment error traces Eric Auger 1 sibling, 2 replies; 9+ messages in thread From: Eric Auger @ 2022-01-13 10:37 UTC (permalink / raw) To: eric.auger.pro, eric.auger, stefanb, qemu-devel, alex.williamson Cc: cohuck, david 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region 2022-01-13 10:37 ` [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region Eric Auger @ 2022-01-13 14:06 ` Stefan Berger 2022-01-13 14:40 ` Eric Auger 2022-01-13 16:20 ` Stefan Berger 1 sibling, 1 reply; 9+ messages in thread From: Stefan Berger @ 2022-01-13 14:06 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, stefanb, qemu-devel, alex.williamson, Marc-André Lureau Cc: cohuck, david 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region 2022-01-13 14:06 ` Stefan Berger @ 2022-01-13 14:40 ` Eric Auger 2022-01-13 15:38 ` Stefan Berger 0 siblings, 1 reply; 9+ messages in thread From: Eric Auger @ 2022-01-13 14:40 UTC (permalink / raw) To: Stefan Berger, eric.auger.pro, stefanb, qemu-devel, alex.williamson, Marc-André Lureau Cc: cohuck, david 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); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region 2022-01-13 14:40 ` Eric Auger @ 2022-01-13 15:38 ` Stefan Berger 2022-01-14 8:33 ` Eric Auger 0 siblings, 1 reply; 9+ messages in thread From: Stefan Berger @ 2022-01-13 15:38 UTC (permalink / raw) To: eric.auger, eric.auger.pro, stefanb, qemu-devel, alex.williamson, Marc-André Lureau Cc: cohuck, david 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region 2022-01-13 15:38 ` Stefan Berger @ 2022-01-14 8:33 ` Eric Auger 0 siblings, 0 replies; 9+ messages in thread From: Eric Auger @ 2022-01-14 8:33 UTC (permalink / raw) To: Stefan Berger, eric.auger.pro, stefanb, qemu-devel, alex.williamson, Marc-André Lureau Cc: cohuck, david 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); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region 2022-01-13 10:37 ` [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region Eric Auger 2022-01-13 14:06 ` Stefan Berger @ 2022-01-13 16:20 ` Stefan Berger 1 sibling, 0 replies; 9+ messages in thread From: Stefan Berger @ 2022-01-13 16:20 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, stefanb, qemu-devel, alex.williamson Cc: cohuck, david 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] hw/vfio/common: Silence ram device offset alignment error traces 2022-01-13 10:37 [PATCH 0/2] TPM-CRB: Remove spurious error report when used with VFIO Eric Auger 2022-01-13 10:37 ` [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region Eric Auger @ 2022-01-13 10:37 ` Eric Auger 2022-01-13 16:21 ` Stefan Berger 1 sibling, 1 reply; 9+ messages in thread From: Eric Auger @ 2022-01-13 10:37 UTC (permalink / raw) To: eric.auger.pro, eric.auger, stefanb, qemu-devel, alex.williamson Cc: cohuck, david 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> --- I am not totally clear why we do not fail on the non RAM device case though. --- 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 080046e3f5..9caa560b07 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 0ef1b5f4a6..ccd9d7610d 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.26.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] hw/vfio/common: Silence ram device offset alignment error traces 2022-01-13 10:37 ` [PATCH 2/2] hw/vfio/common: Silence ram device offset alignment error traces Eric Auger @ 2022-01-13 16:21 ` Stefan Berger 0 siblings, 0 replies; 9+ messages in thread From: Stefan Berger @ 2022-01-13 16:21 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, stefanb, qemu-devel, alex.williamson Cc: cohuck, david On 1/13/22 05:37, Eric Auger wrote: > 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> > > --- > > I am not totally clear why we do not fail on the non RAM device case > though. > --- > 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 080046e3f5..9caa560b07 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 0ef1b5f4a6..ccd9d7610d 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-01-14 8:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-13 10:37 [PATCH 0/2] TPM-CRB: Remove spurious error report when used with VFIO Eric Auger 2022-01-13 10:37 ` [PATCH 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region Eric Auger 2022-01-13 14:06 ` Stefan Berger 2022-01-13 14:40 ` Eric Auger 2022-01-13 15:38 ` Stefan Berger 2022-01-14 8:33 ` Eric Auger 2022-01-13 16:20 ` Stefan Berger 2022-01-13 10:37 ` [PATCH 2/2] hw/vfio/common: Silence ram device offset alignment error traces Eric Auger 2022-01-13 16:21 ` Stefan Berger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.