[PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback

Anthony PERARD posted 1 patch 4 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191219154323.325255-1-anthony.perard@citrix.com
Test asan failed
Test checkpatch failed
Test FreeBSD failed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 failed
Maintainers: Richard Henderson <rth@twiddle.net>, Paolo Bonzini <pbonzini@redhat.com>
exec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback
Posted by Anthony PERARD 4 years, 3 months ago
It is possible that a ramblock doesn't have memory that QEMU can
access, this is the case with the Xen hypervisor.

In order to avoid to trigger an assert, only call ramblock_ptr() when
needed in qemu_ram_writeback(). This should fix migration of Xen
guests that was broken with bd108a44bc29 ("migration: ram: Switch to
ram block writeback").

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 exec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index a34c34818404..b11010e0cb4c 100644
--- a/exec.c
+++ b/exec.c
@@ -2166,14 +2166,13 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
  */
 void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
 {
-    void *addr = ramblock_ptr(block, start);
-
     /* The requested range should fit in within the block range */
     g_assert((start + length) <= block->used_length);
 
 #ifdef CONFIG_LIBPMEM
     /* The lack of support for pmem should not block the sync */
     if (ramblock_is_pmem(block)) {
+        void *addr = ramblock_ptr(block, start);
         pmem_persist(addr, length);
         return;
     }
@@ -2184,6 +2183,7 @@ void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
          * specified as persistent (or is not one) - use the msync.
          * Less optimal but still achieves the same goal
          */
+        void *addr = ramblock_ptr(block, start);
         if (qemu_msync(addr, length, block->fd)) {
             warn_report("%s: failed to sync memory range: start: "
                     RAM_ADDR_FMT " length: " RAM_ADDR_FMT,
-- 
Anthony PERARD


Re: [PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback
Posted by Juan Quintela 4 years, 3 months ago
Anthony PERARD <anthony.perard@citrix.com> wrote:
> It is possible that a ramblock doesn't have memory that QEMU can
> access, this is the case with the Xen hypervisor.
>
> In order to avoid to trigger an assert, only call ramblock_ptr() when
> needed in qemu_ram_writeback(). This should fix migration of Xen
> guests that was broken with bd108a44bc29 ("migration: ram: Switch to
> ram block writeback").
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

This is exec.c, nothing related to migration.

Paolo, are you taking this one?
It could even go through the trivial one.

Thanks, Juan.


Re: [PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback
Posted by Anthony PERARD 4 years, 1 month ago
On Thu, Dec 19, 2019 at 07:10:24PM +0100, Juan Quintela wrote:
> Anthony PERARD <anthony.perard@citrix.com> wrote:
> > It is possible that a ramblock doesn't have memory that QEMU can
> > access, this is the case with the Xen hypervisor.
> >
> > In order to avoid to trigger an assert, only call ramblock_ptr() when
> > needed in qemu_ram_writeback(). This should fix migration of Xen
> > guests that was broken with bd108a44bc29 ("migration: ram: Switch to
> > ram block writeback").
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> This is exec.c, nothing related to migration.
> 
> Paolo, are you taking this one?
> It could even go through the trivial one.

Hi,

I'm going to send a pull request for the xen queue with this patch.
Unless that's an issue?

Cheers,

-- 
Anthony PERARD

Re: [PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback
Posted by Paolo Bonzini 4 years, 1 month ago
On 25/02/20 17:02, Anthony PERARD wrote:
> On Thu, Dec 19, 2019 at 07:10:24PM +0100, Juan Quintela wrote:
>> Anthony PERARD <anthony.perard@citrix.com> wrote:
>>> It is possible that a ramblock doesn't have memory that QEMU can
>>> access, this is the case with the Xen hypervisor.
>>>
>>> In order to avoid to trigger an assert, only call ramblock_ptr() when
>>> needed in qemu_ram_writeback(). This should fix migration of Xen
>>> guests that was broken with bd108a44bc29 ("migration: ram: Switch to
>>> ram block writeback").
>>>
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>
>> This is exec.c, nothing related to migration.
>>
>> Paolo, are you taking this one?
>> It could even go through the trivial one.
> 
> Hi,
> 
> I'm going to send a pull request for the xen queue with this patch.
> Unless that's an issue?

No, absolutely.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo


Re: [PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback
Posted by Beata Michalska 4 years, 3 months ago
Hi Anthony,

On Thu, 19 Dec 2019 at 15:43, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> It is possible that a ramblock doesn't have memory that QEMU can
> access, this is the case with the Xen hypervisor.
>
> In order to avoid to trigger an assert, only call ramblock_ptr() when
> needed in qemu_ram_writeback(). This should fix migration of Xen
> guests that was broken with bd108a44bc29 ("migration: ram: Switch to
> ram block writeback").
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  exec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index a34c34818404..b11010e0cb4c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2166,14 +2166,13 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
>   */
>  void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
>  {
> -    void *addr = ramblock_ptr(block, start);
> -
>      /* The requested range should fit in within the block range */
>      g_assert((start + length) <= block->used_length);
>
>  #ifdef CONFIG_LIBPMEM
>      /* The lack of support for pmem should not block the sync */
>      if (ramblock_is_pmem(block)) {
> +        void *addr = ramblock_ptr(block, start);
>          pmem_persist(addr, length);
>          return;
>      }
> @@ -2184,6 +2183,7 @@ void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
>           * specified as persistent (or is not one) - use the msync.
>           * Less optimal but still achieves the same goal
>           */
> +        void *addr = ramblock_ptr(block, start);
>          if (qemu_msync(addr, length, block->fd)) {
>              warn_report("%s: failed to sync memory range: start: "
>                      RAM_ADDR_FMT " length: " RAM_ADDR_FMT,

We could also do :
void *addr = block->host ? ramblock_ptr : NULL

Looks good to me thought.
Thanks for fixing.

BR

Beata
> --
> Anthony PERARD
>