>> /* we can only map whole pages */
>> - size = QEMU_ALIGN_UP(size, pagesize);
>> + old_size = QEMU_ALIGN_UP(old_size, pagesize);
>> + new_size = QEMU_ALIGN_UP(new_size, pagesize);
>
> Shouldn't we just assert old_size and new_size are aligned with
> pagesize?
>
Already reworked :)
>> +
>> + /* we support actually resizable memory regions only on Linux */
>> + if (old_size < new_size) {
>> + /* populate the missing piece into the reserved area */
>> + ptr = mmap_populate(ptr + old_size, new_size - old_size, fd,
>> old_size, + shared, is_pmem);
>> + } else if (old_size > new_size) {
>> + /* discard this piece, keeping the area reserved (should never
>> fail) */ + ptr = mmap_reserve(ptr + new_size, old_size - new_size,
>> fd); + }
>
> I find the behaviour of this function somewhat confusing. Perhaps I'm
> missing something and need your help to clarify. Please bear with me.
>
> For the case where we want to grow in size, it returns a populated area
> (PROT_READ | PROT_WRITE flags).
>
> And for the case where we want to shrink in size, it returns a reserved
> area (PROT_NONE flag), requiring the caller to call mmap_populate()
> again to be able to use that memory.
>
> I believe the behaviour should be consistent regardless if we want to
> grow or shrink in size. Either return a reserved or an already
> populated area. Not both.
The return value is only used to differentiate between success (!=
MAP_FAILED) and failure (== MAP_FAILED), nothing else.
For now, I switched to returning a bool instead (resized vs. not
resized), that avoids this inconsistency. See my reply to Richard's comment.
>
> Would "old_size == new_size" situation be possible? In this case, ptr
> would be returned without changing protection flags of the mapping.
It could, although in this patch set, it never would :) The result would
be a NOP, which is the right thing to do.
>
> Shouldn't we also assert that new_size <= max_size?
Then we would have to pass max_size, just for asserting. I'll leave that
to the callers for now.
>
>> + return ptr;
>> +}
>> +
>> +void qemu_ram_munmap(int fd, void *ptr, size_t max_size)
>> +{
>> + const size_t pagesize = mmap_pagesize(fd);
>> +
>> + /* we can only map whole pages */
>> + max_size = QEMU_ALIGN_UP(max_size, pagesize);
>
> Shouldn't we just assert this and leave the alignment to the caller?
Already reworked.
Thanks!
--
Thanks,
David / dhildenb
