diff mbox series

[v4] vfio/common: remove spurious tpm-crb-cmd misalignment warning

Message ID 20220428134945.511829-1-eric.auger@redhat.com
State New
Headers show
Series [v4] vfio/common: remove spurious tpm-crb-cmd misalignment warning | expand

Commit Message

Eric Auger April 28, 2022, 1:49 p.m. UTC 
The CRB command buffer currently is a RAM MemoryRegion and given
its base address alignment, it causes an error report on
vfio_listener_region_add(). This region could have been a RAM device
region, easing the detection of such safe situation but this option
was not well received. So let's add a helper function that uses the
memory region owner type to detect the situation is safe wrt
the assignment. Other device types can be checked here if such kind
of problem occurs again.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

---

v3 -> v4:
- rebase on top of qemu_real_host_page_size() and
  qemu_real_host_page_size(). Print the size and make the message
  consistent
- Added Stefan's A-b and Connie R-b (despite the changes)
---
 hw/vfio/common.c     | 27 ++++++++++++++++++++++++++-
 hw/vfio/trace-events |  1 +
 2 files changed, 27 insertions(+), 1 deletion(-)

Comments

Cornelia Huck April 28, 2022, 1:58 p.m. UTC | #1 
On Thu, Apr 28 2022, Eric Auger <eric.auger@redhat.com> wrote:

> The CRB command buffer currently is a RAM MemoryRegion and given
> its base address alignment, it causes an error report on
> vfio_listener_region_add(). This region could have been a RAM device
> region, easing the detection of such safe situation but this option
> was not well received. So let's add a helper function that uses the
> memory region owner type to detect the situation is safe wrt
> the assignment. Other device types can be checked here if such kind
> of problem occurs again.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Acked-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> ---
>
> v3 -> v4:
> - rebase on top of qemu_real_host_page_size() and
>   qemu_real_host_page_size(). Print the size and make the message
>   consistent
> - Added Stefan's A-b and Connie R-b (despite the changes)

Still fine with me.

> ---
>  hw/vfio/common.c     | 27 ++++++++++++++++++++++++++-
>  hw/vfio/trace-events |  1 +
>  2 files changed, 27 insertions(+), 1 deletion(-)
Alex Williamson April 28, 2022, 8:14 p.m. UTC | #2 
On Thu, 28 Apr 2022 15:49:45 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> The CRB command buffer currently is a RAM MemoryRegion and given
> its base address alignment, it causes an error report on
> vfio_listener_region_add(). This region could have been a RAM device
> region, easing the detection of such safe situation but this option
> was not well received. So let's add a helper function that uses the
> memory region owner type to detect the situation is safe wrt
> the assignment. Other device types can be checked here if such kind
> of problem occurs again.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Acked-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> ---
> 
> v3 -> v4:
> - rebase on top of qemu_real_host_page_size() and
>   qemu_real_host_page_size(). Print the size and make the message
>   consistent
> - Added Stefan's A-b and Connie R-b (despite the changes)
> ---
>  hw/vfio/common.c     | 27 ++++++++++++++++++++++++++-
>  hw/vfio/trace-events |  1 +
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 2b1f78fdfa..f6b9bb6d71 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -40,6 +40,7 @@
>  #include "trace.h"
>  #include "qapi/error.h"
>  #include "migration/migration.h"
> +#include "sysemu/tpm.h"
>  
>  VFIOGroupList vfio_group_list =
>      QLIST_HEAD_INITIALIZER(vfio_group_list);
> @@ -861,6 +862,22 @@ static void vfio_unregister_ram_discard_listener(VFIOContainer *container,
>      g_free(vrdl);
>  }
>  
> +static bool vfio_known_safe_misalignment(MemoryRegionSection *section)
> +{
> +    MemoryRegion *mr = section->mr;
> +
> +    if (!TPM_IS_CRB(mr->owner)) {
> +        return false;
> +    }

It looks like this test is going to need to be wrapped in #ifdef
CONFIG_TPM:

https://gitlab.com/alex.williamson/qemu/-/jobs/2391952412

Thanks,

Alex

> +
> +    /* this is a known safe misaligned region, just trace for debug purpose */
> +    trace_vfio_known_safe_misalignment(memory_region_name(mr),
> +                                       section->offset_within_address_space,
> +                                       section->offset_within_region,
> +                                       qemu_real_host_page_size());
> +    return true;
> +}
> +
>  static void vfio_listener_region_add(MemoryListener *listener,
>                                       MemoryRegionSection *section)
>  {
> @@ -884,7 +901,15 @@ static void vfio_listener_region_add(MemoryListener *listener,
>      if (unlikely((section->offset_within_address_space &
>                    ~qemu_real_host_page_mask()) !=
>                   (section->offset_within_region & ~qemu_real_host_page_mask()))) {
> -        error_report("%s received unaligned region", __func__);
> +        if (!vfio_known_safe_misalignment(section)) {
> +            error_report("%s received unaligned region %s iova=0x%"PRIx64
> +                         " offset_within_region=0x%"PRIx64
> +                         " qemu_real_host_page_size=0x%"PRIxPTR,
> +                         __func__, memory_region_name(section->mr),
> +                         section->offset_within_address_space,
> +                         section->offset_within_region,
> +                         qemu_real_host_page_size());
> +        }
>          return;
>      }
>  
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index 0ef1b5f4a6..582882db91 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -100,6 +100,7 @@ vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add
>  vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d"
>  vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add [iommu] 0x%"PRIx64" - 0x%"PRIx64
>  vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]"
> +vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t offset_within_region, uintptr_t page_size) "Region \"%s\" iova=0x%"PRIx64" offset_within_region=0x%"PRIx64" qemu_real_host_page_size=0x%"PRIxPTR ": cannot be mapped for DMA"
>  vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA"
>  vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
>  vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64
Eric Auger May 6, 2022, 7:05 a.m. UTC | #3 
Hi Alex,

On 4/28/22 22:14, Alex Williamson wrote:
> On Thu, 28 Apr 2022 15:49:45 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> The CRB command buffer currently is a RAM MemoryRegion and given
>> its base address alignment, it causes an error report on
>> vfio_listener_region_add(). This region could have been a RAM device
>> region, easing the detection of such safe situation but this option
>> was not well received. So let's add a helper function that uses the
>> memory region owner type to detect the situation is safe wrt
>> the assignment. Other device types can be checked here if such kind
>> of problem occurs again.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Acked-by: Stefan Berger <stefanb@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>
>> ---
>>
>> v3 -> v4:
>> - rebase on top of qemu_real_host_page_size() and
>>   qemu_real_host_page_size(). Print the size and make the message
>>   consistent
>> - Added Stefan's A-b and Connie R-b (despite the changes)
>> ---
>>  hw/vfio/common.c     | 27 ++++++++++++++++++++++++++-
>>  hw/vfio/trace-events |  1 +
>>  2 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 2b1f78fdfa..f6b9bb6d71 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -40,6 +40,7 @@
>>  #include "trace.h"
>>  #include "qapi/error.h"
>>  #include "migration/migration.h"
>> +#include "sysemu/tpm.h"
>>  
>>  VFIOGroupList vfio_group_list =
>>      QLIST_HEAD_INITIALIZER(vfio_group_list);
>> @@ -861,6 +862,22 @@ static void vfio_unregister_ram_discard_listener(VFIOContainer *container,
>>      g_free(vrdl);
>>  }
>>  
>> +static bool vfio_known_safe_misalignment(MemoryRegionSection *section)
>> +{
>> +    MemoryRegion *mr = section->mr;
>> +
>> +    if (!TPM_IS_CRB(mr->owner)) {
>> +        return false;
>> +    }
> It looks like this test is going to need to be wrapped in #ifdef
> CONFIG_TPM:

sorry for the delay. Your message fell though the cracks :-(

if I put an '#ifdef CONFIG_TPM' I need to inverse the logic because by
default the function shall return false.

solution #1

#ifdef CONFIG_TPM  
 if (TPM_IS_CRB(mr->owner)) {

    /* this is a known safe misaligned region, just trace for debug purpose */
    trace_vfio_known_safe_misalignment(memory_region_name(mr),
                                       section->offset_within_address_space,
                                       section->offset_within_region,
                                       qemu_real_host_page_size());

    return true;
   }

#endif
return false;

This looks weird to me.

+    if (!object_dynamic_cast(mr->owner, TYPE_TPM_CRB)) {
+        return false;
+    }


solution #2
replace !object_dynamic_cast(mr->owner, TYPE_TPM_CRB) by
!object_dynamic_cast(mr->owner, "tpm-crb")
and add a comment saying that we don't use TYPE_TPM_CRB on purpose

solution #3
Move #define TPM_IS_CRB(chr) and related defined out of
#ifdef CONFIG_TPM hoping it does not have other side effects

Thoughts?
Eric
>
> https://gitlab.com/alex.williamson/qemu/-/jobs/2391952412
>
> Thanks,
>
> Alex
>
>> +
>> +    /* this is a known safe misaligned region, just trace for debug purpose */
>> +    trace_vfio_known_safe_misalignment(memory_region_name(mr),
>> +                                       section->offset_within_address_space,
>> +                                       section->offset_within_region,
>> +                                       qemu_real_host_page_size());
>> +    return true;
>> +}
>> +
>>  static void vfio_listener_region_add(MemoryListener *listener,
>>                                       MemoryRegionSection *section)
>>  {
>> @@ -884,7 +901,15 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>      if (unlikely((section->offset_within_address_space &
>>                    ~qemu_real_host_page_mask()) !=
>>                   (section->offset_within_region & ~qemu_real_host_page_mask()))) {
>> -        error_report("%s received unaligned region", __func__);
>> +        if (!vfio_known_safe_misalignment(section)) {
>> +            error_report("%s received unaligned region %s iova=0x%"PRIx64
>> +                         " offset_within_region=0x%"PRIx64
>> +                         " qemu_real_host_page_size=0x%"PRIxPTR,
>> +                         __func__, memory_region_name(section->mr),
>> +                         section->offset_within_address_space,
>> +                         section->offset_within_region,
>> +                         qemu_real_host_page_size());
>> +        }
>>          return;
>>      }
>>  
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index 0ef1b5f4a6..582882db91 100644
>> --- a/hw/vfio/trace-events
>> +++ b/hw/vfio/trace-events
>> @@ -100,6 +100,7 @@ vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add
>>  vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d"
>>  vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add [iommu] 0x%"PRIx64" - 0x%"PRIx64
>>  vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]"
>> +vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t offset_within_region, uintptr_t page_size) "Region \"%s\" iova=0x%"PRIx64" offset_within_region=0x%"PRIx64" qemu_real_host_page_size=0x%"PRIxPTR ": cannot be mapped for DMA"
>>  vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA"
>>  vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
>>  vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64
Cornelia Huck May 6, 2022, 7:34 a.m. UTC | #4 
On Fri, May 06 2022, Eric Auger <eric.auger@redhat.com> wrote:

> Hi Alex,
>
> On 4/28/22 22:14, Alex Williamson wrote:
>> On Thu, 28 Apr 2022 15:49:45 +0200
>> Eric Auger <eric.auger@redhat.com> wrote:

>>> +static bool vfio_known_safe_misalignment(MemoryRegionSection *section)
>>> +{
>>> +    MemoryRegion *mr = section->mr;
>>> +
>>> +    if (!TPM_IS_CRB(mr->owner)) {
>>> +        return false;
>>> +    }
>> It looks like this test is going to need to be wrapped in #ifdef
>> CONFIG_TPM:
>
> sorry for the delay. Your message fell though the cracks :-(
>
> if I put an '#ifdef CONFIG_TPM' I need to inverse the logic because by
> default the function shall return false.
>
> solution #1
>
> #ifdef CONFIG_TPM  
>  if (TPM_IS_CRB(mr->owner)) {
>
>     /* this is a known safe misaligned region, just trace for debug purpose */
>     trace_vfio_known_safe_misalignment(memory_region_name(mr),
>                                        section->offset_within_address_space,
>                                        section->offset_within_region,
>                                        qemu_real_host_page_size());
>
>     return true;
>    }
>
> #endif
> return false;
>
> This looks weird to me.
>
> +    if (!object_dynamic_cast(mr->owner, TYPE_TPM_CRB)) {
> +        return false;
> +    }
>
>
> solution #2
> replace !object_dynamic_cast(mr->owner, TYPE_TPM_CRB) by
> !object_dynamic_cast(mr->owner, "tpm-crb")
> and add a comment saying that we don't use TYPE_TPM_CRB on purpose
>
> solution #3
> Move #define TPM_IS_CRB(chr) and related defined out of
> #ifdef CONFIG_TPM hoping it does not have other side effects
>
> Thoughts?
> Eric

solution #4

#ifndef CONFIG_TPM
/* needed for an alignment check in non-tpm code */
static inline Object *TPM_IS_CRB(Object *obj)
{
    return NULL;
}
#endif

I think it would be good if we could hide the configuration details in
the header.
Eric Auger May 6, 2022, 7:49 a.m. UTC | #5 
Hi Connie,

On 5/6/22 09:34, Cornelia Huck wrote:
> On Fri, May 06 2022, Eric Auger <eric.auger@redhat.com> wrote:
>
>> Hi Alex,
>>
>> On 4/28/22 22:14, Alex Williamson wrote:
>>> On Thu, 28 Apr 2022 15:49:45 +0200
>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>> +static bool vfio_known_safe_misalignment(MemoryRegionSection *section)
>>>> +{
>>>> +    MemoryRegion *mr = section->mr;
>>>> +
>>>> +    if (!TPM_IS_CRB(mr->owner)) {
>>>> +        return false;
>>>> +    }
>>> It looks like this test is going to need to be wrapped in #ifdef
>>> CONFIG_TPM:
>> sorry for the delay. Your message fell though the cracks :-(
>>
>> if I put an '#ifdef CONFIG_TPM' I need to inverse the logic because by
>> default the function shall return false.
>>
>> solution #1
>>
>> #ifdef CONFIG_TPM  
>>  if (TPM_IS_CRB(mr->owner)) {
>>
>>     /* this is a known safe misaligned region, just trace for debug purpose */
>>     trace_vfio_known_safe_misalignment(memory_region_name(mr),
>>                                        section->offset_within_address_space,
>>                                        section->offset_within_region,
>>                                        qemu_real_host_page_size());
>>
>>     return true;
>>    }
>>
>> #endif
>> return false;
>>
>> This looks weird to me.
>>
>> +    if (!object_dynamic_cast(mr->owner, TYPE_TPM_CRB)) {
>> +        return false;
>> +    }
>>
>>
>> solution #2
>> replace !object_dynamic_cast(mr->owner, TYPE_TPM_CRB) by
>> !object_dynamic_cast(mr->owner, "tpm-crb")
>> and add a comment saying that we don't use TYPE_TPM_CRB on purpose
>>
>> solution #3
>> Move #define TPM_IS_CRB(chr) and related defined out of
>> #ifdef CONFIG_TPM hoping it does not have other side effects
>>
>> Thoughts?
>> Eric
> solution #4
>
> #ifndef CONFIG_TPM
> /* needed for an alignment check in non-tpm code */
> static inline Object *TPM_IS_CRB(Object *obj)
> {
>     return NULL;
> }
> #endif
>
> I think it would be good if we could hide the configuration details in
> the header.
>
Yep, I forgot to mention solution #3 also happened in include/sysemu/tpm.h.
Connie, either we add your stub function or we move the following out of
the #ifdef CONFIG_TPM. This should be harmless, no?
Stefan, any preference?

#define TYPE_TPM_TIS_ISA            "tpm-tis"
#define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
#define TYPE_TPM_CRB                "tpm-crb"
#define TYPE_TPM_SPAPR              "tpm-spapr"

#define TPM_IS_TIS_ISA(chr)                         \
    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
#define TPM_IS_TIS_SYSBUS(chr)                      \
    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
#define TPM_IS_CRB(chr)                             \
    object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
#define TPM_IS_SPAPR(chr)                           \
    object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)

Eric
Cornelia Huck May 6, 2022, 7:56 a.m. UTC | #6 
On Fri, May 06 2022, Eric Auger <eric.auger@redhat.com> wrote:

> Hi Connie,
>
> On 5/6/22 09:34, Cornelia Huck wrote:
>> On Fri, May 06 2022, Eric Auger <eric.auger@redhat.com> wrote:
>>
>>> Hi Alex,
>>>
>>> On 4/28/22 22:14, Alex Williamson wrote:
>>>> On Thu, 28 Apr 2022 15:49:45 +0200
>>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>>> +static bool vfio_known_safe_misalignment(MemoryRegionSection *section)
>>>>> +{
>>>>> +    MemoryRegion *mr = section->mr;
>>>>> +
>>>>> +    if (!TPM_IS_CRB(mr->owner)) {
>>>>> +        return false;
>>>>> +    }
>>>> It looks like this test is going to need to be wrapped in #ifdef
>>>> CONFIG_TPM:
>>> sorry for the delay. Your message fell though the cracks :-(
>>>
>>> if I put an '#ifdef CONFIG_TPM' I need to inverse the logic because by
>>> default the function shall return false.
>>>
>>> solution #1
>>>
>>> #ifdef CONFIG_TPM  
>>>  if (TPM_IS_CRB(mr->owner)) {
>>>
>>>     /* this is a known safe misaligned region, just trace for debug purpose */
>>>     trace_vfio_known_safe_misalignment(memory_region_name(mr),
>>>                                        section->offset_within_address_space,
>>>                                        section->offset_within_region,
>>>                                        qemu_real_host_page_size());
>>>
>>>     return true;
>>>    }
>>>
>>> #endif
>>> return false;
>>>
>>> This looks weird to me.
>>>
>>> +    if (!object_dynamic_cast(mr->owner, TYPE_TPM_CRB)) {
>>> +        return false;
>>> +    }
>>>
>>>
>>> solution #2
>>> replace !object_dynamic_cast(mr->owner, TYPE_TPM_CRB) by
>>> !object_dynamic_cast(mr->owner, "tpm-crb")
>>> and add a comment saying that we don't use TYPE_TPM_CRB on purpose
>>>
>>> solution #3
>>> Move #define TPM_IS_CRB(chr) and related defined out of
>>> #ifdef CONFIG_TPM hoping it does not have other side effects
>>>
>>> Thoughts?
>>> Eric
>> solution #4
>>
>> #ifndef CONFIG_TPM
>> /* needed for an alignment check in non-tpm code */
>> static inline Object *TPM_IS_CRB(Object *obj)
>> {
>>     return NULL;
>> }
>> #endif
>>
>> I think it would be good if we could hide the configuration details in
>> the header.
>>
> Yep, I forgot to mention solution #3 also happened in include/sysemu/tpm.h.
> Connie, either we add your stub function or we move the following out of
> the #ifdef CONFIG_TPM. This should be harmless, no?
> Stefan, any preference?
>
> #define TYPE_TPM_TIS_ISA            "tpm-tis"
> #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
> #define TYPE_TPM_CRB                "tpm-crb"
> #define TYPE_TPM_SPAPR              "tpm-spapr"
>
> #define TPM_IS_TIS_ISA(chr)                         \
>     object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
> #define TPM_IS_TIS_SYSBUS(chr)                      \
>     object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> #define TPM_IS_CRB(chr)                             \
>     object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> #define TPM_IS_SPAPR(chr)                           \
>     object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)

If it's just those simple defines, I can't see how that could break
things (you won't have the respective objects anyway). So yes, I think
that is the best solution.
Stefan Berger May 6, 2022, 12:40 p.m. UTC | #7 
On 5/6/22 03:34, Cornelia Huck wrote:
> On Fri, May 06 2022, Eric Auger <eric.auger@redhat.com> wrote:
> 
>> Hi Alex,
>>
>> On 4/28/22 22:14, Alex Williamson wrote:
>>> On Thu, 28 Apr 2022 15:49:45 +0200
>>> Eric Auger <eric.auger@redhat.com> wrote:
> 
>>>> +static bool vfio_known_safe_misalignment(MemoryRegionSection *section)
>>>> +{
>>>> +    MemoryRegion *mr = section->mr;
>>>> +
>>>> +    if (!TPM_IS_CRB(mr->owner)) {
>>>> +        return false;
>>>> +    }
>>> It looks like this test is going to need to be wrapped in #ifdef
>>> CONFIG_TPM:
>>
>> sorry for the delay. Your message fell though the cracks :-(
>>
>> if I put an '#ifdef CONFIG_TPM' I need to inverse the logic because by
>> default the function shall return false.
>>
>> solution #1
>>
>> #ifdef CONFIG_TPM
>>   if (TPM_IS_CRB(mr->owner)) {
>>
>>      /* this is a known safe misaligned region, just trace for debug purpose */
>>      trace_vfio_known_safe_misalignment(memory_region_name(mr),
>>                                         section->offset_within_address_space,
>>                                         section->offset_within_region,
>>                                         qemu_real_host_page_size());
>>
>>      return true;
>>     }
>>
>> #endif
>> return false;
>>
>> This looks weird to me.
>>
>> +    if (!object_dynamic_cast(mr->owner, TYPE_TPM_CRB)) {
>> +        return false;
>> +    }
>>
>>
>> solution #2
>> replace !object_dynamic_cast(mr->owner, TYPE_TPM_CRB) by
>> !object_dynamic_cast(mr->owner, "tpm-crb")
>> and add a comment saying that we don't use TYPE_TPM_CRB on purpose
>>
>> solution #3
>> Move #define TPM_IS_CRB(chr) and related defined out of
>> #ifdef CONFIG_TPM hoping it does not have other side effects
>>
>> Thoughts?
>> Eric
> 
> solution #4
> 
> #ifndef CONFIG_TPM
> /* needed for an alignment check in non-tpm code */
> static inline Object *TPM_IS_CRB(Object *obj)
> {
>      return NULL;
> }
> #endif
> 
> I think it would be good if we could hide the configuration details in
> the header.
> 

Solution #4 looks good to me...
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 2b1f78fdfa..f6b9bb6d71 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -40,6 +40,7 @@ 
 #include "trace.h"
 #include "qapi/error.h"
 #include "migration/migration.h"
+#include "sysemu/tpm.h"
 
 VFIOGroupList vfio_group_list =
     QLIST_HEAD_INITIALIZER(vfio_group_list);
@@ -861,6 +862,22 @@  static void vfio_unregister_ram_discard_listener(VFIOContainer *container,
     g_free(vrdl);
 }
 
+static bool vfio_known_safe_misalignment(MemoryRegionSection *section)
+{
+    MemoryRegion *mr = section->mr;
+
+    if (!TPM_IS_CRB(mr->owner)) {
+        return false;
+    }
+
+    /* this is a known safe misaligned region, just trace for debug purpose */
+    trace_vfio_known_safe_misalignment(memory_region_name(mr),
+                                       section->offset_within_address_space,
+                                       section->offset_within_region,
+                                       qemu_real_host_page_size());
+    return true;
+}
+
 static void vfio_listener_region_add(MemoryListener *listener,
                                      MemoryRegionSection *section)
 {
@@ -884,7 +901,15 @@  static void vfio_listener_region_add(MemoryListener *listener,
     if (unlikely((section->offset_within_address_space &
                   ~qemu_real_host_page_mask()) !=
                  (section->offset_within_region & ~qemu_real_host_page_mask()))) {
-        error_report("%s received unaligned region", __func__);
+        if (!vfio_known_safe_misalignment(section)) {
+            error_report("%s received unaligned region %s iova=0x%"PRIx64
+                         " offset_within_region=0x%"PRIx64
+                         " qemu_real_host_page_size=0x%"PRIxPTR,
+                         __func__, memory_region_name(section->mr),
+                         section->offset_within_address_space,
+                         section->offset_within_region,
+                         qemu_real_host_page_size());
+        }
         return;
     }
 
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index 0ef1b5f4a6..582882db91 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -100,6 +100,7 @@  vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add
 vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d"
 vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add [iommu] 0x%"PRIx64" - 0x%"PRIx64
 vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]"
+vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t offset_within_region, uintptr_t page_size) "Region \"%s\" iova=0x%"PRIx64" offset_within_region=0x%"PRIx64" qemu_real_host_page_size=0x%"PRIxPTR ": cannot be mapped for DMA"
 vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA"
 vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
 vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64