Re: [PATCH QEMU HCI-8.1 18/21] VZ: physmem: align fd-backed RAM like anonymous RAM #VSTOR-103860

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] physmem: file_ram_alloc() raises shared mr->align to 2M for all fd RAM, with no compat
- [Medium] physmem: 2M alignment for fd-backed RAM shrinks NVDIMM PMEM size
- [Medium] physmem: 2M align forced on all fd-backed RAM, incl. non-shmem files and offsets
--

commit 06425618e51a0375322bf7c00e7205bad272313c
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: physmem: align fd-backed RAM like anonymous RAM #VSTOR-103860

This commit raises the alignment of fd-backed RAM blocks in
file_ram_alloc() to QEMU_VMALLOC_ALIGN on all hosts, instead of only on
s390x KVM. The goal is to make memfd and tmpfs backed guest RAM eligible
for 2M transparent huge pages.

> diff --git a/system/physmem.c b/system/physmem.c
> index 6f89232d257f..ae5aa534c60a 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1476,11 +1476,14 @@ static void *file_ram_alloc(RAMBlock *block,
>          return NULL;
>      }
>      block->mr->align = MAX(block->page_size, block->mr->align);
> -#if defined(__s390x__)
> -    if (kvm_enabled()) {
> -        block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);
> -    }
> -#endif
> +
> +    /*
> +     * Align like anonymous RAM (qemu_anon_ram_alloc), so that shmem-backed
> +     * RAM (memfd, tmpfs files) is eligible for THP.
> +     *
> +     * On s390x KVM this alignment is required.
> +     */
> +    block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);

[Severity: Medium]
Can this unconditional bump of block->mr->align reject configurations
that were accepted before?  mr->align is the same field returned by
memory_region_get_alignment(), so it is not only an mmap alignment.

memory_device_pre_plug() checks it against the backend size:

hw/mem/memory-device.c:memory_device_pre_plug() {
    ...
    if (!QEMU_IS_ALIGNED(memory_region_size(mr),
                         memory_region_get_alignment(mr))) {
        error_setg(errp, "backend memory size must be multiple of 0x%"
    ...
}

A memory-backend-file or memfd backend (not hugetlbfs) used with pc-dimm,
virtio-mem, nvdimm or hv-balloon, whose size is a 4K multiple but not a
2M multiple, was accepted before.  Would it now fail at plug time with
"backend memory size must be multiple of 0x200000"?

The same function also does:

    align = MAX(align, memory_region_get_alignment(mr));

and passes it to memory_device_get_free_addr().  Can this change the
auto-assigned guest-physical address of file-backed DIMMs once earlier
devices have odd sizes, or trigger the "memory fragmented due to
alignments" error?

virtio_mem_device_realize() (vmem->mr->align =
memory_region_get_alignment(&vmem->memdev->mr)) and hv-balloon inherit
the larger alignment as well.

No compat property or machine-type gating is added, and the commit
message does not mention these side effects.  Is that intended?

[Severity: Medium]
Can this change shrink the guest-visible NVDIMM size, or make realize
fail, for file-backed NVDIMM backends?

hw/mem/nvdimm.c:nvdimm_prepare_memory_region() {
    ...
    align = memory_region_get_alignment(mr);
    ...
    pmem_size = QEMU_ALIGN_DOWN(pmem_size, align);

    if (size <= nvdimm->label_size || !pmem_size) {
    ...
    nvdimm->nvdimm_mr->align = align;
}

With align now 2M instead of 4K, a 1G backend with a 128K label would
expose 1G-2M rather than 1G-128K.  A backend smaller than label plus 2M
would hit the "too small to contain nvdimm label and aligned PMEM" error.

The 2M alignment is also carried into nvdimm_mr->align and so into later
memory device placement.  Could this break migration or compatibility
with guests expecting the old size?  The commit message only discusses
memfd and THP and does not address NVDIMM sizing.

[Severity: Medium]
The commit message and comment justify this change for shmem-backed RAM
(memfd, tmpfs), but the bump applies to every fd, including regular
files, DAX/pmem files, and memory-backend-file with an offset.  Should it
be limited to the shmem case?

Non-shmem fds gain nothing from the larger alignment, yet
qemu_ram_mmap() now reserves size + align of address space for them:

util/mmap-alloc.c:qemu_ram_mmap() {
    total = size + align;
    ...
    offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr;
    ptr = mmap_activate(guardptr + offset, size, fd, qemu_map_flags,
                        map_offset);
    ...
}

Also, only the virtual address is aligned here.  The file offset is
passed unchanged as map_offset, and file_ram_alloc() only requires the
offset to be a multiple of page_size:

    area = qemu_ram_mmap(fd, memory, block->mr->align, qemu_map_flags, offset);

A PMD mapping needs the virtual address and file offset to be congruent
modulo 2M.  With a memory-backend-file offset that is a 4K multiple but
not a 2M multiple, would this alignment still give no THP benefit?

Downstream, the memory_device_pre_plug() size check and the
nvdimm_prepare_memory_region() QEMU_ALIGN_DOWN(pmem_size, align) now
apply 2M constraints to these file-backed regions as well.

>  
>      if (memory < block->page_size) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=18