Re: [PATCH QEMU HCI-8.1 v5 32/36] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] vfio: unclaimed CPR BAR mmap entries are never reconciled after cpr-exec
- [Low] vfio: BAR mmaps in the CPR ramblock list get RAM-only mprotect in cpr-ram-rdonly
--

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

VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

This patch saves each VFIO BAR mmap in the CPR ramblock list so that
cpr_exec_cb() preserves it with MADV_DOEXEC. After cpr-exec, new QEMU finds
the saved mapping and reuses it instead of calling mmap() again. It also
blocks cpr-transfer for VFIO devices.

> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c

[ ... ]

> +/*
> + * A saved mapping is one preserved by cpr-exec, reuse it.  None means old
> + * QEMU didn't map the region, e.g. its mmap failed or it's mapped lazily,
> + * so map it afresh.
> + */
> +static void *vfio_region_cpr_find(VFIORegion *region, int index)
> +{
> +    g_autofree char *name = vfio_region_cpr_name(region, index);
> +    cpr_ramblock_t ram = cpr_find_ramblock(name);
> +
> +    if (ram.addr && ram.length != region->mmaps[index].size) {
> +        error_report("CPR mode=exec is requested, but the mapping of %s "
> +                     "can't be reused: addr=%p length=%zu, expected "
> +                     "length=%zu. Aborting", name, (void *)ram.addr,
> +                     ram.length, region->mmaps[index].size);
> +        exit(1);
> +    }
> +
> +    return (void *)(uintptr_t)ram.addr;
> +}

[Severity: Low]
Is there a path that removes an unclaimed entry after cpr-exec?

vfio_region_cpr_find() only reads the entry with cpr_find_ramblock(). It
never consumes it or marks it as claimed. The entry is removed only by
vfio_subregion_unmap() and vfio_region_finalize().

Suppose new QEMU never calls vfio_region_mmap() for a region that old QEMU
had saved. Examples are a different device configuration, a no-mmap
configuration, or a region that new QEMU maps lazily. The entry then stays
in cpr_state.ramblocks.

cpr_exec_cb() calls cpr_walk_ramblock(preserve_ram), which walks every
entry in the list. A stale BAR entry would be marked MADV_DOEXEC again at
the next cpr-exec.

If a stale entry is later found by name with a different length, this code
calls exit(1) instead of falling back to a fresh mmap.

I could not find any code in the series that reconciles unclaimed entries.
Should unclaimed entries be dropped after the new QEMU finishes device
setup, and should a size mismatch fall back to a fresh mmap?

> +
> +static void vfio_region_cpr_save(VFIORegion *region, int index)
> +{
> +    g_autofree char *name = vfio_region_cpr_name(region, index);
> +    void *addr = region->mmaps[index].mmap;
> +    size_t size = region->mmaps[index].size;
> +
> +    cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
> +}

[Severity: Low]
Can putting device BAR mappings into the generic CPR ramblock list
interfere with the -cpr-ram-rdonly debug option?

vfio_region_cpr_save() adds each BAR mmap to the same list that
migration/cpr-exec.c treats as guest RAM. When cpr_ram_rdonly is set,
cpr_exec_cb() runs cpr_walk_ramblock(mark_ram_rdonly) over this list, and
that now includes the BAR mappings.

mark_ram_rw() runs on the exec-failure path and in the new QEMU. It calls
qemu_mprotect_rw() with PROT_READ|PROT_WRITE on every entry, and it calls
abort() on failure:

migration/cpr-exec.c:mark_ram_rw() {
    if (qemu_mprotect_rw((void *) ram.addr, ram.length)) {
        ...
        abort();
    }
}

vfio_region_mmap() derives prot from region->flags (READ and WRITE). A BAR
mapped read-only or with no access would be widened to read-write. If the
kernel or a driver rejects the mprotect(), QEMU aborts.

Typical vfio-pci BARs are read-write, and the option is debug-only. Should
BAR entries be excluded from the mark_ram_rdonly and mark_ram_rw walks?

[ ... ]

> @@ -427,6 +474,8 @@ void vfio_region_finalize(VFIORegion *region)
>  
>      for (i = 0; i < region->nr_mmaps; i++) {
>          if (region->mmaps[i].mmap) {
> +            g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
> +            cpr_delete_ramblock(cpr_name);
>              munmap(region->mmaps[i].mmap, region->mmaps[i].size);
>          }
>      }

This is the second of only two places that delete a BAR entry, as noted in
the vfio_region_cpr_find() comment above.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=32