[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PULL 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region |
Date: |
Wed, 23 Feb 2022 13:02:37 +0000 |
User-agent: |
Mutt/2.1.5 (2021-12-30) |
* Eric Auger (eric.auger@redhat.com) wrote:
> Hi Igor,
>
> On 2/4/22 1:08 PM, Igor Mammedov wrote:
> > On Thu, 03 Feb 2022 15:35:35 -0700
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >
> >> From: Eric Auger <eric.auger@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.auger@redhat.com>
> >> Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> >> Acked-by: Stefan Berger <stefanb@linux.ibm.com>
> >> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> Link: https://lore.kernel.org/r/20220120001242.230082-2-f4bug@amsat.org
> >> Signed-off-by: Alex Williamson <alex.williamson@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.
I think that should be OK; we just rely on the RAM Block name and size.
Dave
> 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;
> >>
> >>
> >>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK