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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>
> 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.

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

~Andrew

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