Re: [Qemu-ppc] [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly

[Igor Mammedov]

On Wed, 26 Sep 2018 11:41:59 +0200
David Hildenbrand <address@hidden> wrote:

> Make address_space_end point at the real end, instead of end + 1, so we don't
> have to handle special cases like it being 0. This will allow us to
> place a memory device at the very end of the guest physical 64bit address
> space (if ever possible).

[...]

> @@ -115,7 +116,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>      }
>      address_space_start = ms->device_memory->base;
>      address_space_end = address_space_start +
> -                        memory_region_size(&ms->device_memory->mr);
> +                        memory_region_size(&ms->device_memory->mr) - 1;
I'm terrible at math, lets assume size = 1 so after this
'real end' would end up at 'address_space_start', which makes
it rather confusing beside not matching variable name anymore.

I'd drop it and maybe extend the following assert to abort
on overflow condition.

>      g_assert(address_space_end >= address_space_start);
>  
>      /* address_space_start indicates the maximum alignment we expect */
> @@ -149,7 +150,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>                         "] before 0x%" PRIx64, new_addr, size,
>                         address_space_start);
>              return 0;
> -        } else if ((new_addr + size) > address_space_end) {
> +        } else if (new_addr + size - 1 < new_addr ||
> +                   new_addr + size - 1 > address_space_end) {
>              error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
>                         "] beyond 0x%" PRIx64, new_addr, size,
>                         address_space_end);
> @@ -182,7 +184,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>          }
>      }
>  
> -    if (new_addr + size > address_space_end) {
> +    if (new_addr + size - 1 < new_addr || !new_addr ||
> +        new_addr + size - 1 > address_space_end) {
>          error_setg(errp, "could not find position in guest address space for 
> "
>                     "memory device - memory fragmented due to alignments");
>          goto out;
I strongly suggest replace non obvious math with several
plain <>= conditions even if adds more conditions compared
to math variant.
At least reader doesn't have to do calculations manually
to figure out what's going on