Re: [PATCH v2 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
On Wed, 19 Jan 2022 23:46:19 +0100 Philippe Mathieu-Daudé wrote: > On 18/1/22 16:33, 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 > > Tested-by: Stefan Berger > > > > --- > > > > v1 -> v2: > > - Add tpm_crb_unrealize > > --- > > hw/tpm/meson.build | 2 +- > > hw/tpm/tpm_crb.c | 22 -- > > 2 files changed, 21 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')) > > We don't need to make this file target-specific. > > > 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..6ec19a9911 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(ç, > > +HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); > > HOST_PAGE_ALIGN() and qemu_real_host_page_size() actually belong > to "exec/cpu-common.h". > > Alex, could you hold on a few days for this patch? I am going to send > a cleanup series. Otherwise no worry, I will clean this on top too. Sure. Thanks, Alex
Re: [PATCH v2 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
On 18/1/22 16:33, 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 Tested-by: Stefan Berger --- v1 -> v2: - Add tpm_crb_unrealize --- hw/tpm/meson.build | 2 +- hw/tpm/tpm_crb.c | 22 -- 2 files changed, 21 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')) We don't need to make this file target-specific. 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..6ec19a9911 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(ç, +HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); HOST_PAGE_ALIGN() and qemu_real_host_page_size() actually belong to "exec/cpu-common.h". Alex, could you hold on a few days for this patch? I am going to send a cleanup series. Otherwise no worry, I will clean this on top too. Thanks, Phil.
[PATCH v2 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
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 Tested-by: Stefan Berger --- v1 -> v2: - Add tpm_crb_unrealize --- hw/tpm/meson.build | 2 +- hw/tpm/tpm_crb.c | 22 -- 2 files changed, 21 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..6ec19a9911 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(>mmio, OBJECT(s), _crb_memory_ops, s, "tpm-crb-mmio", sizeof(s->regs)); -memory_region_init_ram(>cmdmem, OBJECT(s), -"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp); +memory_region_init_ram_device_ptr(>cmdmem, OBJECT(s), "tpm-crb-cmd", + CRB_CTRL_CMD_SIZE, s->crb_cmd_buf); +vmstate_register_ram(>cmdmem, DEVICE(s)); memory_region_add_subregion(get_system_memory(), TPM_CRB_ADDR_BASE, >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 = _tpm_crb; dc->user_creatable = true; -- 2.26.3