Re: [Qemu-devel] [PATCH v2] memory: use signed arithmetic

[Anthony Liguori <anthony@codemonkey.ws>]

On 08/03/2011 03:56 AM, Avi Kivity wrote:
> When trying to map an alias of a ram region, where the alias starts at
> address A and we map it into address B, and A>  B, we had an arithmetic
> underflow.  Because we use unsigned arithmetic, the underflow converted
> into a large number which failed addrrange_intersects() tests.
>
> The concrete example which triggered this was cirrus vga mapping
> the framebuffer at offsets 0xc0000-0xc7fff (relative to the start of
> the framebuffer) into offsets 0xa0000 (relative to system addres space
> start).
>
> With our favorite analogy of a windowing system, this is equivalent to
> dragging a subwindow off the left edge of the screen, and failing to clip
> it into its parent window which is on screen.
>
> Fix by switching to signed arithmetic.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>
> v2: add comment about physical address width limitation to 63 bits
>
>   exec.c   |    2 +-
>   memory.c |   23 ++++++++++++++---------
>   2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 476b507..751fd89 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3818,7 +3818,7 @@ static void io_mem_init(void)
>   static void memory_map_init(void)
>   {
>       system_memory = qemu_malloc(sizeof(*system_memory));
> -    memory_region_init(system_memory, "system", UINT64_MAX);
> +    memory_region_init(system_memory, "system", INT64_MAX);
>       set_system_memory_map(system_memory);
>   }
>
> diff --git a/memory.c b/memory.c
> index 5f20320..be891c6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -22,12 +22,17 @@ unsigned memory_region_transaction_depth = 0;
>
>   typedef struct AddrRange AddrRange;
>
> +/*
> + * Note using signed integers limits us to physical addresses at most
> + * 63 bits wide.  They are needed for negative offsetting in aliases
> + * (large MemoryRegion::alias_offset).
> + */
>   struct AddrRange {
> -    uint64_t start;
> -    uint64_t size;
> +    int64_t start;
> +    int64_t size;
>   };
>
> -static AddrRange addrrange_make(uint64_t start, uint64_t size)
> +static AddrRange addrrange_make(int64_t start, int64_t size)
>   {
>       return (AddrRange) { start, size };
>   }
> @@ -37,7 +42,7 @@ static bool addrrange_equal(AddrRange r1, AddrRange r2)
>       return r1.start == r2.start&&  r1.size == r2.size;
>   }
>
> -static uint64_t addrrange_end(AddrRange r)
> +static int64_t addrrange_end(AddrRange r)
>   {
>       return r.start + r.size;
>   }
> @@ -56,9 +61,9 @@ static bool addrrange_intersects(AddrRange r1, AddrRange r2)
>
>   static AddrRange addrrange_intersection(AddrRange r1, AddrRange r2)
>   {
> -    uint64_t start = MAX(r1.start, r2.start);
> +    int64_t start = MAX(r1.start, r2.start);
>       /* off-by-one arithmetic to prevent overflow */
> -    uint64_t end = MIN(addrrange_end(r1) - 1, addrrange_end(r2) - 1);
> +    int64_t end = MIN(addrrange_end(r1) - 1, addrrange_end(r2) - 1);
>       return addrrange_make(start, end - start + 1);
>   }
>
> @@ -411,8 +416,8 @@ static void render_memory_region(FlatView *view,
>       MemoryRegion *subregion;
>       unsigned i;
>       target_phys_addr_t offset_in_region;
> -    uint64_t remain;
> -    uint64_t now;
> +    int64_t remain;
> +    int64_t now;
>       FlatRange fr;
>       AddrRange tmp;
>
> @@ -486,7 +491,7 @@ static FlatView generate_memory_topology(MemoryRegion *mr)
>
>       flatview_init(&view);
>
> -    render_memory_region(&view, mr, 0, addrrange_make(0, UINT64_MAX));
> +    render_memory_region(&view, mr, 0, addrrange_make(0, INT64_MAX));
>       flatview_simplify(&view);
>
>       return view;