Re: [Xen-devel] [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping()

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Sep 12, 2017 at 01:14:45PM +0100, Andrew Cooper wrote:
> As with the create side of things, these are largely identical.  Most cases
> are actually destroying the mapping rather than replacing it with a stolen
> entry.
> 
> Reimplement their logic in replace_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 two suggestions:

>  int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
>                              unsigned int flags, unsigned int cache_flags)
>  {
> @@ -4136,12 +3959,14 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned 
> long frame,
>  {
>      struct vcpu *curr = current;
>      struct domain *currd = curr->domain;
> -    l1_pgentry_t ol1e;
> -    int rc;
> +    l1_pgentry_t nl1e = l1e_empty(), ol1e, *pl1e;
> +    struct page_info *page;
> +    mfn_t gl1mfn;
> +    int rc = GNTST_general_error;
>      unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
>  
>      /*
> -     * On top of the explicit settings done by create_grant_host_mapping()
> +     * On top of the explicit settings done by create_pv_host_mapping()
>       * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
>       * available and cachability flags, though.
>       */
> @@ -4150,24 +3975,96 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned 
> long frame,
>                             ? _PAGE_GLOBAL
>                             : _PAGE_GUEST_KERNEL | _PAGE_USER;
>  
> +    /*
> +     * addr comes from Xen's active_entry tracking, and was used successfully
> +     * to create a grant.
> +     *
> +     * 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.
> +     *
> +     * Passing a new_addr of zero is taken to mean destroy.  Passing a
> +     * non-zero new_addr has only ever been available via
> +     * GNTABOP_unmap_and_replace and only when using linear addresses.
> +     */

IMHO this should be moved before the function.

>      if ( flags & GNTMAP_contains_pte )
>      {
> -        if ( !new_addr )
> -            return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
> -                                             currd);
> +        /* Replace not available in this addressing mode. */
> +        if ( new_addr )
> +            goto out;
> +

   /*
    * addr comes from Xen's active_entry tracking so isn't guest controlled,
    * but it had still better be PTE-aligned.
    */

Consider keeping this comment?

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