On 27/09/2018 10:47, Igor Mammedov wrote: > On Thu, 27 Sep 2018 10:13:07 +0200 > David Hildenbrand <da...@redhat.com> wrote: > >> 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; > that should never happen, nor valid in any conditions > just add assert so we would catch error if some patch would introduce it > >> 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. > assert(address_space_end > address_space_start) > would be much better for unrealistic values without messing up with > meaning of variables. > >> 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) > start,end pair is a range, there shouldn't be any other interpretations to > this variables. >
Please see include/qemu/range.h:struct Range; So I am using _the definition_. >>> >>> 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. > Looks like I have to insist on dropping this hunk. You're going from "I'd" to "I have to insist". Please make up your mind. I have a R-b from David. But I am willing to change it if you can give me a good reason why we should add asserts instead of fixing the code (== making it work in all possibly valid scenarios, especially end of address space). With the new local variable new_end, the code looks pretty clean. -- Thanks, David / dhildenb
