Re: [Qemu-ppc] [Qemu-devel] [PATCH qemu v7 2/4] vfio/pci: Relax DMA map errors for MMIO regions

[Auger Eric]

Hi Alexey,

On 09/02/18 08:55, Alexey Kardashevskiy wrote:
> At the moment if vfio_memory_listener is registered in the system memory
> address space, it maps/unmaps every RAM memory region for DMA.
> It expects system page size aligned memory sections so vfio_dma_map
> would not fail and so far this has been the case. A mapping failure
> would be fatal. A side effect of such behavior is that some MMIO pages
> would not be mapped silently.
> 
> However we are going to change MSIX BAR handling so we will end having
> non-aligned sections in vfio_memory_listener (more details is in
> the next patch) and vfio_dma_map will exit QEMU.
> 
> In order to avoid fatal failures on what previously was not a failure and
> was just silently ignored, this checks the section alignment to
> the smallest supported IOMMU page size and prints an error if not aligned;
> it also prints an error if vfio_dma_map failed despite the page size check.
> Both errors are not fatal; only MMIO RAM regions are checked
> (aka "RAM device" regions).
> 
> If the amount of errors printed is overwhelming, the MSIX relocation
> could be used to avoid excessive error output.
> 
> This is unlikely to cause any behavioral change.
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
>  hw/vfio/common.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index f895e3c..736f271 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -544,18 +544,40 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>  
>      llsize = int128_sub(llend, int128_make64(iova));
>  
> +    if (memory_region_is_ram_device(section->mr)) {
> +        hwaddr pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
> +
> +        if ((iova & pgmask) || (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),
Didn't you want to print the section->offset_within_address_space
instead of section->offset_within_region?

For an 1000e card*, with a 64kB page, it outputs:
"Region 0x50..0xffb0 is not aligned to 0x10000 and cannot be mapped for DMA"

an absolute gpa range would be simpler I think.


* where         Region 3: Memory at 100e0000 (32-bit, non-prefetchable)
[size=16K] contains the MSIX table

        Capabilities: [a0] MSI-X: Enable+ Count=5 Masked-
                Vector table: BAR=3 offset=00000000
                PBA: BAR=3 offset=00002000

Thanks

Eric
> +                         pgmask + 1);
> +            return;
> +        }
> +    }
> +
>      ret = vfio_dma_map(container, iova, int128_get64(llsize),
>                         vaddr, section->readonly);
>      if (ret) {
>          error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
>                       "0x%"HWADDR_PRIx", %p) = %d (%m)",
>                       container, iova, int128_get64(llsize), vaddr, ret);
> +        if (memory_region_is_ram_device(section->mr)) {
> +            /* Allow unexpected mappings not to be fatal for RAM devices */
> +            return;
> +        }
>          goto fail;
>      }
>  
>      return;
>  
>  fail:
> +    if (memory_region_is_ram_device(section->mr)) {
> +        error_report("failed to vfio_dma_map. pci p2p may not work");
> +        return;
> +    }
>      /*
>       * On the initfn path, store the first error in the container so we
>       * can gracefully fail.  Runtime, there's not much we can do other
> @@ -577,6 +599,7 @@ static void vfio_listener_region_del(MemoryListener 
> *listener,
>      hwaddr iova, end;
>      Int128 llend, llsize;
>      int ret;
> +    bool try_unmap = true;
>  
>      if (vfio_listener_skipped_section(section)) {
>          trace_vfio_listener_region_del_skip(
> @@ -629,13 +652,33 @@ static void vfio_listener_region_del(MemoryListener 
> *listener,
>  
>      trace_vfio_listener_region_del(iova, end);
>  
> -    ret = vfio_dma_unmap(container, iova, int128_get64(llsize));
> +    if (memory_region_is_ram_device(section->mr)) {
> +        hwaddr pgmask;
> +        VFIOHostDMAWindow *hostwin;
> +        bool hostwin_found = false;
> +
> +        QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
> +            if (hostwin->min_iova <= iova && end <= hostwin->max_iova) {
> +                hostwin_found = true;
> +                break;
> +            }
> +        }
> +        assert(hostwin_found); /* or region_add() would have failed */
> +
> +        pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
> +        try_unmap = !((iova & pgmask) || (llsize & pgmask));
> +    }
> +
> +    if (try_unmap) {
> +        ret = vfio_dma_unmap(container, iova, int128_get64(llsize));
> +        if (ret) {
> +            error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
> +                         "0x%"HWADDR_PRIx") = %d (%m)",
> +                         container, iova, int128_get64(llsize), ret);
> +        }
> +    }
> +
>      memory_region_unref(section->mr);
> -    if (ret) {
> -        error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
> -                     "0x%"HWADDR_PRIx") = %d (%m)",
> -                     container, iova, int128_get64(llsize), ret);
> -    }
>  
>      if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>          vfio_spapr_remove_window(container,
>