* 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