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

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

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@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.
> 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();


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