Hi
On Wed, Sep 5, 2018 at 1:37 PM, Juan Quintela <quint...@redhat.com> wrote:
> Marc-André Lureau <marcandre.lur...@redhat.com> wrote:
>> memory_region_init_ram*_ptr() take only the size of the MemoryRegion,
>> and allocate a RAMBlock with the same size. However, it may be
>> convenient to expose a smaller MemoryRegion (for ex: a few bytes) than
>> the RAMBlock (which must be a multiple of TARGET_PAGE_SIZE).
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> ---
>> include/exec/memory.h | 8 ++++++--
>> exec.c | 3 +++
>> hw/display/g364fb.c | 2 +-
>> hw/vfio/common.c | 3 ++-
>> hw/virtio/vhost-user.c | 2 +-
>> memory.c | 10 ++++++----
>> 6 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index eb4f2fb249..03f257829b 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -700,6 +700,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>> * must be unique within any device
>> * @size: size of the region.
>> * @ptr: memory to be mapped; must contain at least @size bytes.
> ^^^^^
>
> this comment gets wrong with your patches
why? it must contain at least @size (And preferrably exactly @ptr_size)
>
>> + * @ptr_size: size of @ptr buffer
>
>> diff --git a/exec.c b/exec.c
>> index 6826c8337d..fcea614e79 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2361,6 +2361,9 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size,
>> ram_addr_t max_size,
>> RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>> MemoryRegion *mr, Error **errp)
>> {
>> + assert(size >= TARGET_PAGE_SIZE);
>> + assert(size % TARGET_PAGE_SIZE == 0);
>> +
>> return qemu_ram_alloc_internal(size, size, NULL, host, false,
>> false, mr, errp);
>> }
>
> ok with this bit.
>
> But how about to change instead to:
>
> void memory_region_init_ram_ptr(MemoryRegion *mr,
> Object *owner,
> const char *name,
> uint64_t size,
> void *ptr)
> {
> uint64_t real_size = ROUND_UP_TARGET_PAGE_SIZE(size);
> memory_region_init(mr, owner, name, real_size);
> mr->ram = true;
> mr->terminates = true;
> mr->destructor = memory_region_destructor_ram;
> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>
> /* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
> assert(ptr != NULL);
> mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_fatal);
> }
>
> For a suitable ROUND_UP_TARGET_PAGE_SIZE() macro. You get the idea.
>
> And memory_region_device_ram_ptr() don't even need a change.
> We need to adjust the comments, but it looks like an easier patch to me, no?
That's a good suggestion, it puts a bit more responsability into
caller side, put that's fair.
>
>> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
>> index fbc2b2422d..f4f5643761 100644
>> --- a/hw/display/g364fb.c
>> +++ b/hw/display/g364fb.c
>> @@ -475,7 +475,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
>>
>> memory_region_init_io(&s->mem_ctrl, NULL, &g364fb_ctrl_ops, s, "ctrl",
>> 0x180000);
>> memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram",
>> - s->vram_size, s->vram);
>> + s->vram_size, s->vram, s->vram_size);
>
> Having to change all the devices that use the function with exactly the
> same parameter looks weird to me.
>
> What do you think?
I'll update the patch, thanks
