[PATCH v2 0/2] Fix dumping in kdump format with non-aligned memory

[PATCH v2 0/2] Fix dumping in kdump format with non-aligned memory

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

dump.c:get_next_page expects GuestPhysBlock to be page-aligned, and crashes over
memory regions such as "tpm-crb-cmd". Teach it to handle non-aligned regions
too, by using a caller pre-allocated filled up page as necessary.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2120480

v2:
 - drop some unnecessary changes in the first patch
 - use pre-allocated caller memory, instead of allocating in get_next_page()
 - fix some comment

Marc-André Lureau (2):
  dump: simplify a bit kdump get_next_page()
  dump: fix kdump to work over non-aligned blocks

 dump/dump.c | 88 +++++++++++++++++++++++++++++++++++------------------
 roms/SLOF   |  2 +-
 2 files changed, 59 insertions(+), 31 deletions(-)

-- 
2.37.2

[PATCH v2 1/2] dump: simplify a bit kdump get_next_page()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This should be functionally equivalent, but slightly easier to read,
with simplified paths and checks at the end of the function.

The following patch is a major rewrite to get rid of the assert().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 dump/dump.c | 21 ++++++++-------------
 roms/SLOF   |  2 +-
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 4d9658ffa2..f465830371 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1110,17 +1110,11 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
     if (!block) {
         block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
         *blockptr = block;
-        assert((block->target_start & ~target_page_mask) == 0);
-        assert((block->target_end & ~target_page_mask) == 0);
-        *pfnptr = dump_paddr_to_pfn(s, block->target_start);
-        if (bufptr) {
-            *bufptr = block->host_addr;
-        }
-        return true;
+        addr = block->target_start;
+    } else {
+        addr = dump_pfn_to_paddr(s, *pfnptr + 1);
     }
-
-    *pfnptr = *pfnptr + 1;
-    addr = dump_pfn_to_paddr(s, *pfnptr);
+    assert(block != NULL);
 
     if ((addr >= block->target_start) &&
         (addr + s->dump_info.page_size <= block->target_end)) {
@@ -1132,12 +1126,13 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
         if (!block) {
             return false;
         }
-        assert((block->target_start & ~target_page_mask) == 0);
-        assert((block->target_end & ~target_page_mask) == 0);
-        *pfnptr = dump_paddr_to_pfn(s, block->target_start);
+        addr = block->target_start;
         buf = block->host_addr;
     }
 
+    assert((block->target_start & ~target_page_mask) == 0);
+    assert((block->target_end & ~target_page_mask) == 0);
+    *pfnptr = dump_paddr_to_pfn(s, addr);
     if (bufptr) {
         *bufptr = buf;
     }
diff --git a/roms/SLOF b/roms/SLOF
index 6b6c16b4b4..5b4c5acdcd 160000
--- a/roms/SLOF
+++ b/roms/SLOF
@@ -1 +1 @@
-Subproject commit 6b6c16b4b40763507cf1f518096f3c3883c5cf2d
+Subproject commit 5b4c5acdcd552a4e1796aeca6bb700f6cbb0282d
-- 
2.37.2

Re: [PATCH v2 1/2] dump: simplify a bit kdump get_next_page()

[David Hildenbrand]

On 05.09.22 14:57, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This should be functionally equivalent, but slightly easier to read,
> with simplified paths and checks at the end of the function.
> 
> The following patch is a major rewrite to get rid of the assert().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---

Thanks!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/2] dump: simplify a bit kdump get_next_page()

[Peter Maydell]

On Mon, 5 Sept 2022 at 13:57, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This should be functionally equivalent, but slightly easier to read,
> with simplified paths and checks at the end of the function.
>
> The following patch is a major rewrite to get rid of the assert().
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  dump/dump.c | 21 ++++++++-------------
>  roms/SLOF   |  2 +-
>  2 files changed, 9 insertions(+), 14 deletions(-)

> diff --git a/roms/SLOF b/roms/SLOF
> index 6b6c16b4b4..5b4c5acdcd 160000
> --- a/roms/SLOF
> +++ b/roms/SLOF
> @@ -1 +1 @@
> -Subproject commit 6b6c16b4b40763507cf1f518096f3c3883c5cf2d
> +Subproject commit 5b4c5acdcd552a4e1796aeca6bb700f6cbb0282d

This shouldn't be in here, right? (I'm guessing a rebase
accident -- git submodules have terrible ergonomics.)

-- PMM

Re: [PATCH v2 1/2] dump: simplify a bit kdump get_next_page()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

Hi

On Mon, Sep 5, 2022 at 5:28 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 5 Sept 2022 at 13:57, <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > This should be functionally equivalent, but slightly easier to read,
> > with simplified paths and checks at the end of the function.
> >
> > The following patch is a major rewrite to get rid of the assert().
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  dump/dump.c | 21 ++++++++-------------
> >  roms/SLOF   |  2 +-
> >  2 files changed, 9 insertions(+), 14 deletions(-)
>
> > diff --git a/roms/SLOF b/roms/SLOF
> > index 6b6c16b4b4..5b4c5acdcd 160000
> > --- a/roms/SLOF
> > +++ b/roms/SLOF
> > @@ -1 +1 @@
> > -Subproject commit 6b6c16b4b40763507cf1f518096f3c3883c5cf2d
> > +Subproject commit 5b4c5acdcd552a4e1796aeca6bb700f6cbb0282d
>
> This shouldn't be in here, right? (I'm guessing a rebase
> accident -- git submodules have terrible ergonomics.)
>

indeed, my bad will fix in v3 (or eventually in PR?)

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1874 bytes --]

[PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Rewrite get_next_page() to work over non-aligned blocks. When it
encounters non aligned addresses, it will try to fill a page provided by
the caller.

This solves a kdump crash with "tpm-crb-cmd" RAM memory region,
qemu-kvm: ../dump/dump.c:1162: _Bool get_next_page(GuestPhysBlock **,
uint64_t *, uint8_t **, DumpState *): Assertion `(block->target_start &
~target_page_mask) == 0' failed.

because:
guest_phys_block_add_section: target_start=00000000fed40080 target_end=00000000fed41000: added (count: 4)

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2120480

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 dump/dump.c | 79 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 56 insertions(+), 23 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index f465830371..500357bafe 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1094,50 +1094,81 @@ static uint64_t dump_pfn_to_paddr(DumpState *s, uint64_t pfn)
 }
 
 /*
- * exam every page and return the page frame number and the address of the page.
- * bufptr can be NULL. note: the blocks here is supposed to reflect guest-phys
- * blocks, so block->target_start and block->target_end should be interal
- * multiples of the target page size.
+ * Return the page frame number and the page content in *bufptr. bufptr can be
+ * NULL. If not NULL, *bufptr must contains a target page size of pre-allocated
+ * memory. This is not necessarily the memory returned.
  */
 static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
                           uint8_t **bufptr, DumpState *s)
 {
     GuestPhysBlock *block = *blockptr;
-    hwaddr addr, target_page_mask = ~((hwaddr)s->dump_info.page_size - 1);
-    uint8_t *buf;
+    uint32_t page_size = s->dump_info.page_size;
+    uint8_t *buf = NULL, *hbuf;
+    hwaddr addr;
 
     /* block == NULL means the start of the iteration */
     if (!block) {
         block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
         *blockptr = block;
         addr = block->target_start;
+        *pfnptr = dump_paddr_to_pfn(s, addr);
     } else {
-        addr = dump_pfn_to_paddr(s, *pfnptr + 1);
+        *pfnptr += 1;
+        addr = dump_pfn_to_paddr(s, *pfnptr);
     }
     assert(block != NULL);
 
-    if ((addr >= block->target_start) &&
-        (addr + s->dump_info.page_size <= block->target_end)) {
-        buf = block->host_addr + (addr - block->target_start);
-    } else {
-        /* the next page is in the next block */
-        block = QTAILQ_NEXT(block, next);
-        *blockptr = block;
-        if (!block) {
-            return false;
+    while (1) {
+        if (addr >= block->target_start && addr < block->target_end) {
+            size_t n = MIN(block->target_end - addr, page_size - addr % page_size);
+            hbuf = block->host_addr + (addr - block->target_start);
+            if (!buf) {
+                if (n == page_size) {
+                    /* this is a whole target page, go for it */
+                    assert(addr % page_size == 0);
+                    buf = hbuf;
+                    break;
+                } else if (bufptr) {
+                    assert(*bufptr);
+                    buf = *bufptr;
+                    memset(buf, 0, page_size);
+                } else {
+                    return true;
+                }
+            }
+
+            memcpy(buf + addr % page_size, hbuf, n);
+            addr += n;
+            if (addr % page_size == 0) {
+                /* we filled up the page */
+                break;
+            }
+        } else {
+            /* the next page is in the next block */
+            *blockptr = block = QTAILQ_NEXT(block, next);
+            if (!block) {
+                break;
+            }
+
+            addr = block->target_start;
+            /* are we still in the same page? */
+            if (dump_paddr_to_pfn(s, addr) != *pfnptr) {
+                if (buf) {
+                    /* no, but we already filled something earlier, return it */
+                    break;
+                } else {
+                    /* else continue from there */
+                    *pfnptr = dump_paddr_to_pfn(s, addr);
+                }
+            }
         }
-        addr = block->target_start;
-        buf = block->host_addr;
     }
 
-    assert((block->target_start & ~target_page_mask) == 0);
-    assert((block->target_end & ~target_page_mask) == 0);
-    *pfnptr = dump_paddr_to_pfn(s, addr);
     if (bufptr) {
         *bufptr = buf;
     }
 
-    return true;
+    return buf != NULL;
 }
 
 static void write_dump_bitmap(DumpState *s, Error **errp)
@@ -1275,6 +1306,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
     uint8_t *buf;
     GuestPhysBlock *block_iter = NULL;
     uint64_t pfn_iter;
+    g_autofree uint8_t *page = NULL;
 
     /* get offset of page_desc and page_data in dump file */
     offset_desc = s->offset_page;
@@ -1310,12 +1342,13 @@ static void write_dump_pages(DumpState *s, Error **errp)
     }
 
     offset_data += s->dump_info.page_size;
+    page = g_malloc(s->dump_info.page_size);
 
     /*
      * dump memory to vmcore page by page. zero page will all be resided in the
      * first page of page section
      */
-    while (get_next_page(&block_iter, &pfn_iter, &buf, s)) {
+    for (buf = page; get_next_page(&block_iter, &pfn_iter, &buf, s); buf = page) {
         /* check zero page */
         if (buffer_is_zero(buf, s->dump_info.page_size)) {
             ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
-- 
2.37.2

Re: [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks

[Stefan Berger]

On 9/5/22 08:57, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Rewrite get_next_page() to work over non-aligned blocks. When it
> encounters non aligned addresses, it will try to fill a page provided by
> the caller.
> 
> This solves a kdump crash with "tpm-crb-cmd" RAM memory region,
> qemu-kvm: ../dump/dump.c:1162: _Bool get_next_page(GuestPhysBlock **,
> uint64_t *, uint8_t **, DumpState *): Assertion `(block->target_start &
> ~target_page_mask) == 0' failed.
> 
> because:
> guest_phys_block_add_section: target_start=00000000fed40080 target_end=00000000fed41000: added (count: 4)
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=2120480
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   dump/dump.c | 79 +++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 56 insertions(+), 23 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index f465830371..500357bafe 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1094,50 +1094,81 @@ static uint64_t dump_pfn_to_paddr(DumpState *s, uint64_t pfn)
>   }
> 
>   /*
> - * exam every page and return the page frame number and the address of the page.
> - * bufptr can be NULL. note: the blocks here is supposed to reflect guest-phys
> - * blocks, so block->target_start and block->target_end should be interal
> - * multiples of the target page size.
> + * Return the page frame number and the page content in *bufptr. bufptr can be
> + * NULL. If not NULL, *bufptr must contains a target page size of pre-allocated

contains->contain

Otherwise I don't have much to say about it...

Re: [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 6264 bytes --]

Hi

On Mon, Sep 5, 2022 at 5:13 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Rewrite get_next_page() to work over non-aligned blocks. When it
> encounters non aligned addresses, it will try to fill a page provided by
> the caller.
>
> This solves a kdump crash with "tpm-crb-cmd" RAM memory region,
> qemu-kvm: ../dump/dump.c:1162: _Bool get_next_page(GuestPhysBlock **,
> uint64_t *, uint8_t **, DumpState *): Assertion `(block->target_start &
> ~target_page_mask) == 0' failed.
>
> because:
> guest_phys_block_add_section: target_start=00000000fed40080
> target_end=00000000fed41000: added (count: 4)
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=2120480
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>

ping: someone to review/ack this patch?


> ---
>  dump/dump.c | 79 +++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 56 insertions(+), 23 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index f465830371..500357bafe 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1094,50 +1094,81 @@ static uint64_t dump_pfn_to_paddr(DumpState *s,
> uint64_t pfn)
>  }
>
>  /*
> - * exam every page and return the page frame number and the address of
> the page.
> - * bufptr can be NULL. note: the blocks here is supposed to reflect
> guest-phys
> - * blocks, so block->target_start and block->target_end should be interal
> - * multiples of the target page size.
> + * Return the page frame number and the page content in *bufptr. bufptr
> can be
> + * NULL. If not NULL, *bufptr must contains a target page size of
> pre-allocated
> + * memory. This is not necessarily the memory returned.
>   */
>  static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
>                            uint8_t **bufptr, DumpState *s)
>  {
>      GuestPhysBlock *block = *blockptr;
> -    hwaddr addr, target_page_mask = ~((hwaddr)s->dump_info.page_size - 1);
> -    uint8_t *buf;
> +    uint32_t page_size = s->dump_info.page_size;
> +    uint8_t *buf = NULL, *hbuf;
> +    hwaddr addr;
>
>      /* block == NULL means the start of the iteration */
>      if (!block) {
>          block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>          *blockptr = block;
>          addr = block->target_start;
> +        *pfnptr = dump_paddr_to_pfn(s, addr);
>      } else {
> -        addr = dump_pfn_to_paddr(s, *pfnptr + 1);
> +        *pfnptr += 1;
> +        addr = dump_pfn_to_paddr(s, *pfnptr);
>      }
>      assert(block != NULL);
>
> -    if ((addr >= block->target_start) &&
> -        (addr + s->dump_info.page_size <= block->target_end)) {
> -        buf = block->host_addr + (addr - block->target_start);
> -    } else {
> -        /* the next page is in the next block */
> -        block = QTAILQ_NEXT(block, next);
> -        *blockptr = block;
> -        if (!block) {
> -            return false;
> +    while (1) {
> +        if (addr >= block->target_start && addr < block->target_end) {
> +            size_t n = MIN(block->target_end - addr, page_size - addr %
> page_size);
> +            hbuf = block->host_addr + (addr - block->target_start);
> +            if (!buf) {
> +                if (n == page_size) {
> +                    /* this is a whole target page, go for it */
> +                    assert(addr % page_size == 0);
> +                    buf = hbuf;
> +                    break;
> +                } else if (bufptr) {
> +                    assert(*bufptr);
> +                    buf = *bufptr;
> +                    memset(buf, 0, page_size);
> +                } else {
> +                    return true;
> +                }
> +            }
> +
> +            memcpy(buf + addr % page_size, hbuf, n);
> +            addr += n;
> +            if (addr % page_size == 0) {
> +                /* we filled up the page */
> +                break;
> +            }
> +        } else {
> +            /* the next page is in the next block */
> +            *blockptr = block = QTAILQ_NEXT(block, next);
> +            if (!block) {
> +                break;
> +            }
> +
> +            addr = block->target_start;
> +            /* are we still in the same page? */
> +            if (dump_paddr_to_pfn(s, addr) != *pfnptr) {
> +                if (buf) {
> +                    /* no, but we already filled something earlier,
> return it */
> +                    break;
> +                } else {
> +                    /* else continue from there */
> +                    *pfnptr = dump_paddr_to_pfn(s, addr);
> +                }
> +            }
>          }
> -        addr = block->target_start;
> -        buf = block->host_addr;
>      }
>
> -    assert((block->target_start & ~target_page_mask) == 0);
> -    assert((block->target_end & ~target_page_mask) == 0);
> -    *pfnptr = dump_paddr_to_pfn(s, addr);
>      if (bufptr) {
>          *bufptr = buf;
>      }
>
> -    return true;
> +    return buf != NULL;
>  }
>
>  static void write_dump_bitmap(DumpState *s, Error **errp)
> @@ -1275,6 +1306,7 @@ static void write_dump_pages(DumpState *s, Error
> **errp)
>      uint8_t *buf;
>      GuestPhysBlock *block_iter = NULL;
>      uint64_t pfn_iter;
> +    g_autofree uint8_t *page = NULL;
>
>      /* get offset of page_desc and page_data in dump file */
>      offset_desc = s->offset_page;
> @@ -1310,12 +1342,13 @@ static void write_dump_pages(DumpState *s, Error
> **errp)
>      }
>
>      offset_data += s->dump_info.page_size;
> +    page = g_malloc(s->dump_info.page_size);
>
>      /*
>       * dump memory to vmcore page by page. zero page will all be resided
> in the
>       * first page of page section
>       */
> -    while (get_next_page(&block_iter, &pfn_iter, &buf, s)) {
> +    for (buf = page; get_next_page(&block_iter, &pfn_iter, &buf, s); buf
> = page) {
>          /* check zero page */
>          if (buffer_is_zero(buf, s->dump_info.page_size)) {
>              ret = write_cache(&page_desc, &pd_zero,
> sizeof(PageDescriptor),
> --
> 2.37.2
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 8021 bytes --]

Re: [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks

[David Hildenbrand]

On 05.09.22 14:57, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Rewrite get_next_page() to work over non-aligned blocks. When it
> encounters non aligned addresses, it will try to fill a page provided by
> the caller.
> 
> This solves a kdump crash with "tpm-crb-cmd" RAM memory region,
> qemu-kvm: ../dump/dump.c:1162: _Bool get_next_page(GuestPhysBlock **,
> uint64_t *, uint8_t **, DumpState *): Assertion `(block->target_start &
> ~target_page_mask) == 0' failed.
> 
> because:
> guest_phys_block_add_section: target_start=00000000fed40080 target_end=00000000fed41000: added (count: 4)
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=2120480
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   dump/dump.c | 79 +++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 56 insertions(+), 23 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index f465830371..500357bafe 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1094,50 +1094,81 @@ static uint64_t dump_pfn_to_paddr(DumpState *s, uint64_t pfn)
>   }
>   
>   /*
> - * exam every page and return the page frame number and the address of the page.
> - * bufptr can be NULL. note: the blocks here is supposed to reflect guest-phys
> - * blocks, so block->target_start and block->target_end should be interal
> - * multiples of the target page size.
> + * Return the page frame number and the page content in *bufptr. bufptr can be
> + * NULL. If not NULL, *bufptr must contains a target page size of pre-allocated
> + * memory. This is not necessarily the memory returned.
>    */
>   static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
>                             uint8_t **bufptr, DumpState *s)
>   {
>       GuestPhysBlock *block = *blockptr;
> -    hwaddr addr, target_page_mask = ~((hwaddr)s->dump_info.page_size - 1);
> -    uint8_t *buf;
> +    uint32_t page_size = s->dump_info.page_size;
> +    uint8_t *buf = NULL, *hbuf;
> +    hwaddr addr;
>   
>       /* block == NULL means the start of the iteration */
>       if (!block) {
>           block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>           *blockptr = block;
>           addr = block->target_start;
> +        *pfnptr = dump_paddr_to_pfn(s, addr);
>       } else {
> -        addr = dump_pfn_to_paddr(s, *pfnptr + 1);
> +        *pfnptr += 1;
> +        addr = dump_pfn_to_paddr(s, *pfnptr);
>       }
>       assert(block != NULL);
>   
> -    if ((addr >= block->target_start) &&
> -        (addr + s->dump_info.page_size <= block->target_end)) {
> -        buf = block->host_addr + (addr - block->target_start);
> -    } else {
> -        /* the next page is in the next block */
> -        block = QTAILQ_NEXT(block, next);
> -        *blockptr = block;
> -        if (!block) {
> -            return false;
> +    while (1) {
> +        if (addr >= block->target_start && addr < block->target_end) {
> +            size_t n = MIN(block->target_end - addr, page_size - addr % page_size);
> +            hbuf = block->host_addr + (addr - block->target_start);
> +            if (!buf) {
> +                if (n == page_size) {
> +                    /* this is a whole target page, go for it */
> +                    assert(addr % page_size == 0);
> +                    buf = hbuf;
> +                    break;
> +                } else if (bufptr) {
> +                    assert(*bufptr);
> +                    buf = *bufptr;
> +                    memset(buf, 0, page_size);
> +                } else {
> +                    return true;
> +                }
> +            }
> +
> +            memcpy(buf + addr % page_size, hbuf, n);
> +            addr += n;
> +            if (addr % page_size == 0) {
> +                /* we filled up the page */
> +                break;
> +            }
> +        } else {
> +            /* the next page is in the next block */
> +            *blockptr = block = QTAILQ_NEXT(block, next);
> +            if (!block) {
> +                break;
> +            }
> +
> +            addr = block->target_start;
> +            /* are we still in the same page? */
> +            if (dump_paddr_to_pfn(s, addr) != *pfnptr) {
> +                if (buf) {
> +                    /* no, but we already filled something earlier, return it */
> +                    break;
> +                } else {
> +                    /* else continue from there */
> +                    *pfnptr = dump_paddr_to_pfn(s, addr);
> +                }
> +            }
>           }

The loop is a bit confusing and the code is not that easy to follow.

... but I don't have a good idea to do it any better/cleaner. :)

So I assume as long as testing is good, this is fine

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb