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

[PATCH 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 returning allocated and filled up pages as necessary.

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

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

 dump/dump.c | 103 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 68 insertions(+), 35 deletions(-)

-- 
2.37.2

[PATCH 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 | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 4d9658ffa2..18f06cffe2 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1107,37 +1107,31 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
     uint8_t *buf;
 
     /* block == NULL means the start of the iteration */
-    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;
+    if (block == NULL) {
+        *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
+        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)) {
         buf = block->host_addr + (addr - block->target_start);
     } else {
         /* the next page is in the next block */
-        block = QTAILQ_NEXT(block, next);
-        *blockptr = block;
+        *blockptr = block = QTAILQ_NEXT(block, next);
         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;
     }
 
+    /* those checks are going away next */
+    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;
     }
-- 
2.37.2

[PATCH 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 allocate a zero-page and try
to fill it.

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 | 89 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 25 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 18f06cffe2..9aeed88688 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1094,49 +1094,79 @@ 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 and allocptr can be NULL. If alloced, *bufptr must be freed.
  */
 static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
-                          uint8_t **bufptr, DumpState *s)
+                          uint8_t **bufptr, bool *allocptr, 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;
+    bool alloced = false;
+    uint8_t *buf = NULL, *hbuf;
+    hwaddr addr;
 
     /* block == NULL means the start of the iteration */
     if (block == NULL) {
         *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
         addr = block->target_start;
+        *pfnptr = dump_paddr_to_pfn(s, addr);
     } else {
-        addr = dump_pfn_to_paddr(s, *pfnptr + 1);
+        assert(block != NULL);
+        *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 */
-        *blockptr = block = QTAILQ_NEXT(block, next);
-        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 (!alloced) {
+                if (n == page_size) {
+                    /* this is a whole host page, go for it */
+                    assert(addr % page_size == 0);
+                    buf = hbuf;
+                    break;
+                } else {
+                    buf = g_malloc0(page_size);
+                    alloced = true;
+                }
+            }
+
+            memcpy(buf + addr % page_size, hbuf, n);
+            addr += n;
+            if (addr % page_size == 0) {
+                /* we filled up the alloc 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 (alloced) {
+                    /* 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;
     }
 
-    /* those checks are going away next */
-    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;
+        *allocptr = alloced;
     }
 
-    return true;
+    return buf != NULL;
 }
 
 static void write_dump_bitmap(DumpState *s, Error **errp)
@@ -1159,7 +1189,7 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
      * exam memory page by page, and set the bit in dump_bitmap corresponded
      * to the existing page.
      */
-    while (get_next_page(&block_iter, &pfn, NULL, s)) {
+    while (get_next_page(&block_iter, &pfn, NULL, NULL, s)) {
         ret = set_dump_bitmap(last_pfn, pfn, true, dump_bitmap_buf, s);
         if (ret < 0) {
             error_setg(errp, "dump: failed to set dump_bitmap");
@@ -1274,6 +1304,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
     uint8_t *buf;
     GuestPhysBlock *block_iter = NULL;
     uint64_t pfn_iter;
+    bool freebuf = false;
 
     /* get offset of page_desc and page_data in dump file */
     offset_desc = s->offset_page;
@@ -1314,7 +1345,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
      * 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)) {
+    while (get_next_page(&block_iter, &pfn_iter, &buf, &freebuf, s)) {
         /* check zero page */
         if (buffer_is_zero(buf, s->dump_info.page_size)) {
             ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
@@ -1403,6 +1434,10 @@ static void write_dump_pages(DumpState *s, Error **errp)
                 error_setg(errp, "dump: failed to write page desc");
                 goto out;
             }
+            if (freebuf) {
+                g_free(buf);
+                freebuf = false;
+            }
         }
         s->written_size += s->dump_info.page_size;
     }
@@ -1419,6 +1454,10 @@ static void write_dump_pages(DumpState *s, Error **errp)
     }
 
 out:
+    if (freebuf) {
+        g_free(buf);
+    }
+
     free_data_cache(&page_desc);
     free_data_cache(&page_data);
 
-- 
2.37.2

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

[Peter Maydell]

On Thu, 25 Aug 2022 at 14:21, <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 allocate a zero-page and try
> to fill it.
>
> 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


>  static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
> -                          uint8_t **bufptr, DumpState *s)
> +                          uint8_t **bufptr, bool *allocptr, DumpState *s)
>  {
>      GuestPhysBlock *block = *blockptr;
> -    hwaddr addr, target_page_mask = ~((hwaddr)s->dump_info.page_size - 1);

In the old code, we treated the dump_info.page size as an indication
of the target's page size...

> -    uint8_t *buf;
> +    uint32_t page_size = s->dump_info.page_size;
> +    bool alloced = false;
> +    uint8_t *buf = NULL, *hbuf;
> +    hwaddr addr;
>
>      /* block == NULL means the start of the iteration */
>      if (block == NULL) {
>          *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>          addr = block->target_start;
> +        *pfnptr = dump_paddr_to_pfn(s, addr);
>      } else {
> -        addr = dump_pfn_to_paddr(s, *pfnptr + 1);
> +        assert(block != NULL);
> +        *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 */
> -        *blockptr = block = QTAILQ_NEXT(block, next);
> -        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 (!alloced) {
> +                if (n == page_size) {
> +                    /* this is a whole host page, go for it */
> +                    assert(addr % page_size == 0);

...but here we're claiming in this comment that it is the host's
page size.

Which is it ?

thanks
-- PMM

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

[Marc-André Lureau]

Hi

On Thu, Aug 25, 2022 at 5:35 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 25 Aug 2022 at 14:21, <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 allocate a zero-page and try
> > to fill it.
> >
> > 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
>
>
> >  static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
> > -                          uint8_t **bufptr, DumpState *s)
> > +                          uint8_t **bufptr, bool *allocptr, DumpState *s)
> >  {
> >      GuestPhysBlock *block = *blockptr;
> > -    hwaddr addr, target_page_mask = ~((hwaddr)s->dump_info.page_size - 1);
>
> In the old code, we treated the dump_info.page size as an indication
> of the target's page size...
>
> > -    uint8_t *buf;
> > +    uint32_t page_size = s->dump_info.page_size;
> > +    bool alloced = false;
> > +    uint8_t *buf = NULL, *hbuf;
> > +    hwaddr addr;
> >
> >      /* block == NULL means the start of the iteration */
> >      if (block == NULL) {
> >          *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
> >          addr = block->target_start;
> > +        *pfnptr = dump_paddr_to_pfn(s, addr);
> >      } else {
> > -        addr = dump_pfn_to_paddr(s, *pfnptr + 1);
> > +        assert(block != NULL);
> > +        *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 */
> > -        *blockptr = block = QTAILQ_NEXT(block, next);
> > -        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 (!alloced) {
> > +                if (n == page_size) {
> > +                    /* this is a whole host page, go for it */
> > +                    assert(addr % page_size == 0);
>
> ...but here we're claiming in this comment that it is the host's
> page size.

+    uint32_t page_size = s->dump_info.page_size;

bad comment, will be fixed, thanks

>
> Which is it ?
>
> thanks
> -- PMM
>

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

[David Hildenbrand]

On 25.08.22 15:21, 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 | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 4d9658ffa2..18f06cffe2 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1107,37 +1107,31 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
>      uint8_t *buf;
>  
>      /* block == NULL means the start of the iteration */
> -    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;


Instead of the "return true" we'll now do take the  "if ((addr >=
block->target_start) &&" path below I guess, always ending up with
essentially "buf = buf;" because addr == block->target_start.

I guess that's fine.

> +    if (block == NULL) {

What's wrong with keeping the "if (!block) {" ? :)

> +        *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);

Another unnecessary change.

> +        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)) {
>          buf = block->host_addr + (addr - block->target_start);
>      } else {
>          /* the next page is in the next block */
> -        block = QTAILQ_NEXT(block, next);
> -        *blockptr = block;
> +        *blockptr = block = QTAILQ_NEXT(block, next);

Another unnecessary change. (avoiding these really eases review, because
the focus is then completely on the actual code changes)

>          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;
>      }
>  
> +    /* those checks are going away next */

This comment seems to imply a story documented in code. Rather just drop
it -- the patch description already points that out.

> +    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;
>      }


Apart from the nits, LGTM.

-- 
Thanks,

David / dhildenb

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

[Marc-André Lureau]

Hi

On Fri, Aug 26, 2022 at 1:45 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 25.08.22 15:21, 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 | 30 ++++++++++++------------------
> >  1 file changed, 12 insertions(+), 18 deletions(-)
> >
> > diff --git a/dump/dump.c b/dump/dump.c
> > index 4d9658ffa2..18f06cffe2 100644
> > --- a/dump/dump.c
> > +++ b/dump/dump.c
> > @@ -1107,37 +1107,31 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
> >      uint8_t *buf;
> >
> >      /* block == NULL means the start of the iteration */
> > -    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;
>
>
> Instead of the "return true" we'll now do take the  "if ((addr >=
> block->target_start) &&" path below I guess, always ending up with
> essentially "buf = buf;" because addr == block->target_start.
>
> I guess that's fine.
>
> > +    if (block == NULL) {
>
> What's wrong with keeping the "if (!block) {" ? :)

That's just to be consistent with the comment above.

>
> > +        *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>
> Another unnecessary change.
>
> > +        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)) {
> >          buf = block->host_addr + (addr - block->target_start);
> >      } else {
> >          /* the next page is in the next block */
> > -        block = QTAILQ_NEXT(block, next);
> > -        *blockptr = block;
> > +        *blockptr = block = QTAILQ_NEXT(block, next);
>
> Another unnecessary change. (avoiding these really eases review, because
> the focus is then completely on the actual code changes)
>
> >          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;
> >      }
> >
> > +    /* those checks are going away next */
>
> This comment seems to imply a story documented in code. Rather just drop
> it -- the patch description already points that out.
>
> > +    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;
> >      }
>
>
> Apart from the nits, LGTM.

We could also drop this patch, it helped me to rewrite the function next mostly.

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

[David Hildenbrand]

On 25.08.22 15:21, 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 allocate a zero-page and try
> to fill it.

Could we simplify by using one global helper page (or caller provided
page) and avoiding the allocation/freeing?


-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 26.08.22 11:56, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Aug 26, 2022 at 1:45 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 25.08.22 15:21, 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 | 30 ++++++++++++------------------
>>>  1 file changed, 12 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/dump/dump.c b/dump/dump.c
>>> index 4d9658ffa2..18f06cffe2 100644
>>> --- a/dump/dump.c
>>> +++ b/dump/dump.c
>>> @@ -1107,37 +1107,31 @@ static bool get_next_page(GuestPhysBlock **blockptr, uint64_t *pfnptr,
>>>      uint8_t *buf;
>>>
>>>      /* block == NULL means the start of the iteration */
>>> -    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;
>>
>>
>> Instead of the "return true" we'll now do take the  "if ((addr >=
>> block->target_start) &&" path below I guess, always ending up with
>> essentially "buf = buf;" because addr == block->target_start.
>>
>> I guess that's fine.
>>
>>> +    if (block == NULL) {
>>
>> What's wrong with keeping the "if (!block) {" ? :)
> 
> That's just to be consistent with the comment above.
> 
>>
>>> +        *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>>
>> Another unnecessary change.
>>
>>> +        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)) {
>>>          buf = block->host_addr + (addr - block->target_start);
>>>      } else {
>>>          /* the next page is in the next block */
>>> -        block = QTAILQ_NEXT(block, next);
>>> -        *blockptr = block;
>>> +        *blockptr = block = QTAILQ_NEXT(block, next);
>>
>> Another unnecessary change. (avoiding these really eases review, because
>> the focus is then completely on the actual code changes)
>>
>>>          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;
>>>      }
>>>
>>> +    /* those checks are going away next */
>>
>> This comment seems to imply a story documented in code. Rather just drop
>> it -- the patch description already points that out.
>>
>>> +    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;
>>>      }
>>
>>
>> Apart from the nits, LGTM.
> 
> We could also drop this patch, it helped me to rewrite the function next mostly.

I think it's fine. Small logical changes are easier to review -- at
least for me.

-- 
Thanks,

David / dhildenb

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

[Marc-André Lureau]

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

Hi


On Fri, Aug 26, 2022 at 2:01 PM David Hildenbrand <david@redhat.com> wrote:

> On 25.08.22 15:21, 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 allocate a zero-page and try
> > to fill it.
>
> Could we simplify by using one global helper page (or caller provided
> page) and avoiding the allocation/freeing?
>
>
I don't think that makes a big difference, but certainly doable.


-- 
Marc-André Lureau

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

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

[David Hildenbrand]

On 26.08.22 12:02, Marc-André Lureau wrote:
> Hi
> 
> 
> On Fri, Aug 26, 2022 at 2:01 PM David Hildenbrand <david@redhat.com
> <mailto:david@redhat.com>> wrote:
> 
>     On 25.08.22 15:21, marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com> wrote:
>     > From: Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>
>     >
>     > Rewrite get_next_page() to work over non-aligned blocks. When it
>     > encounters non aligned addresses, it will allocate a zero-page and try
>     > to fill it.
> 
>     Could we simplify by using one global helper page (or caller provided
>     page) and avoiding the allocation/freeing?
> 
> 
> I don't think that makes a big difference, but certainly doable.

If we're using one central page, I guess we'd have to pass "flag_sync =
true" to write_cache() in case that page is used. Or we simply specify
on the single global page in there and force a sync. Changes would be
limited to get_next_page() and write_cache() then.


-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 26.08.22 12:07, David Hildenbrand wrote:
> On 26.08.22 12:02, Marc-André Lureau wrote:
>> Hi
>>
>>
>> On Fri, Aug 26, 2022 at 2:01 PM David Hildenbrand <david@redhat.com
>> <mailto:david@redhat.com>> wrote:
>>
>>     On 25.08.22 15:21, marcandre.lureau@redhat.com
>>     <mailto:marcandre.lureau@redhat.com> wrote:
>>     > From: Marc-André Lureau <marcandre.lureau@redhat.com
>>     <mailto:marcandre.lureau@redhat.com>>
>>     >
>>     > Rewrite get_next_page() to work over non-aligned blocks. When it
>>     > encounters non aligned addresses, it will allocate a zero-page and try
>>     > to fill it.
>>
>>     Could we simplify by using one global helper page (or caller provided
>>     page) and avoiding the allocation/freeing?
>>
>>
>> I don't think that makes a big difference, but certainly doable.
> 
> If we're using one central page, I guess we'd have to pass "flag_sync =
> true" to write_cache() in case that page is used. Or we simply specify
> on the single global page in there and force a sync. Changes would be
> limited to get_next_page() and write_cache() then.

I might be wrong. I think we might not have to touch write_cache() at
all -- it will copy the data into the DataCache buffer.


-- 
Thanks,

David / dhildenb