| Message ID | 20220113103757.2801389-2-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TPM-CRB: Remove spurious error report when used with VFIO | expand |
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);
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); >
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);
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);
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); >
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);
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(-)