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: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.

> 
> >
> >>      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?
> 
> Is it really that helpful?  It is in the context of "addr comes from
> Xen's active_entry tracking, and was used successfully to create the grant".

OK. I won't insist on this.

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