Re: [PATCH v4 2/7] system/physmem: poisoned memory discard on reboot

[David Hildenbrand]

On 14.12.24 14:45, “William Roche wrote:
> From: William Roche <william.roche@oracle.com>
>
> Repair poisoned memory location(s), calling ram_block_discard_range():
> punching a hole in the backend file when necessary and regenerating
> a usable memory.
> If the kernel doesn't support the madvise calls used by this function
> and we are dealing with anonymous memory, fall back to remapping the
> location(s).
>
> Signed-off-by: William Roche <william.roche@oracle.com>
> ---
>   system/physmem.c | 63 ++++++++++++++++++++++++++++++++----------------
>   1 file changed, 42 insertions(+), 21 deletions(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 2c90cc2d78..b228a692f8 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2180,13 +2180,37 @@ void qemu_ram_free(RAMBlock *block)
>   }
>   
>   #ifndef _WIN32
> +/* Try to simply remap the given location */
> +static void qemu_ram_remap_mmap(RAMBlock *block, void* vaddr, size_t size,
> +                                ram_addr_t offset)

Can you make the parameters match the ones of ram_block_discard_range() 
so the invocation gets easier to read? You can calculate vaddr easily 
internally.

Something like

static void qemu_ram_remap_mmap(RAMBlock *rb, uint64_t start,
                                size_t length)

> +{
> +    int flags, prot;
> +    void *area;
> +
> +    flags = MAP_FIXED;
> +    flags |= block->flags & RAM_SHARED ? MAP_SHARED : MAP_PRIVATE;
> +    flags |= block->flags & RAM_NORESERVE ? MAP_NORESERVE : 0;
> +    prot = PROT_READ;
> +    prot |= block->flags & RAM_READONLY ? 0 : PROT_WRITE;
> +    if (block->fd >= 0) {

Heh, that case can no longer happen!

assert(block->fs < 0);

?

> +        area = mmap(vaddr, size, prot, flags, block->fd,
> +                    offset + block->fd_offset);
> +    } else {
> +        flags |= MAP_ANONYMOUS;
> +        area = mmap(vaddr, size, prot, flags, -1, 0);
> +    }
> +    if (area != vaddr) {
> +        error_report("Could not remap addr: " RAM_ADDR_FMT "@" RAM_ADDR_FMT "",
> +                     size, block->offset + offset);
> +        exit(1);
> +    }
> +}
> +
>   void qemu_ram_remap(ram_addr_t addr)
>   {
>       RAMBlock *block;
>       ram_addr_t offset;
> -    int flags;
> -    void *area, *vaddr;
> -    int prot;
> +    void *vaddr;
>       size_t page_size;
>   
>       RAMBLOCK_FOREACH(block) {
> @@ -2202,24 +2226,21 @@ void qemu_ram_remap(ram_addr_t addr)
>               } else if (xen_enabled()) {
>                   abort();
>               } else {
> -                flags = MAP_FIXED;
> -                flags |= block->flags & RAM_SHARED ?
> -                         MAP_SHARED : MAP_PRIVATE;
> -                flags |= block->flags & RAM_NORESERVE ? MAP_NORESERVE : 0;
> -                prot = PROT_READ;
> -                prot |= block->flags & RAM_READONLY ? 0 : PROT_WRITE;
> -                if (block->fd >= 0) {
> -                    area = mmap(vaddr, page_size, prot, flags, block->fd,
> -                                offset + block->fd_offset);
> -                } else {
> -                    flags |= MAP_ANONYMOUS;
> -                    area = mmap(vaddr, page_size, prot, flags, -1, 0);
> -                }
> -                if (area != vaddr) {
> -                    error_report("Could not remap addr: "
> -                                 RAM_ADDR_FMT "@" RAM_ADDR_FMT "",
> -                                 page_size, addr);
> -                    exit(1);
> +                if (ram_block_discard_range(block, offset + block->fd_offset,
> +                                            page_size) != 0) {
> +                    /*
> +                     * Fold back to using mmap() only for anonymous mapping,

s/Fold/Fall/

> +                     * as if a backing file is associated we may not be able
> +                     * to recover the memory in all cases.
> +                     * So don't take the risk of using only mmap and fail now.
> +                     */
> +                    if (block->fd >= 0) {
> +                        error_report("Memory poison recovery failure addr: "
> +                                     RAM_ADDR_FMT "@" RAM_ADDR_FMT "",
> +                                     page_size, addr);
> +                        exit(1);
> +                    }
> +                    qemu_ram_remap_mmap(block, vaddr, page_size, offset);
>                   }
>                   memory_try_enable_merging(vaddr, page_size);
>                   qemu_ram_setup_dump(vaddr, page_size);

These two can be moved into qemu_ram_remap_mmap(). They are not required 
if we didn't actually mess with mmap().

--
Cheers,

David / dhildenb