Re: [PATCH for-7.1] vfio/common: remove spurious tpm-crb-cmd misalignment warning

[Eric Auger]

Hi Alex,

On 3/17/22 12:08 AM, Alex Williamson wrote:
> On Wed, 16 Mar 2022 21:29:51 +0100
> 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 name to recognize the region and detect the situation
>> is safe wrt assignment. Other regions can be listed here if such kind
>> of problem occurs again.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/common.c     | 26 +++++++++++++++++++++++++-
>>  hw/vfio/trace-events |  1 +
>>  2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 080046e3f51..b58a38f5c57 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -861,6 +861,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 (strcmp(memory_region_name(mr), "tpm-crb-cmd") != 0) {
>> +        return false;
>> +    }
> Hi Eric,
>
> I was thinking more along the lines that we could use
> memory_region_owner() to get the owning Object, then on
> that we could maybe use INTERFACE_CHECK to look for TYPE_MEMORY_DEVICE,
> then consider anything else optional.  (a) could something like that
> work and (b) do all required mappings currently expose that interface?
> Thanks,
If I understand correctly you just want to error_report() misalignement
of MR sections belonging to

TYPE_MEMORY_DEVICE devices and silence the rest? Is that a correct 
understanding? I thought you wanted to be much more protective and ignore 
misalignments on a case by case basis hence the white listing of this single 
tpm-crb-cmd region.

Thanks

Eric

>
> 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 +900,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_mask=0x%"PRIxPTR,
>> +                         __func__, memory_region_name(section->mr),
>> +                         section->offset_within_address_space,
>> +                         section->offset_within_region,
>> +                         qemu_real_host_page_mask);
>> +        }
>>          return;
>>      }
>>  
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index 0ef1b5f4a65..6f38a2e6991 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, uint64_t page_size) "Region \"%s\" iova=0x%"PRIx64" 
>> offset_within_region=0x%"PRIx64" qemu_real_host_page_mask=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