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
>