* [PATCH v2 0/2] Fix dumping in kdump format with non-aligned memory
@ 2022-09-05 12:57 marcandre.lureau
2022-09-05 12:57 ` [PATCH v2 1/2] dump: simplify a bit kdump get_next_page() marcandre.lureau
2022-09-05 12:57 ` [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks marcandre.lureau
0 siblings, 2 replies; 9+ messages in thread
From: marcandre.lureau @ 2022-09-05 12:57 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, qiaonuohan, Peter Maydell, Stefan Berger,
David Hildenbrand, Marc-André 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] dump: simplify a bit kdump get_next_page()
2022-09-05 12:57 [PATCH v2 0/2] Fix dumping in kdump format with non-aligned memory marcandre.lureau
@ 2022-09-05 12:57 ` marcandre.lureau
2022-09-05 13:12 ` David Hildenbrand
2022-09-05 13:27 ` Peter Maydell
2022-09-05 12:57 ` [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks marcandre.lureau
1 sibling, 2 replies; 9+ messages in thread
From: marcandre.lureau @ 2022-09-05 12:57 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, qiaonuohan, Peter Maydell, Stefan Berger,
David Hildenbrand, Marc-André 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks
2022-09-05 12:57 [PATCH v2 0/2] Fix dumping in kdump format with non-aligned memory marcandre.lureau
2022-09-05 12:57 ` [PATCH v2 1/2] dump: simplify a bit kdump get_next_page() marcandre.lureau
@ 2022-09-05 12:57 ` marcandre.lureau
2022-09-06 20:11 ` Stefan Berger
` (2 more replies)
1 sibling, 3 replies; 9+ messages in thread
From: marcandre.lureau @ 2022-09-05 12:57 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, qiaonuohan, Peter Maydell, Stefan Berger,
David Hildenbrand, Marc-André 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] dump: simplify a bit kdump get_next_page()
2022-09-05 12:57 ` [PATCH v2 1/2] dump: simplify a bit kdump get_next_page() marcandre.lureau
@ 2022-09-05 13:12 ` David Hildenbrand
2022-09-05 13:27 ` Peter Maydell
1 sibling, 0 replies; 9+ messages in thread
From: David Hildenbrand @ 2022-09-05 13:12 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Paolo Bonzini, qiaonuohan, Peter Maydell, Stefan Berger
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] dump: simplify a bit kdump get_next_page()
2022-09-05 12:57 ` [PATCH v2 1/2] dump: simplify a bit kdump get_next_page() marcandre.lureau
2022-09-05 13:12 ` David Hildenbrand
@ 2022-09-05 13:27 ` Peter Maydell
2022-09-05 13:33 ` Marc-André Lureau
1 sibling, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2022-09-05 13:27 UTC (permalink / raw)
To: marcandre.lureau
Cc: qemu-devel, Paolo Bonzini, qiaonuohan, Stefan Berger, David Hildenbrand
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] dump: simplify a bit kdump get_next_page()
2022-09-05 13:27 ` Peter Maydell
@ 2022-09-05 13:33 ` Marc-André Lureau
0 siblings, 0 replies; 9+ messages in thread
From: Marc-André Lureau @ 2022-09-05 13:33 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Paolo Bonzini, qiaonuohan, Stefan Berger, David Hildenbrand
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks
2022-09-05 12:57 ` [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks marcandre.lureau
@ 2022-09-06 20:11 ` Stefan Berger
2022-09-29 7:54 ` Marc-André Lureau
2022-10-06 15:05 ` David Hildenbrand
2 siblings, 0 replies; 9+ messages in thread
From: Stefan Berger @ 2022-09-06 20:11 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Paolo Bonzini, qiaonuohan, Peter Maydell, Stefan Berger,
David Hildenbrand
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...
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks
2022-09-05 12:57 ` [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks marcandre.lureau
2022-09-06 20:11 ` Stefan Berger
@ 2022-09-29 7:54 ` Marc-André Lureau
2022-10-06 15:05 ` David Hildenbrand
2 siblings, 0 replies; 9+ messages in thread
From: Marc-André Lureau @ 2022-09-29 7:54 UTC (permalink / raw)
To: qemu-devel, David Hildenbrand
Cc: Paolo Bonzini, qiaonuohan, Peter Maydell, Stefan Berger
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks
2022-09-05 12:57 ` [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks marcandre.lureau
2022-09-06 20:11 ` Stefan Berger
2022-09-29 7:54 ` Marc-André Lureau
@ 2022-10-06 15:05 ` David Hildenbrand
2 siblings, 0 replies; 9+ messages in thread
From: David Hildenbrand @ 2022-10-06 15:05 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel
Cc: Paolo Bonzini, qiaonuohan, Peter Maydell, Stefan Berger
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-10-06 15:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-05 12:57 [PATCH v2 0/2] Fix dumping in kdump format with non-aligned memory marcandre.lureau
2022-09-05 12:57 ` [PATCH v2 1/2] dump: simplify a bit kdump get_next_page() marcandre.lureau
2022-09-05 13:12 ` David Hildenbrand
2022-09-05 13:27 ` Peter Maydell
2022-09-05 13:33 ` Marc-André Lureau
2022-09-05 12:57 ` [PATCH v2 2/2] dump: fix kdump to work over non-aligned blocks marcandre.lureau
2022-09-06 20:11 ` Stefan Berger
2022-09-29 7:54 ` Marc-André Lureau
2022-10-06 15:05 ` David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.