Hi, On 05/04/18 04:17, Alex Williamson wrote: > On Thu, 5 Apr 2018 10:53:38 +1000 > Alexey Kardashevskiy <a...@ozlabs.ru> wrote: > >> On 5/4/18 9:09 am, Alex Williamson wrote: >>> On Wed, 4 Apr 2018 22:30:50 +0200 >>> Eric Auger <eric.au...@redhat.com> 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 <a...@ozlabs.ru> >>>> Signed-off-by: Eric Auger <eric.au...@redhat.com> >>>> >>>> --- >>>> >>>> v1 -> v2: >>>> - use a trace point >>>> - add some details in the commit message >>> >>> I think this is v2 with v1 being >>> Message-Id:<20180322081837.21460-1-...@ozlabs.ru> 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 <a...@ozlabs.ru> > > 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" >>> >> >> > >