On 27/09/2018 10:02, Igor Mammedov wrote:
> On Wed, 26 Sep 2018 11:41:59 +0200
> David Hildenbrand <da...@redhat.com> 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.
(very simply and unrealistic) counter example as given in the
description. You should get the idea.
address_space_start = 0xffffffffffffffffull;
size = 1;
-> address_space_end = 0;
While this would be perfectly valid, we would have to handle
address_space_end potentially being 0 in the code below, because this
would be a valid overflow. This, I avoid.
And form my POV, the variable name here matches perfectly. It points at
the last address of the address space. (yes people have different
opinions on this)
>
> I'd drop it and maybe extend the following assert to abort
> on overflow condition.
I'll leave it like this, handling address_space_end = 0 is ugly.
I'll add a comment like
/* address_space_end points at the last valid address */
>
>> 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
>
Right, maybe adding a new temporary variable new_region_end will help.
--
Thanks,
David / dhildenb
