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.lureau@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.
+ * @ptr_size: size of @ptr buffer
*
* Note that this function does not do anything to cause the data in the
* RAM memory region to be migrated; that is the responsibility of the caller.
@@ -708,7 +709,8 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
struct Object *owner,
const char *name,
uint64_t size,
- void *ptr);
+ void *ptr,
+ uint64_t ptr_size);
/**
* memory_region_init_ram_device_ptr: Initialize RAM device memory region from
@@ -727,6 +729,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
* @name: the name of the region.
* @size: size of the region.
* @ptr: memory to be mapped; must contain at least @size bytes.
+ * @ptr_size: size of @ptr buffer
*
* Note that this function does not do anything to cause the data in the
* RAM memory region to be migrated; that is the responsibility of the caller.
@@ -736,7 +739,8 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr,
struct Object *owner,
const char *name,
uint64_t size,
- void *ptr);
+ void *ptr,
+ uint64_t ptr_size);
/**
* memory_region_init_alias: Initialize a memory region that aliases all or a
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);
}
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);
vmstate_register_ram(&s->mem_vram, dev);
memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
}
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7c185e5a2e..e5dfbbafe3 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -854,7 +854,8 @@ int vfio_region_mmap(VFIORegion *region)
memory_region_init_ram_device_ptr(®ion->mmaps[i].mem,
memory_region_owner(region->mem),
name, region->mmaps[i].size,
- region->mmaps[i].mmap);
+ region->mmaps[i].mmap,
+ region->mmaps[i].size);
g_free(name);
memory_region_add_subregion(region->mem, region->mmaps[i].offset,
®ion->mmaps[i].mem);
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b041343632..3abe960f29 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -935,7 +935,7 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
name = g_strdup_printf("vhost-user/host-notifier@%p mmaps[%d]",
user, queue_idx);
memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
- page_size, addr);
+ page_size, addr, page_size);
g_free(name);
if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
diff --git a/memory.c b/memory.c
index 9b73892768..07f8458d38 100644
--- a/memory.c
+++ b/memory.c
@@ -1587,7 +1587,8 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
Object *owner,
const char *name,
uint64_t size,
- void *ptr)
+ void *ptr,
+ uint64_t ptr_size)
{
memory_region_init(mr, owner, name, size);
mr->ram = true;
@@ -1597,16 +1598,17 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
/* 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);
+ mr->ram_block = qemu_ram_alloc_from_ptr(ptr_size, ptr, mr, &error_fatal);
}
void memory_region_init_ram_device_ptr(MemoryRegion *mr,
Object *owner,
const char *name,
uint64_t size,
- void *ptr)
+ void *ptr,
+ uint64_t ptr_size)
{
- memory_region_init_ram_ptr(mr, owner, name, size, ptr);
+ memory_region_init_ram_ptr(mr, owner, name, size, ptr, ptr_size);
mr->ram_device = true;
mr->ops = &ram_device_mem_ops;
mr->opaque = mr;
--
2.19.0.rc1
Marc-André Lureau <marcandre.lureau@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.lureau@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
> + * @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?
> 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?
Later, Juan.
Hi
On Wed, Sep 5, 2018 at 1:37 PM, Juan Quintela <quintela@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@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.lureau@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
Hi
On Wed, Sep 5, 2018 at 7:51 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> Hi
>
> On Wed, Sep 5, 2018 at 1:37 PM, Juan Quintela <quintela@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@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.lureau@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
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
© 2016 - 2025 Red Hat, Inc.