* [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length
@ 2017-07-26 16:53 Anthony PERARD
2017-07-26 17:09 ` Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Anthony PERARD @ 2017-07-26 16:53 UTC (permalink / raw)
To: qemu-devel
Cc: Stefano Stabellini, Anthony PERARD, Paolo Bonzini,
Peter Crosthwaite, Richard Henderson
Commit 04bf2526ce87f21b32c9acba1c5518708c243ad0 (exec: use
qemu_ram_ptr_length to access guest ram) start using qemu_ram_ptr_length
instead of qemu_map_ram_ptr, but when used with Xen, the behavior of
both function is different. They both call xen_map_cache, but one with
"lock", meaning the mapping of guest memory is never released
implicitly, and the second one without, which means, mapping can be
release later, when needed.
In the context of address_space_{read,write}_continue, the ptr to those
mapping should not be locked because it is used immediatly and never
used again.
The lock parameter make it explicit in which context qemu_ram_ptr_length
is called.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
exec.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/exec.c b/exec.c
index 01ac21e3cd..63508cd35e 100644
--- a/exec.c
+++ b/exec.c
@@ -2203,7 +2203,7 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
* Called within RCU critical section.
*/
static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
- hwaddr *size)
+ hwaddr *size, bool lock)
{
RAMBlock *block = ram_block;
if (*size == 0) {
@@ -2222,10 +2222,10 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
* In that case just map the requested area.
*/
if (block->offset == 0) {
- return xen_map_cache(addr, *size, 1, true);
+ return xen_map_cache(addr, *size, lock ? 1 : 0, lock);
}
- block->host = xen_map_cache(block->offset, block->max_length, 1, true);
+ block->host = xen_map_cache(block->offset, block->max_length, 1, lock);
}
return ramblock_ptr(block, addr);
@@ -2947,7 +2947,7 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
}
} else {
/* RAM case */
- ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
+ ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
memcpy(ptr, buf, l);
invalidate_and_set_dirty(mr, addr1, l);
}
@@ -3038,7 +3038,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
}
} else {
/* RAM case */
- ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
+ ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
memcpy(buf, ptr, l);
}
@@ -3349,7 +3349,7 @@ void *address_space_map(AddressSpace *as,
memory_region_ref(mr);
*plen = address_space_extend_translation(as, addr, len, mr, xlat, l, is_write);
- ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen);
+ ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
rcu_read_unlock();
return ptr;
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length
2017-07-26 16:53 [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length Anthony PERARD
@ 2017-07-26 17:09 ` Paolo Bonzini
2017-07-26 17:14 ` Stefano Stabellini
2017-07-27 14:23 ` Paolo Bonzini
2 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2017-07-26 17:09 UTC (permalink / raw)
To: Anthony PERARD, qemu-devel
Cc: Richard Henderson, Stefano Stabellini, Peter Crosthwaite
On 26/07/2017 18:53, Anthony PERARD wrote:
> Commit 04bf2526ce87f21b32c9acba1c5518708c243ad0 (exec: use
> qemu_ram_ptr_length to access guest ram) start using qemu_ram_ptr_length
> instead of qemu_map_ram_ptr, but when used with Xen, the behavior of
> both function is different. They both call xen_map_cache, but one with
> "lock", meaning the mapping of guest memory is never released
> implicitly, and the second one without, which means, mapping can be
> release later, when needed.
>
> In the context of address_space_{read,write}_continue, the ptr to those
> mapping should not be locked because it is used immediatly and never
> used again.
>
> The lock parameter make it explicit in which context qemu_ram_ptr_length
> is called.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks for implementing it. I'll send a pull request for rc1.
Paolo
> ---
> exec.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 01ac21e3cd..63508cd35e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2203,7 +2203,7 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
> * Called within RCU critical section.
> */
> static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> - hwaddr *size)
> + hwaddr *size, bool lock)
> {
> RAMBlock *block = ram_block;
> if (*size == 0) {
> @@ -2222,10 +2222,10 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> * In that case just map the requested area.
> */
> if (block->offset == 0) {
> - return xen_map_cache(addr, *size, 1, true);
> + return xen_map_cache(addr, *size, lock ? 1 : 0, lock);
> }
>
> - block->host = xen_map_cache(block->offset, block->max_length, 1, true);
> + block->host = xen_map_cache(block->offset, block->max_length, 1, lock);
> }
>
> return ramblock_ptr(block, addr);
> @@ -2947,7 +2947,7 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
> }
> } else {
> /* RAM case */
> - ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
> + ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
> memcpy(ptr, buf, l);
> invalidate_and_set_dirty(mr, addr1, l);
> }
> @@ -3038,7 +3038,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
> }
> } else {
> /* RAM case */
> - ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
> + ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
> memcpy(buf, ptr, l);
> }
>
> @@ -3349,7 +3349,7 @@ void *address_space_map(AddressSpace *as,
>
> memory_region_ref(mr);
> *plen = address_space_extend_translation(as, addr, len, mr, xlat, l, is_write);
> - ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen);
> + ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
> rcu_read_unlock();
>
> return ptr;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length
2017-07-26 16:53 [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length Anthony PERARD
2017-07-26 17:09 ` Paolo Bonzini
@ 2017-07-26 17:14 ` Stefano Stabellini
2017-07-27 14:23 ` Paolo Bonzini
2 siblings, 0 replies; 4+ messages in thread
From: Stefano Stabellini @ 2017-07-26 17:14 UTC (permalink / raw)
To: Anthony PERARD
Cc: qemu-devel, Stefano Stabellini, Paolo Bonzini, Peter Crosthwaite,
Richard Henderson
On Wed, 26 Jul 2017, Anthony PERARD wrote:
> Commit 04bf2526ce87f21b32c9acba1c5518708c243ad0 (exec: use
> qemu_ram_ptr_length to access guest ram) start using qemu_ram_ptr_length
> instead of qemu_map_ram_ptr, but when used with Xen, the behavior of
> both function is different. They both call xen_map_cache, but one with
> "lock", meaning the mapping of guest memory is never released
> implicitly, and the second one without, which means, mapping can be
> release later, when needed.
>
> In the context of address_space_{read,write}_continue, the ptr to those
> mapping should not be locked because it is used immediatly and never
> used again.
>
> The lock parameter make it explicit in which context qemu_ram_ptr_length
> is called.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> exec.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 01ac21e3cd..63508cd35e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2203,7 +2203,7 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
> * Called within RCU critical section.
> */
> static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> - hwaddr *size)
> + hwaddr *size, bool lock)
> {
> RAMBlock *block = ram_block;
> if (*size == 0) {
> @@ -2222,10 +2222,10 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> * In that case just map the requested area.
> */
> if (block->offset == 0) {
> - return xen_map_cache(addr, *size, 1, true);
> + return xen_map_cache(addr, *size, lock ? 1 : 0, lock);
> }
>
> - block->host = xen_map_cache(block->offset, block->max_length, 1, true);
> + block->host = xen_map_cache(block->offset, block->max_length, 1, lock);
> }
>
> return ramblock_ptr(block, addr);
> @@ -2947,7 +2947,7 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
> }
> } else {
> /* RAM case */
> - ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
> + ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
> memcpy(ptr, buf, l);
> invalidate_and_set_dirty(mr, addr1, l);
> }
> @@ -3038,7 +3038,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
> }
> } else {
> /* RAM case */
> - ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
> + ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
> memcpy(buf, ptr, l);
> }
>
> @@ -3349,7 +3349,7 @@ void *address_space_map(AddressSpace *as,
>
> memory_region_ref(mr);
> *plen = address_space_extend_translation(as, addr, len, mr, xlat, l, is_write);
> - ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen);
> + ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
> rcu_read_unlock();
>
> return ptr;
> --
> Anthony PERARD
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length
2017-07-26 16:53 [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length Anthony PERARD
2017-07-26 17:09 ` Paolo Bonzini
2017-07-26 17:14 ` Stefano Stabellini
@ 2017-07-27 14:23 ` Paolo Bonzini
2 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2017-07-27 14:23 UTC (permalink / raw)
To: Anthony PERARD, qemu-devel
Cc: Stefano Stabellini, Peter Crosthwaite, Richard Henderson
On 26/07/2017 18:53, Anthony PERARD wrote:
> Commit 04bf2526ce87f21b32c9acba1c5518708c243ad0 (exec: use
> qemu_ram_ptr_length to access guest ram) start using qemu_ram_ptr_length
> instead of qemu_map_ram_ptr, but when used with Xen, the behavior of
> both function is different. They both call xen_map_cache, but one with
> "lock", meaning the mapping of guest memory is never released
> implicitly, and the second one without, which means, mapping can be
> release later, when needed.
>
> In the context of address_space_{read,write}_continue, the ptr to those
> mapping should not be locked because it is used immediatly and never
> used again.
>
> The lock parameter make it explicit in which context qemu_ram_ptr_length
> is called.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> exec.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 01ac21e3cd..63508cd35e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2203,7 +2203,7 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
> * Called within RCU critical section.
> */
> static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> - hwaddr *size)
> + hwaddr *size, bool lock)
> {
> RAMBlock *block = ram_block;
> if (*size == 0) {
> @@ -2222,10 +2222,10 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> * In that case just map the requested area.
> */
> if (block->offset == 0) {
> - return xen_map_cache(addr, *size, 1, true);
> + return xen_map_cache(addr, *size, lock ? 1 : 0, lock);
> }
>
> - block->host = xen_map_cache(block->offset, block->max_length, 1, true);
> + block->host = xen_map_cache(block->offset, block->max_length, 1, lock);
> }
>
> return ramblock_ptr(block, addr);
> @@ -2947,7 +2947,7 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
> }
> } else {
> /* RAM case */
> - ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
> + ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
> memcpy(ptr, buf, l);
> invalidate_and_set_dirty(mr, addr1, l);
> }
> @@ -3038,7 +3038,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
> }
> } else {
> /* RAM case */
> - ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
> + ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
> memcpy(buf, ptr, l);
> }
>
> @@ -3349,7 +3349,7 @@ void *address_space_map(AddressSpace *as,
>
> memory_region_ref(mr);
> *plen = address_space_extend_translation(as, addr, len, mr, xlat, l, is_write);
> - ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen);
> + ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
> rcu_read_unlock();
>
> return ptr;
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-07-27 14:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-26 16:53 [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length Anthony PERARD
2017-07-26 17:09 ` Paolo Bonzini
2017-07-26 17:14 ` Stefano Stabellini
2017-07-27 14:23 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).