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

[Peter Maydell <peter.maydell@linaro.org>]

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