[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] xen/grant_table: introduce GNTTABOP_unmap_and_duplicate



>>> On 04.11.13 at 17:30, Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
>  int replace_grant_host_mapping(
> -    uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int 
> flags)
> +    uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int 
> flags,
> +    bool_t zero_new_addr)

So why can this flag not be merged into "flags"?

> -    if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(),
> -                                gl1mfn, curr, 0)) )
> +    if ( zero_new_addr )
>      {
> -        page_unlock(l1pg);
> -        put_page(l1pg);
> -        MEM_LOG("Cannot delete PTE entry at %p", (unsigned long *)pl1e);
> -        guest_unmap_l1e(curr, pl1e);
> -        return GNTST_general_error;
> +        if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(),
> +                                    gl1mfn, curr, 0)) )
> +        {
> +            page_unlock(l1pg);
> +            put_page(l1pg);
> +            MEM_LOG("Cannot delete PTE entry at %p", (unsigned long *)pl1e);
> +            guest_unmap_l1e(curr, pl1e);
> +            return GNTST_general_error;
> +        }
> +    }
> +    else
> +    {
> +        /* Increase refcount */
> +        if ( unlikely(!get_page_and_type(l1e_get_page(ol1e), curr->domain,
> +                                         PGT_writable_page)) )
> +        {
> +            page_unlock(l1pg);
> +            put_page(l1pg);
> +            guest_unmap_l1e(curr, pl1e);
> +            MEM_LOG("Cannot increase refcount of frame at %"PRI_mfn,
> +                    page_to_mfn(l1e_get_page(ol1e)));
> +            return GNTST_general_error;
> +        }

The two error paths are identical except for the messages they
log. Please avoid unnecessary code duplication like this. That'll
also reduce the churn the patch causes due to the indentation
change. (Moving the guest_unmap_l1e() ahead of the issuing of
the message is, btw, appreciated.)

> +/*
> + * GNTTABOP_unmap_and_duplicate: Destroy one or more grant-reference mappings
> + * tracked by <handle> but atomically replace the page table entry with one
> + * pointing to the machine address under <new_addr>.
> + * NOTES:
> + *  1. The call may fail in an undefined manner if either mapping is not
> + *     tracked by <handle>.
> + *  2. After executing a batch of unmaps, it is guaranteed that no stale
> + *     mappings will remain in the device or host TLBs.
> + */
> +struct gnttab_unmap_and_duplicate {
> +    /* IN parameters. */
> +    uint64_t host_addr;
> +    uint64_t new_addr;
> +    grant_handle_t handle;
> +    /* OUT parameters. */
> +    int16_t  status;              /* => enum grant_status */
> +};

If you re-used the identical in layout gnttab_unmap_and_replace
here (perhaps nevertheless creating aliases with the right name),
wouldn't it be possible to also reduce the amount of added code
duplication in xen/common/grant_table.c?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.