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.au...@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);
>
