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 05:36:32PM +0100, Andrew Cooper wrote:
> On 12/09/17 17:32, Wei Liu wrote:
> > On Tue, Sep 12, 2017 at 05:30:11PM +0100, Andrew Cooper wrote:
> >> On 12/09/17 15:58, Wei Liu wrote:
> >>> 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.
> >> Which bit?  The addr and GNTMAP_contains_pte need to be here to explain
> >> the curious if statement below.
> >>
> >> The final paragraph only makes sense in the context of the middle 
> >> paragraph.
> > At least the new_addr == 0 means destroying mapping bit.
> 
> I've folded the following incremental delta.
> 
> ~Andrew
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index f05a1d7..202eee2 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3958,6 +3958,11 @@ static bool steal_linear_address(unsigned long
> linear, l1_pgentry_t *out)
>      return okay;
>  }
>  
> +/*
> + * 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 !(flags & GNTMAP_contains_pte).
> + */
>  int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>                               uint64_t new_addr, unsigned int flags)
>  {
> @@ -3986,10 +3991,6 @@ int replace_grant_pv_mapping(uint64_t addr,
> unsigned long frame,
>       * 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.
>       */
>      if ( flags & GNTMAP_contains_pte )
>      {
> @@ -3997,6 +3998,7 @@ int replace_grant_pv_mapping(uint64_t addr,
> unsigned long frame,
>          if ( new_addr )
>              goto out;
>  
> +        /* Sanity check that we won't clobber the pagetable. */
>          if ( !IS_ALIGNED(addr, sizeof(nl1e)) )
>          {
>              ASSERT_UNREACHABLE();
> 

LGTM. Thanks.

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