Re: [Qemu-devel] [PATCH v2 for 2.12] vfio: Use a trace point when a RAM section cannot be DMA mapped

[Alexey Kardashevskiy]

On 5/4/18 9:09 am, Alex Williamson wrote:
> On Wed,  4 Apr 2018 22:30:50 +0200
> Eric Auger <address@hidden> wrote:
> 
>> The 567b5b309abe ("vfio/pci: Relax DMA map errors for MMIO regions")
>> added an error message if a passed memory section address or size
>> is not aligned to the page size and thus cannot be DMA mapped.
>>
>> This patch fixes the trace by printing the region name and the
>> memory region section offset within the address space (instead of
>> offset_within_region).
>>
>> We also turn the error_report into a trace event. Indeed, In some
>> cases, the traces can be confusing to non expert end-users and
>> let think the use case does not work (whereas it
>> works as before).
>>
>> This is the case where a BAR is successively mapped at different
>> GPAs and its sections are not compatible with dma map. The listener
>> is called several times and traces are issued for each intermediate
>> mapping.  The end-user cannot easily match those GPAs against the
>> final GPA output by lscpi. So let's keep those information to
>> informed users. In mid term, the plan is to advise the user about
>> BAR relocation relevance.
>>
>> Fixes: 567b5b309abe "vfio/pci: Relax DMA map errors for MMIO regions"
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> v1 -> v2:
>> - use a trace point
>> - add some details in the commit message
> 
> I think this is v2 with v1 being
> Message-Id:<address@hidden> but this is
> substantially different from Alexey's posting so I'd like to verify the
> Sign-off.  Alexey?

My understanding it is not "sign-off" any more (I am fine either way, it
just does not seem fully correct), but

Reviewed-by: Alexey Kardashevskiy <address@hidden>


> 
> I agree that this seems to be the right thing to do for 2.12, nothing
> has changed for these mappings and the error reports are difficult even
> for experts to decipher and explain to users.  Thanks,
> 
> Alex
> 
>> ---
>>  hw/vfio/common.c     | 11 +++++------
>>  hw/vfio/trace-events |  1 +
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 5e84716..07ffa0b 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -548,12 +548,11 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>          hwaddr pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
>>  
>>          if ((iova & pgmask) || (int128_get64(llsize) & pgmask)) {
>> -            error_report("Region 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx
>> -                         " is not aligned to 0x%"HWADDR_PRIx
>> -                         " and cannot be mapped for DMA",
>> -                         section->offset_within_region,
>> -                         int128_getlo(section->size),
>> -                         pgmask + 1);
>> +            trace_vfio_listener_region_add_no_dma_map(
>> +                memory_region_name(section->mr),
>> +                section->offset_within_address_space,
>> +                int128_getlo(section->size),
>> +                pgmask + 1);
>>              return;
>>          }
>>      }
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index 79f63a2..20109cb 100644
>> --- a/hw/vfio/trace-events
>> +++ b/hw/vfio/trace-events
>> @@ -90,6 +90,7 @@ vfio_iommu_map_notify(const char *op, uint64_t iova_start, 
>> uint64_t iova_end) "i
>>  vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING 
>> region_add 0x%"PRIx64" - 0x%"PRIx64
>>  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_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
>>  vfio_disconnect_container(int fd) "close container->fd=%d"
> 


-- 
Alexey