Re: [Qemu-devel] [PATCH 10/21] memory: make section size a 128-bit integer

[Alexey Kardashevskiy]

On 05/31/2013 07:16 AM, Paolo Bonzini wrote:
> So far, the size of all regions passed to listeners could fit in 64 bits,
> because artificial regions (containers and aliases) are eliminated by
> the memory core, leaving only device regions which have reasonable sizes
> 
> An IOMMU however cannot be eliminated by the memory core, and may have
> an artificial size, hence we may need 65 bits to represent its size.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  exec.c                        | 37 +++++++++++++++++++++----------------
>  hw/core/loader.c              |  2 +-
>  hw/display/exynos4210_fimd.c  |  4 ++--
>  hw/display/framebuffer.c      |  3 ++-
>  hw/misc/vfio.c                |  4 ++--
>  hw/virtio/dataplane/hostmem.c |  2 +-
>  hw/virtio/vhost.c             |  4 ++--
>  hw/virtio/virtio-balloon.c    |  2 +-
>  include/exec/memory.h         |  5 ++++-
>  include/qemu/int128.h         | 10 ++++++++++
>  kvm-all.c                     | 23 ++++++++++++++---------
>  memory.c                      | 14 +++++++-------
>  xen-all.c                     |  6 +++---
>  13 files changed, 70 insertions(+), 46 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index cf3ea6c..b86f0cc 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -801,7 +801,7 @@ static void register_subpage(AddressSpaceDispatch *d, 
> MemoryRegionSection *secti
>      MemoryRegionSection *existing = phys_page_find(d, base >> 
> TARGET_PAGE_BITS);
>      MemoryRegionSection subsection = {
>          .offset_within_address_space = base,
> -        .size = TARGET_PAGE_SIZE,
> +        .size = int128_make64(TARGET_PAGE_SIZE),
>      };
>      hwaddr start, end;
>  
> @@ -816,16 +816,18 @@ static void register_subpage(AddressSpaceDispatch *d, 
> MemoryRegionSection *secti
>          subpage = container_of(existing->mr, subpage_t, iomem);
>      }
>      start = section->offset_within_address_space & ~TARGET_PAGE_MASK;
> -    end = start + section->size - 1;
> +    end = start + int128_get64(section->size) - 1;
>      subpage_register(subpage, start, end, phys_section_add(section));
>  }
>  
>  
> -static void register_multipage(AddressSpaceDispatch *d, MemoryRegionSection 
> *section)
> +static void register_multipage(AddressSpaceDispatch *d,
> +                               MemoryRegionSection *section)
>  {
>      hwaddr start_addr = section->offset_within_address_space;
>      uint16_t section_index = phys_section_add(section);
> -    uint64_t num_pages = section->size >> TARGET_PAGE_BITS;
> +    uint64_t num_pages = int128_get64(int128_rshift(section->size,
> +                                                    TARGET_PAGE_BITS));
>  
>      assert(num_pages);
>      phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, 
> section_index);
> @@ -835,28 +837,29 @@ static void mem_add(MemoryListener *listener, 
> MemoryRegionSection *section)
>  {
>      AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, 
> listener);
>      MemoryRegionSection now = *section, remain = *section;
> +    Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
>  
>      if (now.offset_within_address_space & ~TARGET_PAGE_MASK) {
>          uint64_t left = TARGET_PAGE_ALIGN(now.offset_within_address_space)
>                         - now.offset_within_address_space;
>  
> -        now.size = MIN(left, now.size);
> +        now.size = int128_min(int128_make64(left), now.size);
>          register_subpage(d, &now);
>      } else {
> -        now.size = 0;
> +        now.size = int128_zero();
>      }
> -    while (remain.size != now.size) {
> -        remain.size -= now.size;
> -        remain.offset_within_address_space += now.size;
> -        remain.offset_within_region += now.size;
> +    while (int128_ne(remain.size, now.size)) {
> +        remain.size = int128_sub(remain.size, now.size);
> +        remain.offset_within_address_space += int128_get64(now.size);
> +        remain.offset_within_region += int128_get64(now.size);
>          now = remain;
> -        if (remain.size < TARGET_PAGE_SIZE) {
> +        if (int128_lt(remain.size, page_size)) {
>              register_subpage(d, &now);
>          } else if (remain.offset_within_region & ~TARGET_PAGE_MASK) {
> -            now.size = TARGET_PAGE_SIZE;
> +            now.size = page_size;
>              register_subpage(d, &now);
>          } else {
> -            now.size &= -TARGET_PAGE_SIZE;
> +            now.size = int128_and(now.size, int128_neg(page_size));
>              register_multipage(d, &now);
>          }
>      }
> @@ -1666,7 +1669,7 @@ static uint16_t dummy_section(MemoryRegion *mr)
>          .mr = mr,
>          .offset_within_address_space = 0,
>          .offset_within_region = 0,
> -        .size = UINT64_MAX,
> +        .size = int128_2_64(),
>      };
>  
>      return phys_section_add(&section);
> @@ -1735,14 +1738,16 @@ static void io_region_add(MemoryListener *listener,
>      mrio->mr = section->mr;
>      mrio->offset = section->offset_within_region;
>      iorange_init(&mrio->iorange, &memory_region_iorange_ops,
> -                 section->offset_within_address_space, section->size);
> +                 section->offset_within_address_space,
> +                 int128_get64(section->size));
>      ioport_register(&mrio->iorange);
>  }
>  
>  static void io_region_del(MemoryListener *listener,
>                            MemoryRegionSection *section)
>  {
> -    isa_unassign_ioport(section->offset_within_address_space, section->size);
> +    isa_unassign_ioport(section->offset_within_address_space,
> +                        int128_get64(section->size));
>  }
>  
>  static MemoryListener core_memory_listener = {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 7507914..3a60cbe 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -726,7 +726,7 @@ int rom_load_all(void)
>          addr  = rom->addr;
>          addr += rom->romsize;
>          section = memory_region_find(get_system_memory(), rom->addr, 1);
> -        rom->isrom = section.size && memory_region_is_rom(section.mr);
> +        rom->isrom = int128_nz(section.size) && 
> memory_region_is_rom(section.mr);
>      }
>      qemu_register_reset(rom_reset, NULL);
>      roms_loaded = 1;
> diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
> index 6cb5016..0da00a9 100644
> --- a/hw/display/exynos4210_fimd.c
> +++ b/hw/display/exynos4210_fimd.c
> @@ -1133,7 +1133,7 @@ static void 
> fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>      DPRINT_TRACE("Window %u framebuffer changed: address=0x%08x, len=0x%x\n",
>              win, fb_start_addr, w->fb_len);
>  
> -    if (w->mem_section.size != w->fb_len ||
> +    if (int128_get64(w->mem_section.size) != w->fb_len ||
>              !memory_region_is_ram(w->mem_section.mr)) {
>          DPRINT_ERROR("Failed to find window %u framebuffer region\n", win);
>          goto error_return;
> @@ -1155,7 +1155,7 @@ static void 
> fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>  
>  error_return:
>      w->mem_section.mr = NULL;
> -    w->mem_section.size = 0;
> +    w->mem_section.size = int128_zero();
>      w->host_fb_addr = NULL;
>      w->fb_len = 0;
>  }
> diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
> index 6be31db..49c9e59 100644
> --- a/hw/display/framebuffer.c
> +++ b/hw/display/framebuffer.c
> @@ -54,7 +54,8 @@ void framebuffer_update_display(
>      src_len = src_width * rows;
>  
>      mem_section = memory_region_find(address_space, base, src_len);
> -    if (mem_section.size != src_len || 
> !memory_region_is_ram(mem_section.mr)) {
> +    if (int128_get64(mem_section.size) != src_len ||
> +            !memory_region_is_ram(mem_section.mr)) {
>          return;
>      }
>      mem = mem_section.mr;
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 693a9ff..c89676b 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -1953,7 +1953,7 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>      }
>  
>      iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
> -    end = (section->offset_within_address_space + section->size) &
> +    end = (section->offset_within_address_space + 
> int128_get64(section->size)) &
>            TARGET_PAGE_MASK;



Another problem with this patch. Here is some more context (***):

===
     iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
-    end = (section->offset_within_address_space + section->size) &
+    end = (section->offset_within_address_space +
int128_get64(section->size)) &
           TARGET_PAGE_MASK;

     if (iova >= end) {
         return;
     }

     vaddr = memory_region_get_ram_ptr(section->mr) +
             section->offset_within_region +
             (iova - section->offset_within_address_space);

     DPRINTF("region_add %"HWADDR_PRIx" - %"HWADDR_PRIx" [%p]\n",
             iova, end - 1, vaddr);

     ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly);

===

What happens:

1. "spapr: use memory core for iommu support"  patch calls
memory_region_init_iommu() with size=UINT64_MAX.

2. "memory: use 128-bit integers for sizes and intermediates" patch fixes
such values to UINT64_MAX+1:

void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size)
 {
     mr->ops = NULL;
     mr->parent = NULL;
-    mr->size = size;
+    mr->size = int128_make64(size);
+    if (size == UINT64_MAX) {
+        mr->size = int128_2_64();
+    }

3. (***) patch calls int128_get64() which fails in assert.


At the moment I fixed it by calling  memory_region_init_iommu(...
UINT64_MAX >> 1) + 1) and it makes me happy (or it can be INT64_MAX+1) but
I am not sure it is canonically right :)

What would be the right fix?


-- 
Alexey