Hi Igor,
On 2/4/22 1:08 PM, Igor Mammedov wrote:
> On Thu, 03 Feb 2022 15:35:35 -0700
> Alex Williamson <alex.william...@redhat.com> wrote:
>
>> From: Eric Auger <eric.au...@redhat.com>
>>
>> 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.
>>
>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>> Tested-by: Stefan Berger <stef...@linux.ibm.com>
>> Acked-by: Stefan Berger <stef...@linux.ibm.com>
>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>> Link: https://lore.kernel.org/r/20220120001242.230082-2-f4...@amsat.org
>> Signed-off-by: Alex Williamson <alex.william...@redhat.com>
>> ---
>> hw/tpm/tpm_crb.c | 22 ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> index 58ebd1469c35..be0884ea6031 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 "exec/cpu-common.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));
> Does it need a compat knob for the case of migrating to older QEMU/machine
> type,
> not to end-up with target aborting migration when it sees unknown section.
It does not seem to be requested. I am able to migrate between this
version and qemu 6.2, back and forth, using a pc-q35-6.2 machine type.
My guess is, as the amount of RAM that is migrated is the same, it does
not complain. Adding Dave and Juan though.
Thanks
Eric
>
>
>> memory_region_add_subregion(get_system_memory(),
>> TPM_CRB_ADDR_BASE, &s->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);
>> +
> likewise, should vmstate be unregistered here, before freeing
> actually happens?
>
>> + 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 = &vmstate_tpm_crb;
>> dc->user_creatable = true;
>>
>>
>>
