From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42640) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkVgg-0008RD-Kt for qemu-devel@nongnu.org; Thu, 06 Jun 2013 04:36:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UkVgd-00054H-45 for qemu-devel@nongnu.org; Thu, 06 Jun 2013 04:36:50 -0400 Received: from mail-pd0-f180.google.com ([209.85.192.180]:45724) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkVgc-000549-QZ for qemu-devel@nongnu.org; Thu, 06 Jun 2013 04:36:47 -0400 Received: by mail-pd0-f180.google.com with SMTP id 10so3045270pdi.39 for ; Thu, 06 Jun 2013 01:36:46 -0700 (PDT) Message-ID: <51B04A17.7080106@ozlabs.ru> Date: Thu, 06 Jun 2013 18:36:39 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1369948629-2833-1-git-send-email-pbonzini@redhat.com> <1369948629-2833-11-git-send-email-pbonzini@redhat.com> In-Reply-To: <1369948629-2833-11-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 10/21] memory: make section size a 128-bit integer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Alex Williamson , qemu-devel@nongnu.org, David Gibson 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 > --- > 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(§ion); > @@ -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