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

Commit Message

Eric Auger Feb. 8, 2022, 1:38 p.m. UTC

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 <[e-mail]>
Tested-by: Stefan Berger <[e-mail]>
Acked-by: Stefan Berger <[e-mail]>
[PMD: Keep tpm_crb.c in meson's softmmu_ss]
Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>

---

v3 -> v4:
-  call vmstate_unregister_ram
---
 hw/tpm/tpm_crb.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Peter Maydell Feb. 8, 2022, 3:17 p.m. UTC | #1

On Tue, 8 Feb 2022 at 15:08, Eric Auger <[e-mail]> 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.

> +    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

Eric Auger Feb. 8, 2022, 3:56 p.m. UTC | #2

Hi Peter,

On 2/8/22 4:17 PM, Peter Maydell wrote:
> On Tue, 8 Feb 2022 at 15:08, Eric Auger <[e-mail]> 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
>

Peter Maydell Feb. 8, 2022, 4:01 p.m. UTC | #3

On Tue, 8 Feb 2022 at 15:56, Eric Auger <[e-mail]> wrote:
>
> Hi Peter,
>
> On 2/8/22 4:17 PM, Peter Maydell wrote:
> > On Tue, 8 Feb 2022 at 15:08, Eric Auger <[e-mail]> 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.

Well, it can if it likes, but "this is a RAM-backed MemoryRegion"
doesn't imply "this is really guest actual RAM RAM", so if it's
using that as its discriminator it should probably use something else.
What is it actually trying to do here ?

thanks
-- PMM

Alex Williamson Feb. 8, 2022, 4:36 p.m. UTC | #4

On Tue, 8 Feb 2022 16:01:48 +0000
Peter Maydell <[e-mail]> wrote:

> On Tue, 8 Feb 2022 at 15:56, Eric Auger <[e-mail]> wrote:
> >
> > Hi Peter,
> >
> > On 2/8/22 4:17 PM, Peter Maydell wrote:  
> > > On Tue, 8 Feb 2022 at 15:08, Eric Auger <[e-mail]> 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.  
> 
> Well, it can if it likes, but "this is a RAM-backed MemoryRegion"
> doesn't imply "this is really guest actual RAM RAM", so if it's
> using that as its discriminator it should probably use something else.
> What is it actually trying to do here ?

VFIO is device agnostic, we don't understand the device programming
model, we can't know how the device is programmed to perform DMA.  The
only way we can provide transparent assignment of arbitrary PCI devices
is to install DMA mappings for everything in the device AddressSpace
through the system IOMMU.  If we were to get a sub-page RAM mapping
through the MemoryListener and that mapping had the possibility of
being a DMA target, then we have a problem, because we cannot represent
that through the IOMMU.  If the device were to use that address for DMA,
we'd likely have data loss/corruption in the VM.

AFAIK, and I thought we had some general agreement on this, declaring
device memory as ram_device is the only means we have to differentiate
MemoryRegion segments generated by a device from actual system RAM.
For device memory, we can lean on the fact that peer-to-peer DMA is
much more rare and likely involves some degree of validation by the
drivers since it can be blocked on physical hardware due to various
topology and chipset related issues.  Therefore we can consider
failures to map device memory at a lower risk than failures to map
ranges we think are actual system RAM.

Are there better approaches?  We can't rely on the device sitting
behind a vIOMMU in the guest to restrict the address space and we can't
afford the performance hit for dyanmic DMA mappings through a vIOMMU
either.  Thanks,

Alex

Eric Auger Feb. 8, 2022, 4:42 p.m. UTC | #5

Hi,
On 2/8/22 5:01 PM, Peter Maydell wrote:
> On Tue, 8 Feb 2022 at 15:56, Eric Auger <[e-mail]> wrote:
>> Hi Peter,
>>
>> On 2/8/22 4:17 PM, Peter Maydell wrote:
>>> On Tue, 8 Feb 2022 at 15:08, Eric Auger <[e-mail]> 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.
> Well, it can if it likes, but "this is a RAM-backed MemoryRegion"
> doesn't imply "this is really guest actual RAM RAM", so if it's
> using that as its discriminator it should probably use something else.
> What is it actually trying to do here ?
We avoid outputting an error msg for something that is not an issue.
Besides, a little bit farther in hw/vfio/common.c
(vfio_listener_region_add) memory_region_is_ram_device() already is used
to avoid doing the dma_map for this type of region.

Originally, according to 21e00fa55f3 ("memory: Replace skip_dump flag
with "ram_device"), we had a skip_dump field in MemoryRegion which was
then turned into a whole ram_device type. Doing it differently would
mean that we would somehow introduce a new flag saying skip_dma_map?
Originally this was mainly meant for MMIO bars but I understood from
Alex that it could be sensible in that case too.

Thanks

Eric
>
> thanks
> -- PMM
>

Dr. David Alan Gilbert Feb. 8, 2022, 5:03 p.m. UTC | #6

* Eric Auger ([e-mail]) 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.

Don't the weird sizes and alignments also cause problems with the RAM
migration code?
Why don't you just align this up to a convenient boundary?

Dave

> Signed-off-by: Eric Auger <[e-mail]>
> Tested-by: Stefan Berger <[e-mail]>
> Acked-by: Stefan Berger <[e-mail]>
> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> 
> ---
> 
> v3 -> v4:
> -  call vmstate_unregister_ram
> ---
>  hw/tpm/tpm_crb.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 58ebd1469c3..668f969b409 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, 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);
>  }
>  
> +static void tpm_crb_unrealize(DeviceState *dev)
> +{
> +    CRBState *s = CRB(dev);
> +
> +    vmstate_unregister_ram(&s->cmdmem, 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  = &vmstate_tpm_crb;
>      dc->user_creatable = true;
> -- 
> 2.26.3
>

Peter Maydell Feb. 8, 2022, 5:14 p.m. UTC | #7

On Tue, 8 Feb 2022 at 16:36, Alex Williamson <[e-mail]> wrote:
>
> On Tue, 8 Feb 2022 16:01:48 +0000
> Peter Maydell <[e-mail]> wrote:
> > Well, it can if it likes, but "this is a RAM-backed MemoryRegion"
> > doesn't imply "this is really guest actual RAM RAM", so if it's
> > using that as its discriminator it should probably use something else.
> > What is it actually trying to do here ?
>
> VFIO is device agnostic, we don't understand the device programming
> model, we can't know how the device is programmed to perform DMA.  The
> only way we can provide transparent assignment of arbitrary PCI devices
> is to install DMA mappings for everything in the device AddressSpace
> through the system IOMMU.  If we were to get a sub-page RAM mapping
> through the MemoryListener and that mapping had the possibility of
> being a DMA target, then we have a problem, because we cannot represent
> that through the IOMMU.  If the device were to use that address for DMA,
> we'd likely have data loss/corruption in the VM.

This is true whether that small MR is RAM-backed or MMIO-backed
or RAM-backed and marked as being a "ram device", though,
isn't it ?

> AFAIK, and I thought we had some general agreement on this, declaring
> device memory as ram_device is the only means we have to differentiate
> MemoryRegion segments generated by a device from actual system RAM.

What do you mean by "generated by a device" here? Devices within
QEMU create MemoryRegions of all kinds; some of them are RAM-backed,
some of them are not.

memory_region_init_ram_device_ptr() is (per the documentation)
for when the backing device is a real host device that vfio is
wrapping to turn into a QEMU device.

tpm_crb is not a real host device, though -- it's an actually
emulated-by-QEMU device.

> For device memory, we can lean on the fact that peer-to-peer DMA is
> much more rare and likely involves some degree of validation by the
> drivers since it can be blocked on physical hardware due to various
> topology and chipset related issues.  Therefore we can consider
> failures to map device memory at a lower risk than failures to map
> ranges we think are actual system RAM.

Well, if it's not page aligned and at least page sized it's
a pretty reasonable guess that it's not system RAM...

thanks
-- PMM

Stefan Berger Feb. 8, 2022, 5:16 p.m. UTC | #8

On 2/8/22 08:38, 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.
>
> Signed-off-by: Eric Auger <[e-mail]>
> Tested-by: Stefan Berger <[e-mail]>
> Acked-by: Stefan Berger <[e-mail]>
> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>


v4 doesn't build for me:

../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?:
../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function 
?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration]
   297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
       |                                 ^~~~~~~~~~~~~~~
../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of 
?HOST_PAGE_ALIGN? [-Werror=nested-externs]
cc1: all warnings being treated as errors

Eric Auger Feb. 8, 2022, 5:58 p.m. UTC | #9

Hi Stefan,

On 2/8/22 6:16 PM, Stefan Berger wrote:
>
> On 2/8/22 08:38, 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.
>>
>> Signed-off-by: Eric Auger <[e-mail]>
>> Tested-by: Stefan Berger <[e-mail]>
>> Acked-by: Stefan Berger <[e-mail]>
>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>
>
> v4 doesn't build for me:
>
> ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?:
> ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function
> ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration]
>   297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
>       |                                 ^~~~~~~~~~~~~~~
> ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of
> ?HOST_PAGE_ALIGN? [-Werror=nested-externs]
> cc1: all warnings being treated as errors

Do you have
b269a70810a  exec/cpu: Make host pages variables / macros 'target
agnostic' in your tree?

Thanks

Eric
>
>
>

Eric Auger Feb. 9, 2022, 9:39 a.m. UTC | #10

Hi Dave,

On 2/8/22 6:03 PM, Dr. David Alan Gilbert wrote:
> * Eric Auger ([e-mail]) 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.
> Don't the weird sizes and alignments also cause problems with the RAM
> migration code?
I tested CRB migration and it seems to work properly.
> Why don't you just align this up to a convenient boundary?
The spec (CG PC Client Platform TPM Profile (PTP)
    Specification Family “2.0” Level 00 Revision 01.03 v22, page 100) 
says that the command/response data "may be defined as large as 3968",
which is (0x1000 - 0x80), 0x80 being the size of the control struct.
so the size of the region logically is less than a 4kB page, hence our
trouble.

We learnt in the past Windows driver has some stronger expectation wrt
memory mapping. I don't know if those latter would work if we were to
enlarge the window by some tricks.


Besides
https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf
says

"
Including the control structure, the three memory areas comprise the
entirety of the CRB. There are no constraints on how those three memory
areas are provided. They can all be in system RAM, or all be in device
memory, or any combination.
"
So device memory looks an option too.

Adding Marc-Andre in the loop

Thanks

Eric





>
> Dave
>
>> Signed-off-by: Eric Auger <[e-mail]>
>> Tested-by: Stefan Berger <[e-mail]>
>> Acked-by: Stefan Berger <[e-mail]>
>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>>
>> ---
>>
>> v3 -> v4:
>> -  call vmstate_unregister_ram
>> ---
>>  hw/tpm/tpm_crb.c | 23 +++++++++++++++++++++--
>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> index 58ebd1469c3..668f969b409 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, 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);
>>  }
>>  
>> +static void tpm_crb_unrealize(DeviceState *dev)
>> +{
>> +    CRBState *s = CRB(dev);
>> +
>> +    vmstate_unregister_ram(&s->cmdmem, 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  = &vmstate_tpm_crb;
>>      dc->user_creatable = true;
>> -- 
>> 2.26.3
>>

Eric Auger Feb. 9, 2022, 9:54 a.m. UTC | #11

Hi Peter,

On 2/8/22 6:14 PM, Peter Maydell wrote:
> On Tue, 8 Feb 2022 at 16:36, Alex Williamson <[e-mail]> wrote:
>> On Tue, 8 Feb 2022 16:01:48 +0000
>> Peter Maydell <[e-mail]> wrote:
>>> Well, it can if it likes, but "this is a RAM-backed MemoryRegion"
>>> doesn't imply "this is really guest actual RAM RAM", so if it's
>>> using that as its discriminator it should probably use something else.
>>> What is it actually trying to do here ?
>> VFIO is device agnostic, we don't understand the device programming
>> model, we can't know how the device is programmed to perform DMA.  The
>> only way we can provide transparent assignment of arbitrary PCI devices
>> is to install DMA mappings for everything in the device AddressSpace
>> through the system IOMMU.  If we were to get a sub-page RAM mapping
>> through the MemoryListener and that mapping had the possibility of
>> being a DMA target, then we have a problem, because we cannot represent
>> that through the IOMMU.  If the device were to use that address for DMA,
>> we'd likely have data loss/corruption in the VM.
> This is true whether that small MR is RAM-backed or MMIO-backed
> or RAM-backed and marked as being a "ram device", though,
> isn't it ?
>
>> AFAIK, and I thought we had some general agreement on this, declaring
>> device memory as ram_device is the only means we have to differentiate
>> MemoryRegion segments generated by a device from actual system RAM.
> What do you mean by "generated by a device" here? Devices within
> QEMU create MemoryRegions of all kinds; some of them are RAM-backed,
> some of them are not.
>
> memory_region_init_ram_device_ptr() is (per the documentation)
> for when the backing device is a real host device that vfio is
> wrapping to turn into a QEMU device.
>
> tpm_crb is not a real host device, though -- it's an actually
> emulated-by-QEMU device.
CRB can work in passthrough mode though, although I don't know the
underlying implementation.

As mentionned in the other email

https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf
says

"
Including the control structure, the three memory areas comprise the
entirety of the CRB. There are no constraints on how those three memory
areas are provided. They can all be in system RAM, or all be in device
memory, or any combination.



>
>> For device memory, we can lean on the fact that peer-to-peer DMA is
>> much more rare and likely involves some degree of validation by the
>> drivers since it can be blocked on physical hardware due to various
>> topology and chipset related issues.  Therefore we can consider
>> failures to map device memory at a lower risk than failures to map
>> ranges we think are actual system RAM.
> Well, if it's not page aligned and at least page sized it's
> a pretty reasonable guess that it's not system RAM...
Assuming that obviously makes things simpler and remove a bunch of
checks & error reports in vfio :) But wouldn' we silence some actual
dma-map failures we properly detect & report today?

Thanks

Eric
>
> thanks
> -- PMM
>

Eric Auger March 3, 2022, 2:37 p.m. UTC | #12

Hi Stefan,

On 2/8/22 6:58 PM, Eric Auger wrote:
> Hi Stefan,
> 
> On 2/8/22 6:16 PM, Stefan Berger wrote:
>>
>> On 2/8/22 08:38, 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.
>>>
>>> Signed-off-by: Eric Auger <[e-mail]>
>>> Tested-by: Stefan Berger <[e-mail]>
>>> Acked-by: Stefan Berger <[e-mail]>
>>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
>>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>>
>>
>> v4 doesn't build for me:
>>
>> ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?:
>> ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function
>> ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration]
>>   297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
>>       |                                 ^~~~~~~~~~~~~~~
>> ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of
>> ?HOST_PAGE_ALIGN? [-Werror=nested-externs]
>> cc1: all warnings being treated as errors
> 
> Do you have
> b269a70810a  exec/cpu: Make host pages variables / macros 'target
> agnostic' in your tree?
I may have missed your reply. Did you have that dependency? Were you
able to compile eventually?

Besides, do you have any opinion overall about the relevance of
transforming the CRB ctrl cmd region into a RAM device wrt the TPM spec?

Again spec says:

"
Including the control structure, the three memory areas comprise the
entirety of the CRB. There are no constraints on how those three memory
areas are provided. They can all be in system RAM, or all be in device
memory, or any combination.
"
(https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf)

What was the rationale behind using RAM device for the PPI region?

There are some spurious warnings when using CRB with VFIO and that would
be nice to remove them one way or the other.

Thanks

Eric
> 
> Thanks
> 
> Eric
>>
>>
>>
>

Marc-André Lureau March 3, 2022, 4:16 p.m. UTC | #13

Hi

On Thu, Mar 3, 2022 at 6:41 PM Eric Auger <[e-mail]> wrote:

> Hi Stefan,
>
> On 2/8/22 6:58 PM, Eric Auger wrote:
> > Hi Stefan,
> >
> > On 2/8/22 6:16 PM, Stefan Berger wrote:
> >>
> >> On 2/8/22 08:38, 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.
> >>>
> >>> Signed-off-by: Eric Auger <[e-mail]>
> >>> Tested-by: Stefan Berger <[e-mail]>
> >>> Acked-by: Stefan Berger <[e-mail]>
> >>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
> >>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> >>
> >>
> >> v4 doesn't build for me:
> >>
> >> ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?:
> >> ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function
> >> ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration]
> >>   297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
> >>       |                                 ^~~~~~~~~~~~~~~
> >> ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of
> >> ?HOST_PAGE_ALIGN? [-Werror=nested-externs]
> >> cc1: all warnings being treated as errors
> >
> > Do you have
> > b269a70810a  exec/cpu: Make host pages variables / macros 'target
> > agnostic' in your tree?
> I may have missed your reply. Did you have that dependency? Were you
> able to compile eventually?
>
> Besides, do you have any opinion overall about the relevance of
> transforming the CRB ctrl cmd region into a RAM device wrt the TPM spec?
>
> Again spec says:
>
> "
> Including the control structure, the three memory areas comprise the
> entirety of the CRB. There are no constraints on how those three memory
> areas are provided. They can all be in system RAM, or all be in device
> memory, or any combination.
> "
> (
> https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf
> )
>
> What was the rationale behind using RAM device for the PPI region?
>

Is this the rationale you are looking for?
https://gitlab.com/qemu-project/qemu/-/commit/3b97c01e9ccdfbd517a0fd631838d6252dbfa692

    Note: bios_linker cannot be used to allocate the PPI memory region,
    since the reserved memory should stay stable across reboots, and might
    be needed before the ACPI tables are installed.


>
> There are some spurious warnings when using CRB with VFIO and that would
> be nice to remove them one way or the other.
>
> Thanks
>
> Eric
> >
> > Thanks
> >
> > Eric
> >>
> >>
> >>
> >
>
>
>

Eric Auger March 4, 2022, 9:32 a.m. UTC | #14

Hi Marc-André,
On 3/3/22 5:16 PM, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Mar 3, 2022 at 6:41 PM Eric Auger <[e-mail]
> <mailto:[e-mail]>> wrote:
> 
>     Hi Stefan,
> 
>     On 2/8/22 6:58 PM, Eric Auger wrote:
>     > Hi Stefan,
>     >
>     > On 2/8/22 6:16 PM, Stefan Berger wrote:
>     >>
>     >> On 2/8/22 08:38, 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.
>     >>>
>     >>> Signed-off-by: Eric Auger <[e-mail]
>     <mailto:[e-mail]>>
>     >>> Tested-by: Stefan Berger <[e-mail]
>     <mailto:[e-mail]>>
>     >>> Acked-by: Stefan Berger <[e-mail]
>     <mailto:[e-mail]>>
>     >>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
>     >>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]
>     <mailto:[e-mail]>>
>     >>
>     >>
>     >> v4 doesn't build for me:
>     >>
>     >> ../hw/tpm/tpm_crb.c: In function ?tpm_crb_realize?:
>     >> ../hw/tpm/tpm_crb.c:297:33: error: implicit declaration of function
>     >> ?HOST_PAGE_ALIGN? [-Werror=implicit-function-declaration]
>     >>   297 | HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
>     >>       |                                 ^~~~~~~~~~~~~~~
>     >> ../hw/tpm/tpm_crb.c:297:33: error: nested extern declaration of
>     >> ?HOST_PAGE_ALIGN? [-Werror=nested-externs]
>     >> cc1: all warnings being treated as errors
>     >
>     > Do you have
>     > b269a70810a  exec/cpu: Make host pages variables / macros 'target
>     > agnostic' in your tree?
>     I may have missed your reply. Did you have that dependency? Were you
>     able to compile eventually?
> 
>     Besides, do you have any opinion overall about the relevance of
>     transforming the CRB ctrl cmd region into a RAM device wrt the TPM spec?
> 
>     Again spec says:
> 
>     "
>     Including the control structure, the three memory areas comprise the
>     entirety of the CRB. There are no constraints on how those three memory
>     areas are provided. They can all be in system RAM, or all be in device
>     memory, or any combination.
>     "
>     (https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf
>     <https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf>)
> 
>     What was the rationale behind using RAM device for the PPI region?
> 
> 
> Is this the rationale you are looking for?
> https://gitlab.com/qemu-project/qemu/-/commit/3b97c01e9ccdfbd517a0fd631838d6252dbfa692
> <https://gitlab.com/qemu-project/qemu/-/commit/3b97c01e9ccdfbd517a0fd631838d6252dbfa692>
> 
>     Note: bios_linker cannot be used to allocate the PPI memory region,
>     since the reserved memory should stay stable across reboots, and might
>     be needed before the ACPI tables are installed.
And did this mandate to use "ram_device" memory type instead of standard
system RAM.

As I understand the spec (statement above), the CRB areas can be
implemented as system RAM or device memory. So I want to understand why
using RAM device for the CRB is not a reasonable choice. By the way I
understand my motivation behind that change is a bit far-fetched and
aiming at fixing another issue, but well ;-)

Thanks

Eric
>  
> 
> 
>     There are some spurious warnings when using CRB with VFIO and that would
>     be nice to remove them one way or the other.
> 
>     Thanks
> 
>     Eric
>     >
>     > Thanks
>     >
>     > Eric
>     >>
>     >>
>     >>
>     >
> 
> 
> 
> 
> -- 
> Marc-André Lureau

Patch

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 58ebd1469c3..668f969b409 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, 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);
 }
 
+static void tpm_crb_unrealize(DeviceState *dev)
+{
+    CRBState *s = CRB(dev);
+
+    vmstate_unregister_ram(&s->cmdmem, 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  = &vmstate_tpm_crb;
     dc->user_creatable = true;