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

[PATCH v2 0/1] 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 aligned memory.

v1 of this series proposed to set the RAM_PROTECTED flag for the
TPM CRB memory region.

v2:
  - do not introduce special case for TPM CRB
  - do not set RAM_PROTECTED flag for TPM CRB
  - remove error_report() and replace it with a trace

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 (1):
  vhost-vdpa: mute unaligned memory error report

 hw/virtio/trace-events | 2 ++
 hw/virtio/vhost-vdpa.c | 8 ++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.41.0

[PATCH v2 1/1] vhost-vdpa: mute unaligned memory error report

[Laurent Vivier]

With TPM CRM device, vhost-vdpa reports an error when it tries
to register a listener for a non aligned memory region:

  qemu-system-x86_64: vhost_vdpa_listener_region_add received unaligned region
  qemu-system-x86_64: vhost_vdpa_listener_region_del received unaligned region

This error can be confusing for the user whereas we only need to skip
the region (as it's already done after the error_report())

Rather than introducing a special case for TPM CRB memory section
to not display the message in this case, simply replace the
error_report() by a trace function (with more information, like the
memory region name).

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/virtio/trace-events | 2 ++
 hw/virtio/vhost-vdpa.c | 8 ++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 8f8d05cf9b01..9b0d643b9475 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -34,7 +34,9 @@ vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_
 vhost_vdpa_dma_unmap(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_t iova, uint64_t size, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" asid: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" type: %"PRIu8
 vhost_vdpa_listener_begin_batch(void *v, int fd, uint32_t msg_type, uint8_t type)  "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
 vhost_vdpa_listener_commit(void *v, int fd, uint32_t msg_type, uint8_t type)  "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
+vhost_vdpa_listener_region_add_unaligned(void *v, const char *name, uint64_t offset_as, uint64_t offset_page) "vdpa: %p region %s offset_within_address_space %"PRIu64" offset_within_region %"PRIu64
 vhost_vdpa_listener_region_add(void *vdpa, uint64_t iova, uint64_t llend, void *vaddr, bool readonly) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64" vaddr: %p read-only: %d"
+vhost_vdpa_listener_region_del_unaligned(void *v, const char *name, uint64_t offset_as, uint64_t offset_page) "vdpa: %p region %s offset_within_address_space %"PRIu64" offset_within_region %"PRIu64
 vhost_vdpa_listener_region_del(void *vdpa, uint64_t iova, uint64_t llend) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64
 vhost_vdpa_add_status(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
 vhost_vdpa_init(void *dev, void *vdpa) "dev: %p vdpa: %p"
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3c575a9a6e9e..24d32f0d3728 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -323,7 +323,9 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
 
     if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
                  (section->offset_within_region & ~TARGET_PAGE_MASK))) {
-        error_report("%s received unaligned region", __func__);
+        trace_vhost_vdpa_listener_region_add_unaligned(v, section->mr->name,
+                       section->offset_within_address_space & ~TARGET_PAGE_MASK,
+                       section->offset_within_region & ~TARGET_PAGE_MASK);
         return;
     }
 
@@ -405,7 +407,9 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
 
     if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
                  (section->offset_within_region & ~TARGET_PAGE_MASK))) {
-        error_report("%s received unaligned region", __func__);
+        trace_vhost_vdpa_listener_region_del_unaligned(v, section->mr->name,
+                       section->offset_within_address_space & ~TARGET_PAGE_MASK,
+                       section->offset_within_region & ~TARGET_PAGE_MASK);
         return;
     }
 
-- 
2.41.0

Re: [PATCH v2 1/1] vhost-vdpa: mute unaligned memory error report

[David Hildenbrand]

On 04.07.23 09:19, Laurent Vivier wrote:
> With TPM CRM device, vhost-vdpa reports an error when it tries
> to register a listener for a non aligned memory region:
> 
>    qemu-system-x86_64: vhost_vdpa_listener_region_add received unaligned region
>    qemu-system-x86_64: vhost_vdpa_listener_region_del received unaligned region
> 
> This error can be confusing for the user whereas we only need to skip
> the region (as it's already done after the error_report())
> 
> Rather than introducing a special case for TPM CRB memory section
> to not display the message in this case, simply replace the
> error_report() by a trace function (with more information, like the
> memory region name).
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   hw/virtio/trace-events | 2 ++
>   hw/virtio/vhost-vdpa.c | 8 ++++++--
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 8f8d05cf9b01..9b0d643b9475 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -34,7 +34,9 @@ vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_
>   vhost_vdpa_dma_unmap(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_t iova, uint64_t size, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" asid: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" type: %"PRIu8
>   vhost_vdpa_listener_begin_batch(void *v, int fd, uint32_t msg_type, uint8_t type)  "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
>   vhost_vdpa_listener_commit(void *v, int fd, uint32_t msg_type, uint8_t type)  "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
> +vhost_vdpa_listener_region_add_unaligned(void *v, const char *name, uint64_t offset_as, uint64_t offset_page) "vdpa: %p region %s offset_within_address_space %"PRIu64" offset_within_region %"PRIu64
>   vhost_vdpa_listener_region_add(void *vdpa, uint64_t iova, uint64_t llend, void *vaddr, bool readonly) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64" vaddr: %p read-only: %d"
> +vhost_vdpa_listener_region_del_unaligned(void *v, const char *name, uint64_t offset_as, uint64_t offset_page) "vdpa: %p region %s offset_within_address_space %"PRIu64" offset_within_region %"PRIu64
>   vhost_vdpa_listener_region_del(void *vdpa, uint64_t iova, uint64_t llend) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64
>   vhost_vdpa_add_status(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
>   vhost_vdpa_init(void *dev, void *vdpa) "dev: %p vdpa: %p"
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 3c575a9a6e9e..24d32f0d3728 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -323,7 +323,9 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
>   
>       if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
>                    (section->offset_within_region & ~TARGET_PAGE_MASK))) {
> -        error_report("%s received unaligned region", __func__);
> +        trace_vhost_vdpa_listener_region_add_unaligned(v, section->mr->name,
> +                       section->offset_within_address_space & ~TARGET_PAGE_MASK,
> +                       section->offset_within_region & ~TARGET_PAGE_MASK);
>           return;
>       }
>   
> @@ -405,7 +407,9 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
>   
>       if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
>                    (section->offset_within_region & ~TARGET_PAGE_MASK))) {
> -        error_report("%s received unaligned region", __func__);
> +        trace_vhost_vdpa_listener_region_del_unaligned(v, section->mr->name,
> +                       section->offset_within_address_space & ~TARGET_PAGE_MASK,
> +                       section->offset_within_region & ~TARGET_PAGE_MASK);
>           return;
>       }
>   

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


Do we also want to touch the vfio side in vfio_listener_valid_section(), 
or why is that one unaffected?

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/1] vhost-vdpa: mute unaligned memory error report

[Laurent Vivier]

On 7/4/23 09:25, David Hildenbrand wrote:
> On 04.07.23 09:19, Laurent Vivier wrote:
>> With TPM CRM device, vhost-vdpa reports an error when it tries
>> to register a listener for a non aligned memory region:
>>
>>    qemu-system-x86_64: vhost_vdpa_listener_region_add received unaligned region
>>    qemu-system-x86_64: vhost_vdpa_listener_region_del received unaligned region
>>
>> This error can be confusing for the user whereas we only need to skip
>> the region (as it's already done after the error_report())
>>
>> Rather than introducing a special case for TPM CRB memory section
>> to not display the message in this case, simply replace the
>> error_report() by a trace function (with more information, like the
>> memory region name).
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   hw/virtio/trace-events | 2 ++
>>   hw/virtio/vhost-vdpa.c | 8 ++++++--
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>> index 8f8d05cf9b01..9b0d643b9475 100644
>> --- a/hw/virtio/trace-events
>> +++ b/hw/virtio/trace-events
>> @@ -34,7 +34,9 @@ vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint32_t 
>> asid, uint64_
>>   vhost_vdpa_dma_unmap(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_t 
>> iova, uint64_t size, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" asid: %"PRIu32" 
>> iova: 0x%"PRIx64" size: 0x%"PRIx64" type: %"PRIu8
>>   vhost_vdpa_listener_begin_batch(void *v, int fd, uint32_t msg_type, uint8_t type)  
>> "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
>>   vhost_vdpa_listener_commit(void *v, int fd, uint32_t msg_type, uint8_t type)  "vdpa:%p 
>> fd: %d msg_type: %"PRIu32" type: %"PRIu8
>> +vhost_vdpa_listener_region_add_unaligned(void *v, const char *name, uint64_t offset_as, 
>> uint64_t offset_page) "vdpa: %p region %s offset_within_address_space %"PRIu64" 
>> offset_within_region %"PRIu64
>>   vhost_vdpa_listener_region_add(void *vdpa, uint64_t iova, uint64_t llend, void *vaddr, 
>> bool readonly) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64" vaddr: %p read-only: %d"
>> +vhost_vdpa_listener_region_del_unaligned(void *v, const char *name, uint64_t offset_as, 
>> uint64_t offset_page) "vdpa: %p region %s offset_within_address_space %"PRIu64" 
>> offset_within_region %"PRIu64
>>   vhost_vdpa_listener_region_del(void *vdpa, uint64_t iova, uint64_t llend) "vdpa: %p 
>> iova 0x%"PRIx64" llend 0x%"PRIx64
>>   vhost_vdpa_add_status(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
>>   vhost_vdpa_init(void *dev, void *vdpa) "dev: %p vdpa: %p"
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 3c575a9a6e9e..24d32f0d3728 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -323,7 +323,9 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
>>       if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
>>                    (section->offset_within_region & ~TARGET_PAGE_MASK))) {
>> -        error_report("%s received unaligned region", __func__);
>> +        trace_vhost_vdpa_listener_region_add_unaligned(v, section->mr->name,
>> +                       section->offset_within_address_space & ~TARGET_PAGE_MASK,
>> +                       section->offset_within_region & ~TARGET_PAGE_MASK);
>>           return;
>>       }
>> @@ -405,7 +407,9 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
>>       if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
>>                    (section->offset_within_region & ~TARGET_PAGE_MASK))) {
>> -        error_report("%s received unaligned region", __func__);
>> +        trace_vhost_vdpa_listener_region_del_unaligned(v, section->mr->name,
>> +                       section->offset_within_address_space & ~TARGET_PAGE_MASK,
>> +                       section->offset_within_region & ~TARGET_PAGE_MASK);
>>           return;
>>       }
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> 
> Do we also want to touch the vfio side in vfio_listener_valid_section(), or why is that 
> one unaffected?
> 

I don't know if we can apply the same solution for VFIO.
I don't know if the error message is relevant or if we can keep only the trace and remove 
the error_report() for all the cases (in this case vfio_known_safe_misalignment() becomes 
useless).

Thanks,
Laurent

Re: [PATCH v2 1/1] vhost-vdpa: mute unaligned memory error report

[Jason Wang]

On Tue, Jul 4, 2023 at 3:19 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> With TPM CRM device, vhost-vdpa reports an error when it tries
> to register a listener for a non aligned memory region:
>
>   qemu-system-x86_64: vhost_vdpa_listener_region_add received unaligned region
>   qemu-system-x86_64: vhost_vdpa_listener_region_del received unaligned region
>
> This error can be confusing for the user whereas we only need to skip
> the region (as it's already done after the error_report())
>
> Rather than introducing a special case for TPM CRB memory section
> to not display the message in this case, simply replace the
> error_report() by a trace function (with more information, like the
> memory region name).
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  hw/virtio/trace-events | 2 ++
>  hw/virtio/vhost-vdpa.c | 8 ++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 8f8d05cf9b01..9b0d643b9475 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -34,7 +34,9 @@ vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_
>  vhost_vdpa_dma_unmap(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_t iova, uint64_t size, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" asid: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" type: %"PRIu8
>  vhost_vdpa_listener_begin_batch(void *v, int fd, uint32_t msg_type, uint8_t type)  "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
>  vhost_vdpa_listener_commit(void *v, int fd, uint32_t msg_type, uint8_t type)  "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
> +vhost_vdpa_listener_region_add_unaligned(void *v, const char *name, uint64_t offset_as, uint64_t offset_page) "vdpa: %p region %s offset_within_address_space %"PRIu64" offset_within_region %"PRIu64
>  vhost_vdpa_listener_region_add(void *vdpa, uint64_t iova, uint64_t llend, void *vaddr, bool readonly) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64" vaddr: %p read-only: %d"
> +vhost_vdpa_listener_region_del_unaligned(void *v, const char *name, uint64_t offset_as, uint64_t offset_page) "vdpa: %p region %s offset_within_address_space %"PRIu64" offset_within_region %"PRIu64
>  vhost_vdpa_listener_region_del(void *vdpa, uint64_t iova, uint64_t llend) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64
>  vhost_vdpa_add_status(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
>  vhost_vdpa_init(void *dev, void *vdpa) "dev: %p vdpa: %p"
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 3c575a9a6e9e..24d32f0d3728 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -323,7 +323,9 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
>
>      if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
>                   (section->offset_within_region & ~TARGET_PAGE_MASK))) {
> -        error_report("%s received unaligned region", __func__);
> +        trace_vhost_vdpa_listener_region_add_unaligned(v, section->mr->name,
> +                       section->offset_within_address_space & ~TARGET_PAGE_MASK,
> +                       section->offset_within_region & ~TARGET_PAGE_MASK);
>          return;
>      }
>
> @@ -405,7 +407,9 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
>
>      if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
>                   (section->offset_within_region & ~TARGET_PAGE_MASK))) {
> -        error_report("%s received unaligned region", __func__);
> +        trace_vhost_vdpa_listener_region_del_unaligned(v, section->mr->name,
> +                       section->offset_within_address_space & ~TARGET_PAGE_MASK,
> +                       section->offset_within_region & ~TARGET_PAGE_MASK);
>          return;
>      }
>
> --
> 2.41.0
>