Hi
On Wed, Jul 13, 2022 at 5:07 PM Janosch Frank <fran...@linux.ibm.com> wrote:
>
> The iteration over the memblocks is hard to understand so it's about
> time to clean it up.
>
> struct DumpState's next_block and start members can and should be
> local variables within the iterator.
>
> Instead of manually grabbing the next memblock we can use
> QTAILQ_FOREACH to iterate over all memblocks.
>
> The begin and length fields in the DumpState have been left untouched
> since the qmp arguments share their names.
>
> Signed-off-by: Janosch Frank <fran...@linux.ibm.com>
After this patch:
./qemu-system-x86_64 -monitor stdio -S
(qemu) dump-guest-memory foo
Error: dump: failed to save memory: Bad address
> ---
> dump/dump.c | 91 +++++++++++--------------------------------
> include/sysemu/dump.h | 47 +++++++++++++++++++---
> 2 files changed, 65 insertions(+), 73 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 4d9658ffa2..6feba3cbfa 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -591,56 +591,27 @@ static void dump_begin(DumpState *s, Error **errp)
> write_elf_notes(s, errp);
> }
>
> -static int get_next_block(DumpState *s, GuestPhysBlock *block)
> -{
> - while (1) {
> - block = QTAILQ_NEXT(block, next);
> - if (!block) {
> - /* no more block */
> - return 1;
> - }
> -
> - s->start = 0;
> - s->next_block = block;
> - if (s->has_filter) {
> - if (block->target_start >= s->begin + s->length ||
> - block->target_end <= s->begin) {
> - /* This block is out of the range */
> - continue;
> - }
> -
> - if (s->begin > block->target_start) {
> - s->start = s->begin - block->target_start;
> - }
> - }
> -
> - return 0;
> - }
> -}
> -
> /* write all memory to vmcore */
> static void dump_iterate(DumpState *s, Error **errp)
> {
> ERRP_GUARD();
> GuestPhysBlock *block;
> - int64_t size;
> + int64_t memblock_size, memblock_start;
>
> - do {
> - block = s->next_block;
> -
> - size = block->target_end - block->target_start;
> - if (s->has_filter) {
> - size -= s->start;
> - if (s->begin + s->length < block->target_end) {
> - size -= block->target_end - (s->begin + s->length);
> - }
> + QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> + memblock_start = dump_get_memblock_start(block, s->begin, s->length);
> + if (memblock_start == -1) {
> + continue;
> }
> - write_memory(s, block, s->start, size, errp);
> +
> + memblock_size = dump_get_memblock_size(block, s->begin, s->length);
> +
> + /* Write the memory to file */
> + write_memory(s, block, memblock_start, memblock_size, errp);
> if (*errp) {
> return;
> }
> -
> - } while (!get_next_block(s, block));
> + }
> }
>
> static void create_vmcore(DumpState *s, Error **errp)
> @@ -1490,30 +1461,22 @@ static void create_kdump_vmcore(DumpState *s, Error
> **errp)
> }
> }
>
> -static ram_addr_t get_start_block(DumpState *s)
> +static int validate_start_block(DumpState *s)
> {
> GuestPhysBlock *block;
>
> if (!s->has_filter) {
> - s->next_block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
> return 0;
> }
>
> QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> + /* This block is out of the range */
> if (block->target_start >= s->begin + s->length ||
> block->target_end <= s->begin) {
> - /* This block is out of the range */
> continue;
> }
> -
> - s->next_block = block;
> - if (s->begin > block->target_start) {
> - s->start = s->begin - block->target_start;
> - } else {
> - s->start = 0;
> - }
> - return s->start;
> - }
> + return 0;
> + }
>
> return -1;
> }
> @@ -1540,25 +1503,17 @@ bool qemu_system_dump_in_progress(void)
> return (qatomic_read(&state->status) == DUMP_STATUS_ACTIVE);
> }
>
> -/* calculate total size of memory to be dumped (taking filter into
> - * acoount.) */
> +/*
> + * calculate total size of memory to be dumped (taking filter into
> + * account.)
thanks for fixing the typo
> + */
> static int64_t dump_calculate_size(DumpState *s)
> {
> GuestPhysBlock *block;
> - int64_t size = 0, total = 0, left = 0, right = 0;
> + int64_t total = 0;
>
> QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> - if (s->has_filter) {
> - /* calculate the overlapped region. */
> - left = MAX(s->begin, block->target_start);
> - right = MIN(s->begin + s->length, block->target_end);
> - size = right - left;
> - size = size > 0 ? size : 0;
> - } else {
> - /* count the whole region in */
> - size = (block->target_end - block->target_start);
> - }
> - total += size;
> + total += dump_get_memblock_size(block, s->begin, s->length);
> }
>
> return total;
> @@ -1660,8 +1615,8 @@ static void dump_init(DumpState *s, int fd, bool
> has_format,
> goto cleanup;
> }
>
> - s->start = get_start_block(s);
> - if (s->start == -1) {
> + /* Is the filter filtering everything? */
> + if (validate_start_block(s) == -1) {
> error_setg(errp, QERR_INVALID_PARAMETER, "begin");
> goto cleanup;
> }
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index ffc2ea1072..f3bf98c220 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -166,11 +166,10 @@ typedef struct DumpState {
> hwaddr memory_offset;
> int fd;
>
> - GuestPhysBlock *next_block;
> - ram_addr_t start;
> - bool has_filter;
> - int64_t begin;
> - int64_t length;
> + /* Guest memory related data */
> + bool has_filter; /* Are we dumping parts of the memory? */
> + int64_t begin; /* Start address of the chunk we want to dump
> */
> + int64_t length; /* Length of the dump we want to dump */
>
> uint8_t *note_buf; /* buffer for notes */
> size_t note_buf_offset; /* the writing place in note_buf */
> @@ -203,4 +202,42 @@ typedef struct DumpState {
> uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
> uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
> +
> +static inline int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t
> filter_area_start,
> + int64_t filter_area_length)
> +{
> + int64_t size, left, right;
> +
> + /* No filter, return full size */
> + if (!filter_area_length) {
> + return block->target_end - block->target_start;
> + }
> +
> + /* calculate the overlapped region. */
> + left = MAX(filter_area_start, block->target_start);
> + right = MIN(filter_area_start + filter_area_length, block->target_end);
> + size = right - left;
> + size = size > 0 ? size : 0;
> +
> + return size;
> +}
> +
> +static inline int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t
> filter_area_start,
> + int64_t filter_area_length)
> +{
> + if (filter_area_length) {
> + /*
> + * Check if block is within guest memory dump area. If not
> + * go to next one.
> + */
Or rather "return -1 if the block is not within filter area"
> + if (block->target_start >= filter_area_start + filter_area_length ||
> + block->target_end <= filter_area_start) {
> + return -1;
> + }
> + if (filter_area_start > block->target_start) {
> + return filter_area_start - block->target_start;
> + }
> + }
> + return block->target_start;
This used to be 0. Changing that, I think the patch looks good.
Although it could perhaps be splitted to introduce the two functions.
> +}
> #endif
> --
> 2.34.1
>
