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

[David Hildenbrand]

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()"

> 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."

> 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)
--
Cheers,

David / dhildenb