[PATCH 0/2] vhost-vdpa: skip TPM CRB memory section

[PATCH 0/2] vhost-vdpa: skip TPM CRB memory section

[Laurent Vivier]

An error is reported for vhost-vdpa case:
qemu-kvm: vhost_vdpa_listener_region_add received unaligned region

Marc-André has proposed a fix to this problem by skipping
the memory region owned by the TPM CRB but it seems more generic
to skip not DMA-able memory.

We have a memory flag for that, RAM_PROTECTED.

This series expands the memory API to provide a way to initialize
a "protected" memory region and use it with the TPM CRB object.

For the previous discussions, see

https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html

and from Eric for VFIO:

https://lore.kernel.org/all/20220506132510.1847942-1-eric.auger@redhat.com/
https://lore.kernel.org/all/20220524091405.416256-1-eric.auger@redhat.com/

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2141965

Thanks,
Laurent

Laurent Vivier (2):
  memory: introduce memory_region_init_ram_protected()
  tpm_crb: mark memory as protected

 hw/tpm/tpm_crb.c      |  2 +-
 include/exec/memory.h | 33 +++++++++++++++++++++++++++++++++
 softmmu/memory.c      | 33 +++++++++++++++++++++++++++------
 softmmu/physmem.c     |  4 ++--
 4 files changed, 63 insertions(+), 9 deletions(-)

-- 
2.41.0

[PATCH 1/2] memory: introduce memory_region_init_ram_protected()

[Laurent Vivier]

Commit 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
has introduced the RAM_PROTECTED flag to denote "protected" memory.

This flags is only used with qemu_ram_alloc_from_fd() for now.

To be able to register memory region with this flag, define
memory_region_init_ram_protected() and declare the flag as valid in
qemu_ram_alloc_internal() and qemu_ram_alloc().

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 include/exec/memory.h | 33 +++++++++++++++++++++++++++++++++
 softmmu/memory.c      | 33 +++++++++++++++++++++++++++------
 softmmu/physmem.c     |  4 ++--
 3 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 47c2e0221c35..d8760015c381 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1520,6 +1520,39 @@ void memory_region_init_iommu(void *_iommu_mr,
                               const char *name,
                               uint64_t size);
 
+/**
+ * memory_region_init_ram_protected - Initialize RAM memory region.  Accesses
+ *                                    into the region will modify memory
+ *                                    directly.
+ *
+ * The memory is created with the RAM_PROTECTED flag, for memory that
+ * looks and acts like RAM but inaccessible via normal mechanisms,
+ * including DMA.
+ *
+ * @mr: the #MemoryRegion to be initialized
+ * @owner: the object that tracks the region's reference count (must be
+ *         TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL)
+ * @name: name of the memory region
+ * @size: size of the region in bytes
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * This function allocates RAM for a board model or device, and
+ * arranges for it to be migrated (by calling vmstate_register_ram()
+ * if @owner is a DeviceState, or vmstate_register_ram_global() if
+ * @owner is NULL).
+ *
+ * TODO: Currently we restrict @owner to being either NULL (for
+ * global RAM regions with no owner) or devices, so that we can
+ * give the RAM block a unique name for migration purposes.
+ * We should lift this restriction and allow arbitrary Objects.
+ * If you pass a non-NULL non-device @owner then we will assert.
+ */
+void memory_region_init_ram_protected(MemoryRegion *mr,
+                                      Object *owner,
+                                      const char *name,
+                                      uint64_t size,
+                                      Error **errp);
+
 /**
  * memory_region_init_ram - Initialize RAM memory region.  Accesses into the
  *                          region will modify memory directly.
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 7d9494ce7028..952c87277353 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -3551,16 +3551,18 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
     }
 }
 
-void memory_region_init_ram(MemoryRegion *mr,
-                            Object *owner,
-                            const char *name,
-                            uint64_t size,
-                            Error **errp)
+static void memory_region_init_ram_flags(MemoryRegion *mr,
+                                         Object *owner,
+                                         const char *name,
+                                         uint64_t size,
+                                         uint32_t ram_flags,
+                                         Error **errp)
 {
     DeviceState *owner_dev;
     Error *err = NULL;
 
-    memory_region_init_ram_nomigrate(mr, owner, name, size, &err);
+    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, ram_flags,
+                                           &err);
     if (err) {
         error_propagate(errp, err);
         return;
@@ -3575,6 +3577,25 @@ void memory_region_init_ram(MemoryRegion *mr,
     vmstate_register_ram(mr, owner_dev);
 }
 
+void memory_region_init_ram_protected(MemoryRegion *mr,
+                                      Object *owner,
+                                      const char *name,
+                                      uint64_t size,
+                                      Error **errp)
+{
+        memory_region_init_ram_flags(mr, owner, name, size, RAM_PROTECTED,
+                                     errp);
+}
+
+void memory_region_init_ram(MemoryRegion *mr,
+                            Object *owner,
+                            const char *name,
+                            uint64_t size,
+                            Error **errp)
+{
+        memory_region_init_ram_flags(mr, owner, name, size, 0, errp);
+}
+
 void memory_region_init_rom(MemoryRegion *mr,
                             Object *owner,
                             const char *name,
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 6bdd944fe880..bf66c81e7255 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1978,7 +1978,7 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
     Error *local_err = NULL;
 
     assert((ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE | RAM_PREALLOC |
-                          RAM_NORESERVE)) == 0);
+                          RAM_NORESERVE | RAM_PROTECTED)) == 0);
     assert(!host ^ (ram_flags & RAM_PREALLOC));
 
     size = HOST_PAGE_ALIGN(size);
@@ -2012,7 +2012,7 @@ RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
 RAMBlock *qemu_ram_alloc(ram_addr_t size, uint32_t ram_flags,
                          MemoryRegion *mr, Error **errp)
 {
-    assert((ram_flags & ~(RAM_SHARED | RAM_NORESERVE)) == 0);
+    assert((ram_flags & ~(RAM_SHARED | RAM_NORESERVE | RAM_PROTECTED)) == 0);
     return qemu_ram_alloc_internal(size, size, NULL, NULL, ram_flags, mr, errp);
 }
 
-- 
2.41.0

Re: [PATCH 1/2] memory: introduce memory_region_init_ram_protected()

[Stefan Berger]

On 6/20/23 15:50, Laurent Vivier wrote:
> Commit 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
> has introduced the RAM_PROTECTED flag to denote "protected" memory.
> 
> This flags is only used with qemu_ram_alloc_from_fd() for now.
> 
> To be able to register memory region with this flag, define
> memory_region_init_ram_protected() and declare the flag as valid in
> qemu_ram_alloc_internal() and qemu_ram_alloc().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   include/exec/memory.h | 33 +++++++++++++++++++++++++++++++++
>   softmmu/memory.c      | 33 +++++++++++++++++++++++++++------
>   softmmu/physmem.c     |  4 ++--
>   3 files changed, 62 insertions(+), 8 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 47c2e0221c35..d8760015c381 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1520,6 +1520,39 @@ void memory_region_init_iommu(void *_iommu_mr,
>                                 const char *name,
>                                 uint64_t size);
> 
> +/**
> + * memory_region_init_ram_protected - Initialize RAM memory region.  Accesses
> + *                                    into the region will modify memory
> + *                                    directly.
> + *
> + * The memory is created with the RAM_PROTECTED flag, for memory that
> + * looks and acts like RAM but inaccessible via normal mechanisms,

but is inaccessible


> + * including DMA.
> + *
> + * @mr: the #MemoryRegion to be initialized
> + * @owner: the object that tracks the region's reference count (must be
> + *         TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL)
> + * @name: name of the memory region
> + * @size: size of the region in bytes
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * This function allocates RAM for a board model or device, and
> + * arranges for it to be migrated (by calling vmstate_register_ram()
> + * if @owner is a DeviceState, or vmstate_register_ram_global() if
> + * @owner is NULL).
> + *
> + * TODO: Currently we restrict @owner to being either NULL (for
> + * global RAM regions with no owner) or devices, so that we can
> + * give the RAM block a unique name for migration purposes.
> + * We should lift this restriction and allow arbitrary Objects.
> + * If you pass a non-NULL non-device @owner then we will assert.
> + */
> +void memory_region_init_ram_protected(MemoryRegion *mr,
> +                                      Object *owner,
> +                                      const char *name,
> +                                      uint64_t size,
> +                                      Error **errp);
> +
>   /**
>    * memory_region_init_ram - Initialize RAM memory region.  Accesses into the
>    *                          region will modify memory directly.
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7d9494ce7028..952c87277353 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3551,16 +3551,18 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>       }
>   }
> 
> -void memory_region_init_ram(MemoryRegion *mr,
> -                            Object *owner,
> -                            const char *name,
> -                            uint64_t size,
> -                            Error **errp)
> +static void memory_region_init_ram_flags(MemoryRegion *mr,
> +                                         Object *owner,
> +                                         const char *name,
> +                                         uint64_t size,
> +                                         uint32_t ram_flags,
> +                                         Error **errp)
>   {
>       DeviceState *owner_dev;
>       Error *err = NULL;
> 
> -    memory_region_init_ram_nomigrate(mr, owner, name, size, &err);
> +    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, ram_flags,
> +                                           &err);
>       if (err) {
>           error_propagate(errp, err);
>           return;
> @@ -3575,6 +3577,25 @@ void memory_region_init_ram(MemoryRegion *mr,
>       vmstate_register_ram(mr, owner_dev);
>   }
> 
> +void memory_region_init_ram_protected(MemoryRegion *mr,
> +                                      Object *owner,
> +                                      const char *name,
> +                                      uint64_t size,
> +                                      Error **errp)
> +{
> +        memory_region_init_ram_flags(mr, owner, name, size, RAM_PROTECTED,
> +                                     errp);
> +}
> +
> +void memory_region_init_ram(MemoryRegion *mr,
> +                            Object *owner,
> +                            const char *name,
> +                            uint64_t size,
> +                            Error **errp)
> +{
> +        memory_region_init_ram_flags(mr, owner, name, size, 0, errp);
> +}
> +
>   void memory_region_init_rom(MemoryRegion *mr,
>                               Object *owner,
>                               const char *name,
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 6bdd944fe880..bf66c81e7255 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1978,7 +1978,7 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
>       Error *local_err = NULL;
> 
>       assert((ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE | RAM_PREALLOC |
> -                          RAM_NORESERVE)) == 0);
> +                          RAM_NORESERVE | RAM_PROTECTED)) == 0);
>       assert(!host ^ (ram_flags & RAM_PREALLOC));
> 
>       size = HOST_PAGE_ALIGN(size);
> @@ -2012,7 +2012,7 @@ RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>   RAMBlock *qemu_ram_alloc(ram_addr_t size, uint32_t ram_flags,
>                            MemoryRegion *mr, Error **errp)
>   {
> -    assert((ram_flags & ~(RAM_SHARED | RAM_NORESERVE)) == 0);
> +    assert((ram_flags & ~(RAM_SHARED | RAM_NORESERVE | RAM_PROTECTED)) == 0);
>       return qemu_ram_alloc_internal(size, size, NULL, NULL, ram_flags, mr, errp);
>   }
> 

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH 1/2] memory: introduce memory_region_init_ram_protected()

[David Hildenbrand]

On 20.06.23 21:50, Laurent Vivier wrote:
> Commit 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
> has introduced the RAM_PROTECTED flag to denote "protected" memory.
> 
> This flags is only used with qemu_ram_alloc_from_fd() for now.
> 
> To be able to register memory region with this flag, define
> memory_region_init_ram_protected() and declare the flag as valid in
> qemu_ram_alloc_internal() and qemu_ram_alloc().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   include/exec/memory.h | 33 +++++++++++++++++++++++++++++++++
>   softmmu/memory.c      | 33 +++++++++++++++++++++++++++------
>   softmmu/physmem.c     |  4 ++--
>   3 files changed, 62 insertions(+), 8 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 47c2e0221c35..d8760015c381 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1520,6 +1520,39 @@ void memory_region_init_iommu(void *_iommu_mr,
>                                 const char *name,
>                                 uint64_t size);
>   
> +/**
> + * memory_region_init_ram_protected - Initialize RAM memory region.  Accesses
> + *                                    into the region will modify memory
> + *                                    directly.
> + *
> + * The memory is created with the RAM_PROTECTED flag, for memory that
> + * looks and acts like RAM but inaccessible via normal mechanisms,
> + * including DMA.
> + *
> + * @mr: the #MemoryRegion to be initialized
> + * @owner: the object that tracks the region's reference count (must be
> + *         TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL)
> + * @name: name of the memory region
> + * @size: size of the region in bytes
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * This function allocates RAM for a board model or device, and
> + * arranges for it to be migrated (by calling vmstate_register_ram()
> + * if @owner is a DeviceState, or vmstate_register_ram_global() if
> + * @owner is NULL).
> + *
> + * TODO: Currently we restrict @owner to being either NULL (for
> + * global RAM regions with no owner) or devices, so that we can
> + * give the RAM block a unique name for migration purposes.
> + * We should lift this restriction and allow arbitrary Objects.
> + * If you pass a non-NULL non-device @owner then we will assert.
> + */
> +void memory_region_init_ram_protected(MemoryRegion *mr,
> +                                      Object *owner,
> +                                      const char *name,
> +                                      uint64_t size,
> +                                      Error **errp);
> +
>   /**
>    * memory_region_init_ram - Initialize RAM memory region.  Accesses into the
>    *                          region will modify memory directly.
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7d9494ce7028..952c87277353 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3551,16 +3551,18 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>       }
>   }
>   
> -void memory_region_init_ram(MemoryRegion *mr,
> -                            Object *owner,
> -                            const char *name,
> -                            uint64_t size,
> -                            Error **errp)
> +static void memory_region_init_ram_flags(MemoryRegion *mr,
> +                                         Object *owner,
> +                                         const char *name,
> +                                         uint64_t size,
> +                                         uint32_t ram_flags,
> +                                         Error **errp)
>   {
>       DeviceState *owner_dev;
>       Error *err = NULL;
>   
> -    memory_region_init_ram_nomigrate(mr, owner, name, size, &err);
> +    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, ram_flags,
> +                                           &err);
>       if (err) {
>           error_propagate(errp, err);
>           return;
> @@ -3575,6 +3577,25 @@ void memory_region_init_ram(MemoryRegion *mr,
>       vmstate_register_ram(mr, owner_dev);
>   }
>   
> +void memory_region_init_ram_protected(MemoryRegion *mr,
> +                                      Object *owner,
> +                                      const char *name,
> +                                      uint64_t size,
> +                                      Error **errp)
> +{
> +        memory_region_init_ram_flags(mr, owner, name, size, RAM_PROTECTED,
> +                                     errp);
> +}
> +
> +void memory_region_init_ram(MemoryRegion *mr,
> +                            Object *owner,
> +                            const char *name,
> +                            uint64_t size,
> +                            Error **errp)
> +{
> +        memory_region_init_ram_flags(mr, owner, name, size, 0, errp);
> +}
> +
>   void memory_region_init_rom(MemoryRegion *mr,
>                               Object *owner,
>                               const char *name,
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 6bdd944fe880..bf66c81e7255 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1978,7 +1978,7 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
>       Error *local_err = NULL;
>   
>       assert((ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE | RAM_PREALLOC |
> -                          RAM_NORESERVE)) == 0);
> +                          RAM_NORESERVE | RAM_PROTECTED)) == 0);
>       assert(!host ^ (ram_flags & RAM_PREALLOC));
>   
>       size = HOST_PAGE_ALIGN(size);
> @@ -2012,7 +2012,7 @@ RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>   RAMBlock *qemu_ram_alloc(ram_addr_t size, uint32_t ram_flags,
>                            MemoryRegion *mr, Error **errp)
>   {
> -    assert((ram_flags & ~(RAM_SHARED | RAM_NORESERVE)) == 0);
> +    assert((ram_flags & ~(RAM_SHARED | RAM_NORESERVE | RAM_PROTECTED)) == 0);
>       return qemu_ram_alloc_internal(size, size, NULL, NULL, ram_flags, mr, errp);
>   }
>   

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] memory: introduce memory_region_init_ram_protected()

[Peter Maydell]

On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> wrote:
>
> Commit 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
> has introduced the RAM_PROTECTED flag to denote "protected" memory.
>
> This flags is only used with qemu_ram_alloc_from_fd() for now.
>
> To be able to register memory region with this flag, define
> memory_region_init_ram_protected() and declare the flag as valid in
> qemu_ram_alloc_internal() and qemu_ram_alloc().
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  include/exec/memory.h | 33 +++++++++++++++++++++++++++++++++
>  softmmu/memory.c      | 33 +++++++++++++++++++++++++++------
>  softmmu/physmem.c     |  4 ++--
>  3 files changed, 62 insertions(+), 8 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 47c2e0221c35..d8760015c381 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1520,6 +1520,39 @@ void memory_region_init_iommu(void *_iommu_mr,
>                                const char *name,
>                                uint64_t size);
>
> +/**
> + * memory_region_init_ram_protected - Initialize RAM memory region.  Accesses
> + *                                    into the region will modify memory
> + *                                    directly.
> + *
> + * The memory is created with the RAM_PROTECTED flag, for memory that
> + * looks and acts like RAM but inaccessible via normal mechanisms,
> + * including DMA.

This doesn't really tell me why you might want to mark
a region as RAM_PROTECTED. What kind of memory region is
not DMAable to? What are "normal mechanisms" here?
What are the "non-normal mechanisms" that you *can* use on
this memory region?

At the moment we only seem to use RAM_PROTECTED for
the SGX EPC memory backend. The commit message adding
that flag is pretty vague about what it means...

thanks
-- PMM

[PATCH 2/2] tpm_crb: mark memory as protected

[Laurent Vivier]

This memory is not correctly aligned and cannot be registered
by vDPA and VFIO.

An error is reported for vhost-vdpa case:
qemu-kvm: vhost_vdpa_listener_region_add received unaligned region

To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.

The RAM_PROTECTED flag has been introduced to skip memory
region that looks like RAM but is not accessible via normal
mechanims, including DMA.

See 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2141965

cc: peter.maydell@linaro.org
cc: marcandre.lureau@redhat.com
cc: eric.auger@redhat.com
cc: mst@redhat.com
cc: jasowang@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/tpm/tpm_crb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index ea930da545af..0a93c488f2fa 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -296,7 +296,7 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
 
     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),
+    memory_region_init_ram_protected(&s->cmdmem, OBJECT(s),
         "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
 
     memory_region_add_subregion(get_system_memory(),
-- 
2.41.0

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[David Hildenbrand]

On 20.06.23 21:50, Laurent Vivier wrote:
> This memory is not correctly aligned and cannot be registered
> by vDPA and VFIO.
> 
> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> 
> To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.

So, VFIO will simply skip these sections via 
vfio_listener_valid_section() I guess.

Yes, it will report an error but it will happily continue.

So regarding vDPA, we're also only concerned about removing the reported 
error, everything else works as expected?

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[Laurent Vivier]

On 6/21/23 11:13, David Hildenbrand wrote:
> On 20.06.23 21:50, Laurent Vivier wrote:
>> This memory is not correctly aligned and cannot be registered
>> by vDPA and VFIO.
>>
>> An error is reported for vhost-vdpa case:
>> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>>
>> To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.
> 
> So, VFIO will simply skip these sections via vfio_listener_valid_section() I guess.
> 
> Yes, it will report an error but it will happily continue.
> 
> So regarding vDPA, we're also only concerned about removing the reported error, everything 
> else works as expected?
> 

Yes, it has been tested and vDPA works as expected.

Thanks,
Laurent

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[David Hildenbrand]

On 22.06.23 14:59, Laurent Vivier wrote:
> On 6/21/23 11:13, David Hildenbrand wrote:
>> On 20.06.23 21:50, Laurent Vivier wrote:
>>> This memory is not correctly aligned and cannot be registered
>>> by vDPA and VFIO.
>>>
>>> An error is reported for vhost-vdpa case:
>>> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>>>
>>> To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.
>>
>> So, VFIO will simply skip these sections via vfio_listener_valid_section() I guess.
>>
>> Yes, it will report an error but it will happily continue.
>>
>> So regarding vDPA, we're also only concerned about removing the reported error, everything
>> else works as expected?
>>
> 
> Yes, it has been tested and vDPA works as expected.

Okay, so no Fixes: tags required

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[Stefan Berger]

On 6/20/23 15:50, Laurent Vivier wrote:
> This memory is not correctly aligned and cannot be registered
> by vDPA and VFIO.
> 
> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> 
> To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.
> 
> The RAM_PROTECTED flag has been introduced to skip memory
> region that looks like RAM but is not accessible via normal
> mechanims, including DMA.
> 
> See 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
> 
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> 
> cc: peter.maydell@linaro.org
> cc: marcandre.lureau@redhat.com
> cc: eric.auger@redhat.com
> cc: mst@redhat.com
> cc: jasowang@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   hw/tpm/tpm_crb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index ea930da545af..0a93c488f2fa 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -296,7 +296,7 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
> 
>       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),
> +    memory_region_init_ram_protected(&s->cmdmem, OBJECT(s),
>           "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> 
>       memory_region_add_subregion(get_system_memory(),

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[David Hildenbrand]

On 20.06.23 21:50, Laurent Vivier wrote:
> This memory is not correctly aligned and cannot be registered
> by vDPA and VFIO.
> 
> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> 
> To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.
> 
> The RAM_PROTECTED flag has been introduced to skip memory
> region that looks like RAM but is not accessible via normal
> mechanims, including DMA.
> 
> See 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
> 
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> 
> cc: peter.maydell@linaro.org
> cc: marcandre.lureau@redhat.com
> cc: eric.auger@redhat.com
> cc: mst@redhat.com
> cc: jasowang@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   hw/tpm/tpm_crb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index ea930da545af..0a93c488f2fa 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -296,7 +296,7 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>   
>       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),
> +    memory_region_init_ram_protected(&s->cmdmem, OBJECT(s),
>           "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
>   
>       memory_region_add_subregion(get_system_memory(),

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[Peter Maydell]

On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> wrote:
>
> This memory is not correctly aligned and cannot be registered
> by vDPA and VFIO.

Isn't this a vDPA/VFIO problem? There's no requirement
for RAM MemoryRegions to be aligned in any way. Code
that doesn't want to work with small or weirdly aligned
regions should skip them if that's the right behaviour
for that particular code IMHO.

> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
>
> To make it ignored by VFIO and vDPA devices, mark it as RAM_PROTECTED.
>
> The RAM_PROTECTED flag has been introduced to skip memory
> region that looks like RAM but is not accessible via normal
> mechanims, including DMA.

You can DMA to a small RAM region if you want to...

thanks
-- PMM

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[Laurent Vivier]

On 6/22/23 15:12, Peter Maydell wrote:
> On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> wrote:
>>
>> This memory is not correctly aligned and cannot be registered
>> by vDPA and VFIO.
> 
> Isn't this a vDPA/VFIO problem? There's no requirement
> for RAM MemoryRegions to be aligned in any way. Code
> that doesn't want to work with small or weirdly aligned
> regions should skip them if that's the right behaviour
> for that particular code IMHO.
> 

Marc-André proposed to modify vDPA code to skip the region but Michal disagreed:

https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html

No one wants the modification, so the problem cannot be fixed.

Thanks,
Laurent

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[Peter Maydell]

On Thu, 22 Jun 2023 at 14:39, Laurent Vivier <lvivier@redhat.com> wrote:
>
> On 6/22/23 15:12, Peter Maydell wrote:
> > On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> wrote:
> >>
> >> This memory is not correctly aligned and cannot be registered
> >> by vDPA and VFIO.
> >
> > Isn't this a vDPA/VFIO problem? There's no requirement
> > for RAM MemoryRegions to be aligned in any way. Code
> > that doesn't want to work with small or weirdly aligned
> > regions should skip them if that's the right behaviour
> > for that particular code IMHO.
> >
>
> Marc-André proposed to modify vDPA code to skip the region but Michal disagreed:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html

"Special case the TPM device in the vhost-vdpa" is clearly also
not the right way to solve the issue, so I agree with Michael
about that.

The vhost-vdpa code already seems able to correctly detect
whether a region is unaligned and ignores it. If that's the
right thing to do, maybe we should just remove the
error_report() ? Listeners are going to see MemoryRegions
which are RAM and which aren't necessarily page-aligned,
so they should handle them, not complain about them.

thanks
-- PMM

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[Jason Wang]

On Thu, Jun 22, 2023 at 9:39 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> On 6/22/23 15:12, Peter Maydell wrote:
> > On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> wrote:
> >>
> >> This memory is not correctly aligned and cannot be registered
> >> by vDPA and VFIO.
> >
> > Isn't this a vDPA/VFIO problem? There's no requirement
> > for RAM MemoryRegions to be aligned in any way.

It's more about the limitation of the IOMMU which can't do subpage protection.

> > Code
> > that doesn't want to work with small or weirdly aligned
> > regions should skip them if that's the right behaviour
> > for that particular code IMHO.

We had already had this:

    if ((!memory_region_is_ram(section->mr) &&
         !memory_region_is_iommu(section->mr)) ||
        memory_region_is_protected(section->mr) ||
        /* vhost-vDPA doesn't allow MMIO to be mapped  */
        memory_region_is_ram_device(section->mr)) {
        return true;
    }

> >
>
> Marc-André proposed to modify vDPA code to skip the region but Michal disagreed:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html
>
> No one wants the modification, so the problem cannot be fixed.
>

Yes, otherwise we end up with explicit check for TPM crb in vhost code...

Thanks

> Thanks,
> Laurent
>

Re: [PATCH 2/2] tpm_crb: mark memory as protected

[Laurent Vivier]

Hi,

as the region is already skipped by the test of the memory region alignment, I'm going to 
update my patches by only removing the error_report() as proposed by Peter.

I will replace it by a trace to help to debug.

Thanks,
Laurent

On 7/4/23 05:07, Jason Wang wrote:
> On Thu, Jun 22, 2023 at 9:39 PM Laurent Vivier <lvivier@redhat.com> wrote:
>>
>> On 6/22/23 15:12, Peter Maydell wrote:
>>> On Tue, 20 Jun 2023 at 20:51, Laurent Vivier <lvivier@redhat.com> wrote:
>>>>
>>>> This memory is not correctly aligned and cannot be registered
>>>> by vDPA and VFIO.
>>>
>>> Isn't this a vDPA/VFIO problem? There's no requirement
>>> for RAM MemoryRegions to be aligned in any way.
> 
> It's more about the limitation of the IOMMU which can't do subpage protection.
> 
>>> Code
>>> that doesn't want to work with small or weirdly aligned
>>> regions should skip them if that's the right behaviour
>>> for that particular code IMHO.
> 
> We had already had this:
> 
>      if ((!memory_region_is_ram(section->mr) &&
>           !memory_region_is_iommu(section->mr)) ||
>          memory_region_is_protected(section->mr) ||
>          /* vhost-vDPA doesn't allow MMIO to be mapped  */
>          memory_region_is_ram_device(section->mr)) {
>          return true;
>      }
> 
>>>
>>
>> Marc-André proposed to modify vDPA code to skip the region but Michal disagreed:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html
>>
>> No one wants the modification, so the problem cannot be fixed.
>>
> 
> Yes, otherwise we end up with explicit check for TPM crb in vhost code...
> 
> Thanks
> 
>> Thanks,
>> Laurent
>>
> 

Re: [PATCH 0/2] vhost-vdpa: skip TPM CRB memory section

[Peter Xu]

On Tue, Jun 20, 2023 at 09:50:52PM +0200, Laurent Vivier wrote:
> An error is reported for vhost-vdpa case:
> qemu-kvm: vhost_vdpa_listener_region_add received unaligned region
> 
> Marc-André has proposed a fix to this problem by skipping
> the memory region owned by the TPM CRB but it seems more generic
> to skip not DMA-able memory.
> 
> We have a memory flag for that, RAM_PROTECTED.
> 
> This series expands the memory API to provide a way to initialize
> a "protected" memory region and use it with the TPM CRB object.
> 
> For the previous discussions, see
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03670.html
> 
> and from Eric for VFIO:
> 
> https://lore.kernel.org/all/20220506132510.1847942-1-eric.auger@redhat.com/
> https://lore.kernel.org/all/20220524091405.416256-1-eric.auger@redhat.com/
> 
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2141965
> 
> Thanks,
> Laurent
> 
> Laurent Vivier (2):
>   memory: introduce memory_region_init_ram_protected()
>   tpm_crb: mark memory as protected

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu