| Message ID | 20180926094219.20322-5-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-device: complete refactoring + virtio-pmem | expand |
On Wed, 26 Sep 2018 11:41:59 +0200 David Hildenbrand <david@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. 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
On 27/09/2018 10:02, Igor Mammedov wrote: > On Wed, 26 Sep 2018 11:41:59 +0200 > David Hildenbrand <david@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.
On Thu, 27 Sep 2018 10:13:07 +0200 David Hildenbrand <david@redhat.com> wrote: > On 27/09/2018 10:02, Igor Mammedov wrote: > > On Wed, 26 Sep 2018 11:41:59 +0200 > > David Hildenbrand <david@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. > > > > 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. > > 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. what I was meaning is avoid -1 and rather use and additional range border check
On 27/09/2018 10:47, Igor Mammedov wrote: > On Thu, 27 Sep 2018 10:13:07 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 27/09/2018 10:02, Igor Mammedov wrote: >>> On Wed, 26 Sep 2018 11:41:59 +0200 >>> David Hildenbrand <david@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.
On Thu, 27 Sep 2018 10:58:47 +0200 David Hildenbrand <david@redhat.com> wrote: > On 27/09/2018 10:47, Igor Mammedov wrote: > > On Thu, 27 Sep 2018 10:13:07 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 27/09/2018 10:02, Igor Mammedov wrote: > >>> On Wed, 26 Sep 2018 11:41:59 +0200 > >>> David Hildenbrand <david@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. my suggestions heavily influenced by the fact that the code should be easy (for me and hopefully for others) to maintain in future, when I have to review related code later down the road when original author is not reachable/care anymore. (i.e. by pure selfishness) So far I'm not convinced that this change is for a better hence we stuck without consensus. > 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). if it's about valid cases then it's worth fixing, but how many of the cases targeted here are in need of fixing in practice? I'd keep it simple so it would be readable and do the job but not clutter it with not necessary logic. If we =actually= need to handle every possible permutation, it might be better to reuse range_foo() helpers you've mentioned, instead of open coding conditions over again. But only if we really need it or if it makes current code simpler to read. > With the new local variable new_end, the code looks pretty clean. >
On 27/09/2018 13:53, Igor Mammedov wrote: > On Thu, 27 Sep 2018 10:58:47 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 27/09/2018 10:47, Igor Mammedov wrote: >>> On Thu, 27 Sep 2018 10:13:07 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> On 27/09/2018 10:02, Igor Mammedov wrote: >>>>> On Wed, 26 Sep 2018 11:41:59 +0200 >>>>> David Hildenbrand <david@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. > my suggestions heavily influenced by the fact that the code should > be easy (for me and hopefully for others) to maintain in future, > when I have to review related code later down the road when original > author is not reachable/care anymore. (i.e. by pure selfishness) > > So far I'm not convinced that this change is for a better hence we stuck > without consensus. > >> 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). > if it's about valid cases then it's worth fixing, > but how many of the cases targeted here are in need of fixing in practice? > > I'd keep it simple so it would be readable and do the job > but not clutter it with not necessary logic. > > If we =actually= need to handle every possible permutation, it might > be better to reuse range_foo() helpers you've mentioned, instead of > open coding conditions over again. But only if we really need it or > if it makes current code simpler to read. We would probably have to add some new range_* helpers for that, something I like to avoid now. So to make some progress, I will keep the changes in this file minimal for now (e.g. specifying a device with a huge size is IMHO possible, so we should fix that). Maybe we can instead focus our energy on the more important parts (namely Patch 18, 23, and 24) - I would really like to hear your opinion about these. Thanks!
On Thu, 27 Sep 2018 13:58:45 +0200 David Hildenbrand <david@redhat.com> wrote: > On 27/09/2018 13:53, Igor Mammedov wrote: > > On Thu, 27 Sep 2018 10:58:47 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 27/09/2018 10:47, Igor Mammedov wrote: > >>> On Thu, 27 Sep 2018 10:13:07 +0200 > >>> David Hildenbrand <david@redhat.com> wrote: > >>> > >>>> On 27/09/2018 10:02, Igor Mammedov wrote: > >>>>> On Wed, 26 Sep 2018 11:41:59 +0200 > >>>>> David Hildenbrand <david@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. > > my suggestions heavily influenced by the fact that the code should > > be easy (for me and hopefully for others) to maintain in future, > > when I have to review related code later down the road when original > > author is not reachable/care anymore. (i.e. by pure selfishness) > > > > So far I'm not convinced that this change is for a better hence we stuck > > without consensus. > > > >> 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). > > if it's about valid cases then it's worth fixing, > > but how many of the cases targeted here are in need of fixing in practice? > > > > I'd keep it simple so it would be readable and do the job > > but not clutter it with not necessary logic. > > > > If we =actually= need to handle every possible permutation, it might > > be better to reuse range_foo() helpers you've mentioned, instead of > > open coding conditions over again. But only if we really need it or > > if it makes current code simpler to read. > > We would probably have to add some new range_* helpers for that, > something I like to avoid now. So to make some progress, I will keep the > changes in this file minimal for now (e.g. specifying a device with a > huge size is IMHO possible, so we should fix that). It's not a rush time before release where patch fixing something is good enough reason to convince me even if it's not the way it should be written. To minimize changes it's fine to just drop this patch. If you can reproduce real problem it's fine to fix it in this series or separately and do simplification later. > Maybe we can instead focus our energy on the more important parts > (namely Patch 18, 23, and 24) - I would really like to hear your opinion > about these. Thanks! >
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index 7c706fadfc..8f0b9a9898 100644 --- a/hw/mem/memory-device.c +++ b/hw/mem/memory-device.c @@ -85,7 +85,8 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size, /* will we exceed the total amount of memory specified */ memory_device_used_region_size(OBJECT(ms), &used_region_size); - if (used_region_size + size > ms->maxram_size - ms->ram_size) { + if (used_region_size + size < used_region_size || + used_region_size + size > ms->maxram_size - ms->ram_size) { error_setg(errp, "not enough space, currently 0x%" PRIx64 " in use of total hot pluggable 0x" RAM_ADDR_FMT, used_region_size, ms->maxram_size - ms->ram_size); @@ -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; 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;