Re: [PATCH v4 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region

[Eric Auger]

Hi Peter,

On 2/8/22 4:17 PM, Peter Maydell wrote:
> On Tue, 8 Feb 2022 at 15:08, Eric Auger <eric.auger@redhat.com> 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.
> This seems like VFIO's problem to me. There's nothing
> that guarantees alignment for memory regions at all,
> whether they're RAM, IO or anything else.

VFIO dma maps all the guest RAM. I understand the cmd/response buffer is
RAM but does not need to be dma mapped, all the more so it has a bad
alignment. By the way the PPI region also has the ram_device type
(tpm_ppi.c tpm_ppi_init). In that case, using the ram_device type allows
VFIO to discriminate between critical mapping errors and non critical
ones. We have no other mean atm.

Thanks

Eric
>
>> +    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, dev);
>>
>>      memory_region_add_subregion(get_system_memory(),
>>          TPM_CRB_ADDR_BASE, &s->mmio);
>> @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error 
>> **errp)
>>      qemu_register_reset(tpm_crb_reset, dev);
>>  }
> As QEMU code goes, this seems much worse than what it replaces.
> To have a memory region backed by RAM and migrated in the
> usual way, memory_region_init_ram() is the right thing.
>
> thanks
> -- PMM
>