Re: [RFC PATCH v1 21/25] hw/xen: Add emulated implementation of grant table operations

[Paul Durrant <xadimgnik@gmail.com>]

On 02/03/2023 15:34, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> This is limited to mapping a single grant at a time, because under Xen the
> pages are mapped *contiguously* into qemu's address space, and that's very
> hard to do when those pages actually come from anonymous mappings in qemu
> in the first place.
> 
> Eventually perhaps we can look at using shared mappings of actual objects
> for system RAM, and then we can make new mappings of the same backing
> store (be it deleted files, shmem, whatever). But for now let's stick to
> a page at a time.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/xen_gnttab.c | 299 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 296 insertions(+), 3 deletions(-)
> 
[snip]
> +static uint64_t gnt_ref(XenGnttabState *s, grant_ref_t ref, int prot)
> +{
> +    uint16_t mask = GTF_type_mask | GTF_sub_page;
> +    grant_entry_v1_t gnt, *gnt_p;
> +    int retries = 0;
> +
> +    if (ref >= s->max_frames * ENTRIES_PER_FRAME_V1 ||
> +        s->map_track[ref] == UINT8_MAX) {
> +        return INVALID_GPA;
> +    }
> +
> +    if (prot & PROT_WRITE) {
> +        mask |= GTF_readonly;
> +    }
> +
> +    gnt_p = &s->entries.v1[ref];
> +
> +    /*
> +     * The guest can legitimately be changing the GTF_readonly flag. Allow

I'd call a guest playing with the ref after setting GTF_permit_access a 
buggy guest and not bother with the loop.

> +     * that, but don't let a malicious guest cause a livelock.
> +     */
> +    for (retries = 0; retries < 5; retries++) {
> +        uint16_t new_flags;
> +
> +        /* Read the entry before an atomic operation on its flags */
> +        gnt = *(volatile grant_entry_v1_t *)gnt_p;
> +
> +        if ((gnt.flags & mask) != GTF_permit_access ||
> +            gnt.domid != DOMID_QEMU) {
> +            return INVALID_GPA;
> +        }
> +
> +        new_flags = gnt.flags | GTF_reading;
> +        if (prot & PROT_WRITE) {
> +            new_flags |= GTF_writing;
> +        }
> +
> +        if (qatomic_cmpxchg(&gnt_p->flags, gnt.flags, new_flags) == gnt.flags) {

Xen actually does a cmpxchg on both the flags and the domid. We probably 
ought to fail to set the flags if the guest is playing with the domid 
but since we're single-tenant it doesn't *really* matter... just a 
nice-to-have. So...

Reviewed-by: Paul Durrant <paul@xen.org>