* [Qemu-devel] [PATCH] memory: size can be different from ptr_size
@ 2018-09-05 8:29 Marc-André Lureau
2018-09-05 11:37 ` Juan Quintela
0 siblings, 1 reply; 4+ messages in thread
From: Marc-André Lureau @ 2018-09-05 8:29 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, dgilbert, quintela, Marc-André Lureau,
Paolo Bonzini, Peter Crosthwaite, Richard Henderson,
Alex Williamson, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: size can be different from ptr_size
2018-09-05 8:29 [Qemu-devel] [PATCH] memory: size can be different from ptr_size Marc-André Lureau
@ 2018-09-05 11:37 ` Juan Quintela
2018-09-05 17:51 ` Marc-André Lureau
0 siblings, 1 reply; 4+ messages in thread
From: Juan Quintela @ 2018-09-05 11:37 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, imammedo, dgilbert, Paolo Bonzini, Peter Crosthwaite,
Richard Henderson, Alex Williamson, Michael S. Tsirkin
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: size can be different from ptr_size
2018-09-05 11:37 ` Juan Quintela
@ 2018-09-05 17:51 ` Marc-André Lureau
2018-09-05 19:45 ` Marc-André Lureau
0 siblings, 1 reply; 4+ messages in thread
From: Marc-André Lureau @ 2018-09-05 17:51 UTC (permalink / raw)
To: Juan Quintela
Cc: qemu-devel, Igor Mammedov, Dr. David Alan Gilbert, Paolo Bonzini,
Peter Crosthwaite, Richard Henderson, Alex Williamson,
Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] memory: size can be different from ptr_size
2018-09-05 17:51 ` Marc-André Lureau
@ 2018-09-05 19:45 ` Marc-André Lureau
0 siblings, 0 replies; 4+ messages in thread
From: Marc-André Lureau @ 2018-09-05 19:45 UTC (permalink / raw)
To: Juan Quintela
Cc: qemu-devel, Igor Mammedov, Dr. David Alan Gilbert, Paolo Bonzini,
Peter Crosthwaite, Richard Henderson, Alex Williamson,
Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-05 19:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-05 8:29 [Qemu-devel] [PATCH] memory: size can be different from ptr_size Marc-André Lureau
2018-09-05 11:37 ` Juan Quintela
2018-09-05 17:51 ` Marc-André Lureau
2018-09-05 19:45 ` Marc-André Lureau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.