Re: [Qemu-devel] [PATCH] memory: size can be different from ptr_size

[Marc-André Lureau]

Hi

On Wed, Sep 5, 2018 at 7:51 PM, Marc-André Lureau
<address@hidden> wrote:
> Hi
>
> On Wed, Sep 5, 2018 at 1:37 PM, Juan Quintela <address@hidden> wrote:
>> Marc-André Lureau <address@hidden> 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 <address@hidden>
>>> ---
>>>  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

Actually the patch is a bit pointless, since qemu_ram_alloc_internal()
already HOST_PAGE_ALIGN(size). And host page is supposed to be larger
than target page. So, I can simply over-allocate the ram ptr/device
buffer in the TPM PPI patch.

thanks