Re: [PATCH v4 1/7] hwpoison_page_list and qemu_ram_remap are based on pages

[William Roche]

On 1/8/25 22:34, David Hildenbrand wrote:
> On 14.12.24 14:45, “William Roche wrote:
> > From: William Roche <william.roche@oracle.com>
>
> Subject should likely start with "system/physmem:".
>
> Maybe
>
> "system/physmem: handle hugetlb correctly in qemu_ram_remap()"

I updated the commit title

> > The list of hwpoison pages used to remap the memory on reset
> > is based on the backend real page size. When dealing with
> > hugepages, we create a single entry for the entire page.
>
> Maybe add something like:
>
> "To correctly handle hugetlb, we must mmap(MAP_FIXED) a complete hugetlb 
> page; hugetlb pages cannot be partially mapped."

Updated into the commit message

> > Co-developed-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: William Roche <william.roche@oracle.com>
> > ---
> >   accel/kvm/kvm-all.c       |  6 +++++-
> >   include/exec/cpu-common.h |  3 ++-
> >   system/physmem.c          | 32 ++++++++++++++++++++++++++------
> >   3 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 801cff16a5..24c0c4ce3f 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -1278,7 +1278,7 @@ static void kvm_unpoison_all(void *param)
> >       QLIST_FOREACH_SAFE(page, &hwpoison_page_list, list, next_page) {
> >           QLIST_REMOVE(page, list);
> > -        qemu_ram_remap(page->ram_addr, TARGET_PAGE_SIZE);
> > +        qemu_ram_remap(page->ram_addr);
> >           g_free(page);
> >       }
> >   }
> > @@ -1286,6 +1286,10 @@ static void kvm_unpoison_all(void *param)
> >   void kvm_hwpoison_page_add(ram_addr_t ram_addr)
> >   {
> >       HWPoisonPage *page;
> > +    size_t page_size = qemu_ram_pagesize_from_addr(ram_addr);
> > +
> > +    if (page_size > TARGET_PAGE_SIZE)
> > +        ram_addr = QEMU_ALIGN_DOWN(ram_addr, page_size);
>
> Is that part still required? I thought it would be sufficient (at least 
> in the context of this patch) to handle it all in qemu_ram_remap().
>
> qemu_ram_remap() will calculate the range to process based on the 
> RAMBlock page size. IOW, the QEMU_ALIGN_DOWN() we do now in 
> qemu_ram_remap().
>
> Or am I missing something?
>
> (sorry if we discussed that already; if there is a good reason it might 
> make sense to state it in the patch description)

You are right, but at this patch level we still need to round up the 
address and doing it here is small enough.
Of course, the code changes on patch 3/7 where we change both x86 and 
ARM versions of the code to align the memory pointer correctly in both 
cases.