Re: [Qemu-devel] [PATCH v3 7/7] memory-device: rewrite address assignment using ranges

[Igor Mammedov]

On Tue, 23 Oct 2018 17:23:06 +0200
David Hildenbrand <address@hidden> wrote:

> Let's rewrite it properly using ranges. This fixes certain overflows that
> are right now possible. E.g.
> 
> qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
>     -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
>     -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000
> 
> Now properly reports an error instead of succeeding.
s/error/error out/

> 
> "can't add memory device [0xffffffffc0000000:0x80000000], range overflow"
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  hw/mem/memory-device.c | 53 ++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 8be63c8032..2fb6fc2145 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState 
> *ms,
>                                              uint64_t align, uint64_t size,
>                                              Error **errp)
>  {
> -    uint64_t address_space_start, address_space_end;
>      GSList *list = NULL, *item;
> -    uint64_t new_addr = 0;
> +    Range as, new = range_empty;
>  
>      if (!ms->device_memory) {
>          error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
> @@ -115,13 +114,11 @@ static uint64_t 
> memory_device_get_free_addr(MachineState *ms,
>                           "enabled, please specify the maxmem option");
>          return 0;
>      }
> -    address_space_start = ms->device_memory->base;
> -    address_space_end = address_space_start +
> -                        memory_region_size(&ms->device_memory->mr);
> -    g_assert(address_space_end >= address_space_start);
> +    range_init_nofail(&as, ms->device_memory->base,
> +                      memory_region_size(&ms->device_memory->mr));
>  
> -    /* address_space_start indicates the maximum alignment we expect */
> -    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
> +    /* start of address space indicates the maximum alignment we expect */
> +    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
>          error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
>                     align);
>          return 0;
> @@ -145,20 +142,24 @@ static uint64_t 
> memory_device_get_free_addr(MachineState *ms,
>      }
>  
>      if (hint) {
> -        new_addr = *hint;
> -        if (new_addr < address_space_start) {
> +        if (range_init(&new, *hint, size)) {
>              error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" 
> PRIx64
> -                       "] before 0x%" PRIx64, new_addr, size,
> -                       address_space_start);
> +                       "], range overflow", *hint, size);
>              return 0;
> -        } else if ((new_addr + size) > address_space_end) {
> +        }
> +        if (!range_contains_range(&as, &new)) {
>              error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" 
> PRIx64
> -                       "] beyond 0x%" PRIx64, new_addr, size,
> -                       address_space_end);
> +                       "], usable range for memory devices [0x%" PRIx64 
> ":0x%"
> +                       PRIx64 "]", range_lob(&new), range_size(&new),
> +                       range_lob(&as), range_size(&as));
>              return 0;
>          }
>      } else {
> -        new_addr = address_space_start;
> +        if (range_init(&new, range_lob(&as), size)) {
> +            error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" 
> PRIx64
> +                       "], range overflow", *hint, size);
maybe replace "range overflow" with "too big" or something else more user 
friendly 

> +            return 0;
> +        }
>      }
>  
>      /* find address range that will fit new memory device */
> @@ -166,30 +167,36 @@ static uint64_t 
> memory_device_get_free_addr(MachineState *ms,
>      for (item = list; item; item = g_slist_next(item)) {
>          const MemoryDeviceState *md = item->data;
>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
> -        uint64_t md_size, md_addr;
> +        uint64_t next_addr;
> +        Range tmp;
>  
> -        md_addr = mdc->get_addr(md);
> -        md_size = memory_device_get_region_size(md, &error_abort);
> +        range_init_nofail(&tmp, mdc->get_addr(md),
> +                          memory_device_get_region_size(md, &error_abort));
>  
> -        if (ranges_overlap(md_addr, md_size, new_addr, size)) {
> +        if (range_overlaps_range(&tmp, &new)) {
>              if (hint) {
>                  const DeviceState *d = DEVICE(md);
>                  error_setg(errp, "address range conflicts with memory device"
>                             " id='%s'", d->id ? d->id : "(unnamed)");
>                  goto out;
>              }
> -            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
> +
> +            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
                                         ^^^^^^^^^^^^^^^^^^^
this theoretically could overflow and already past 'as' check so it would
return an invalid address without erroring out.

But in practice we don't have memory device container ending right on 64bit
limit, so it's not really an issue.


> +            if (range_init(&new, next_addr, range_size(&new))) {
> +                range_make_empty(&new);
> +                break;
> +            }
>          }
>      }
>  
> -    if (new_addr + size > address_space_end) {
> +    if (!range_contains_range(&as, &new)) {
>          error_setg(errp, "could not find position in guest address space for 
> "
>                     "memory device - memory fragmented due to alignments");
>          goto out;
>      }
>  out:
>      g_slist_free(list);
> -    return new_addr;
> +    return range_lob(&new);
>  }
>  
>  MemoryDeviceInfoList *qmp_memory_device_list(void)

beside minor notes patch looks good