Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 10/16] vfio: Use different page size for different IOMMU types

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4969 bytes --]

On Tue, Mar 01, 2016 at 08:10:35PM +1100, Alexey Kardashevskiy wrote:
> The existing memory listener is called on RAM or PCI address space
> which implies potentially different page size.
> 
> This uses new memory_region_iommu_get_page_sizes() for IOMMU regions
> or falls back to qemu_real_host_page_size if RAM.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

This doesn't seem right to me.. but neither does the original code.

As far as I can tell, these checks against page sizes are trying to
make sure that the regions are aligned in such a way that we can
actually map them in the host IOMMU.  But TARGET_PAGE_SIZE is a
property of the guest, rather than the host.

So, changing TARGET_PAGE_SIZE to qemu_real_host_page_size seems
correct to me for RAM regions.

But memory_region_iommu_get_page_sizes() is *not* the right choice for
IOMMU regions, because that gives you the granularity of the guest
IOMMU, whereas you need the granularity of the host IOMMU.

Unless I'm mistaking the purpose of these checks, which I hope Alex
can clarify us on when he gets back from holiday next week.

> ---
> Changes:
> * uses the smallest page size for mask as IOMMU MR can support multple
> page sizes
> ---
>  hw/vfio/common.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0e67a5a..3aaa6b5 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -318,6 +318,16 @@ static hwaddr vfio_container_granularity(VFIOContainer *container)
>      return (hwaddr)1 << ctz64(container->iova_pgsizes);
>  }
>  
> +static hwaddr vfio_iommu_page_mask(MemoryRegion *mr)
> +{
> +    if (memory_region_is_iommu(mr)) {
> +        int smallest = ffs(memory_region_iommu_get_page_sizes(mr)) - 1;
> +
> +        return ~((1ULL << smallest) - 1);
> +    }
> +    return qemu_real_host_page_mask;
> +}
> +
>  static void vfio_listener_region_add(VFIOMemoryListener *vlistener,
>                                       MemoryRegionSection *section)
>  {
> @@ -326,6 +336,7 @@ static void vfio_listener_region_add(VFIOMemoryListener *vlistener,
>      Int128 llend;
>      void *vaddr;
>      int ret;
> +    hwaddr page_mask = vfio_iommu_page_mask(section->mr);
>  
>      if (vfio_listener_skipped_section(section)) {
>          trace_vfio_listener_region_add_skip(
> @@ -335,16 +346,16 @@ static void vfio_listener_region_add(VFIOMemoryListener *vlistener,
>          return;
>      }
>  
> -    if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
> -                 (section->offset_within_region & ~TARGET_PAGE_MASK))) {
> +    if (unlikely((section->offset_within_address_space & ~page_mask) !=
> +                 (section->offset_within_region & ~page_mask))) {
>          error_report("%s received unaligned region", __func__);
>          return;
>      }
>  
> -    iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
> +    iova = ROUND_UP(section->offset_within_address_space, ~page_mask + 1);
>      llend = int128_make64(section->offset_within_address_space);
>      llend = int128_add(llend, section->size);
> -    llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
> +    llend = int128_and(llend, int128_exts64(page_mask));
>  
>      if (int128_ge(int128_make64(iova), llend)) {
>          return;
> @@ -432,6 +443,7 @@ static void vfio_listener_region_del(VFIOMemoryListener *vlistener,
>      hwaddr iova, end;
>      int ret;
>      MemoryRegion *iommu = NULL;
> +    hwaddr page_mask = vfio_iommu_page_mask(section->mr);
>  
>      if (vfio_listener_skipped_section(section)) {
>          trace_vfio_listener_region_del_skip(
> @@ -441,8 +453,8 @@ static void vfio_listener_region_del(VFIOMemoryListener *vlistener,
>          return;
>      }
>  
> -    if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
> -                 (section->offset_within_region & ~TARGET_PAGE_MASK))) {
> +    if (unlikely((section->offset_within_address_space & ~page_mask) !=
> +                 (section->offset_within_region & ~page_mask))) {
>          error_report("%s received unaligned region", __func__);
>          return;
>      }
> @@ -469,9 +481,9 @@ static void vfio_listener_region_del(VFIOMemoryListener *vlistener,
>           */
>      }
>  
> -    iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
> +    iova = ROUND_UP(section->offset_within_address_space, ~page_mask + 1);
>      end = (section->offset_within_address_space + int128_get64(section->size)) &
> -          TARGET_PAGE_MASK;
> +          page_mask;
>  
>      if (iova >= end) {
>          return;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]