Re: [Xen-devel] [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping()

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Sep 12, 2017 at 01:14:43PM +0100, Andrew Cooper wrote:
> create_grant_{pte,va}_mapping() are nearly identical; all that is really
> different between them is how they convert their addr parameter to the pte to
> install the grant into.
> 
> Reimplement their logic in create_grant_pv_mapping() in a mostly common way.
> 
> No (intended) change in behaviour from a guests point of view.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>

With one optional request.

> @@ -4126,13 +4012,67 @@ int create_grant_pv_mapping(uint64_t addr, unsigned 
> long frame,
>                              unsigned int flags, unsigned int cache_flags)
>  {
>      struct vcpu *curr = current;
> -    l1_pgentry_t nl1e;
> +    struct domain *currd = curr->domain;
> +    l1_pgentry_t nl1e, ol1e, *pl1e;
> +    struct page_info *page;
> +    mfn_t gl1mfn;
> +    int rc = GNTST_general_error;
>  
>      nl1e = l1e_from_pfn(frame, grant_to_pte_flags(flags, cache_flags));
> +    nl1e = adjust_guest_l1e(nl1e, currd);
>  
> +    /*
> +     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
> +     * machine address of an L1e the guest has nominated to be altered, or a
> +     * linear address we need to look up the appropriate L1e for.
> +     */
>      if ( flags & GNTMAP_contains_pte )
> -        return create_grant_pte_mapping(addr, nl1e, curr);
> -    return create_grant_va_mapping(addr, nl1e, curr);
> +    {
> +        /* addr must be suitably aligned, or we will corrupt adjacent ptes. 
> */
> +        if ( !IS_ALIGNED(addr, sizeof(nl1e)) )
> +            goto out;

Can you sprinkle some gdprintk's in these error paths to aid debugging?

> +
> +        gl1mfn = _mfn(addr >> PAGE_SHIFT);
> +
> +        if ( !get_page_from_mfn(gl1mfn, currd) )
> +            goto out;
> +
> +        pl1e = map_domain_page(gl1mfn) + (addr & ~PAGE_MASK);
> +    }
> +    else
> +    {
> +        pl1e = map_guest_l1e(addr, &gl1mfn);
> +
> +        if ( !pl1e )
> +            goto out;
> +
> +        if ( !get_page_from_mfn(gl1mfn, currd) )
> +            goto out_unmap;
> +    }
> +
> +    page = mfn_to_page(gl1mfn);
> +    if ( !page_lock(page) )
> +        goto out_put;
> +
> +    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
> +        goto out_unlock;
> +
> +    ol1e = *pl1e;
> +    if ( UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn_x(gl1mfn), curr, 0) )
> +        rc = GNTST_okay;
> +
> + out_unlock:
> +    page_unlock(page);
> + out_put:
> +    put_page(page);
> + out_unmap:
> +    unmap_domain_page(pl1e);
> +
> +    if ( rc == GNTST_okay )
> +        put_page_from_l1e(ol1e, currd);
> +
> + out:
> +    return rc;
>  }
>  
>  int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
> -- 
> 2.1.4
> 

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