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

[Auger Eric]

Hi,

On 05/04/18 04:17, Alex Williamson wrote:
> On Thu, 5 Apr 2018 10:53:38 +1000
> Alexey Kardashevskiy <address@hidden> wrote:
> 
>> 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>
> 
> Yep, I think Eric was intending to give you credit since this patch
> builds on your patch.  I'll include the R-b.
Yes this was my intent, sorry for the confusion. I was eager to get this
for 2.12, hence this follow-up.

Thanks

Eric
> 
> There's still work to do, I'd like to see a single, concise message as
> we're parsing the device, but there's no benefit in alarming users when
> nothing has changed and it's too late into the release cycle to do more
> than just silence this for now.  Patches welcome towards a better
> solution, to be considered for 2.13.  Thanks,
> 
> Alex
> 
>>>
>>> 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"  
>>>   
>>
>>
> 
>