Re: [RFC PATCH v2 12/22] hw/xen: Add xen_overlay device for emulating shared xenheap pages

[Paul Durrant <xadimgnik@gmail.com>]

On 09/12/2022 09:56, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> For the shared info page and for grant tables, Xen shares its own pages
> from the "Xen heap" to the guest. The guest requests that a given page
> from a certain address space (XENMAPSPACE_shared_info, etc.) be mapped
> to a given GPA using the XENMEM_add_to_physmap hypercall.
> 
> To support that in qemu when *emulating* Xen, create a memory region
> (migratable) and allow it to be mapped as an overlay when requested.
> 
> Xen theoretically allows the same page to be mapped multiple times
> into the guest, but that's hard to track and reinstate over migration,
> so we automatically *unmap* any previous mapping when creating a new
> one. This approach has been used in production with.... a non-trivial
> number of guests expecting true Xen, without any problems yet being
> noticed.
> 
> This adds just the shared info page for now. The grant tables will be
> a larger region, and will need to be overlaid one page at a time. I
> think that means I need to create separate aliases for each page of
> the overall grant_frames region, so that they can be mapped individually.
> 

Is the following something you want in the commit log?

> Expecting some heckling at the use of xen_overlay_singleton. What is
> the best way to do that? Using qemu_find_recursive() every time seemed
> a bit wrong. But I suppose mapping it into the *guest* isn't a fast
> path, and if the actual grant table code is allowed to just stash the
> pointer it gets from xen_overlay_page_ptr() for later use then that
> isn't a fast path for device I/O either.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/meson.build   |   1 +
>   hw/i386/kvm/xen_overlay.c | 198 ++++++++++++++++++++++++++++++++++++++
>   hw/i386/kvm/xen_overlay.h |  14 +++
>   hw/i386/pc_piix.c         |   8 ++
>   4 files changed, 221 insertions(+)
>   create mode 100644 hw/i386/kvm/xen_overlay.c
>   create mode 100644 hw/i386/kvm/xen_overlay.h
> 
[snip]

> +static int xen_overlay_map_page_locked(uint32_t space, uint64_t idx, uint64_t gpa)
> +{
> +    MemoryRegion *ovl_page;
> +    int err;
> +
> +    if (space != XENMAPSPACE_shared_info || idx != 0)
> +        return -EINVAL;
> +
> +    if (!xen_overlay_singleton)
> +        return -ENOENT;
> +
> +    ovl_page = &xen_overlay_singleton->shinfo_mem;
> +
> +    /* Xen allows guests to map the same page as many times as it likes
> +     * into guest physical frames. We don't, because it would be hard
> +     * to track and restore them all. One mapping of each page is
> +     * perfectly sufficient for all known guests... and we've tested
> +     * that theory on a few now in other implementations. dwmw2. */
> +    if (memory_region_is_mapped(ovl_page)) {
> +        if (gpa == INVALID_GPA) {
> +            /* If removing shinfo page, turn the kernel magic off first */
> +            if (space == XENMAPSPACE_shared_info) {
> +                err = xen_overlay_set_be_shinfo(INVALID_GFN);
> +                if (err)
> +                    return err;
> +            }
> +            memory_region_del_subregion(get_system_memory(), ovl_page);
> +            goto done;

This seems a little ugly when you could...

> +        } else {
> +            /* Just move it */
> +            memory_region_set_address(ovl_page, gpa);
> +        }
> +    } else if (gpa != INVALID_GPA) {
> +        memory_region_add_subregion_overlap(get_system_memory(), gpa, ovl_page, 0);
> +    }
> +

... just wrap the following line in 'if (gpa != INVALID_GPA)'

Paul

> +    xen_overlay_set_be_shinfo(gpa >> XEN_PAGE_SHIFT);
> + done:
> +    xen_overlay_singleton->shinfo_gpa = gpa;
> +    return 0;
> +}
> +