[Xen-devel] [PATCH RESEND v9 01/14] arch/arm: add consistency check to REMOVE p2m changes

[Arianna Avanzini <avanzini.arianna@xxxxxxxxx>]

Currently, the REMOVE case of the switch in apply_p2m_changes()
does not perform any consistency check on the mapping to be removed.
More in detail, the code does not check if the guest address to be
unmapped is actually mapped to the machine address given as a
parameter.
This commit adds the above-described consistency check to the REMOVE
path of apply_p2m_changes() and lets a warning be emitted when trying
to remove a non-existent mapping. This is instrumental to one of the
following commits, which implements the possibility to trigger the
removal of p2m ranges via the memory_mapping DOMCTL for ARM.

Signed-off-by: Arianna Avanzini <avanzini.arianna@xxxxxxxxx>
Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
Cc: Paolo Valente <paolo.valente@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxxxxx>
Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Cc: Jan Beulich <JBeulich@xxxxxxxx>
Cc: Keir Fraser <keir@xxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: Eric Trudeau <etrudeau@xxxxxxxxxxxx>
Cc: Viktor Kleinik <viktor.kleinik@xxxxxxxxxxxxxxx>

---

    v9:
        - Don't return with an error when failing to remove a mapping, but
          simply keep unmapping.
        - Don't force assignment to the flush variable, as it is already
          set before the switch.
        - Change warning message to be more appropriate and clear; use the
          correct format for paddr_t and gdprintk(), which is more restricted
          than regular printk()s.
        - Adapt to rework of p2m-related functions for ARM.

    v8:
        - Re-add erroneously-removed increments to the maddr variable.
        - When failing to remove a mapping, add previously-mapped PT entry,
          unlock the p2m_lock and flush TLBs if necessary.
        - Emit an error message when failing to remove a mapping.
        - Remove tentative phrases from commit description.

    v7:
        - Silently ignore the fact that, when removing a mapping, the specified
          gfn is not mapped at all.
        - Remove spurious spacing change.

    v6:
        - Don't update "count" on REMOVE as it is only used inside the
          RELINQUISH case of the switch in apply_p2m_changes().
        - Return with an error if removal of a page fails instead of just
          skipping the page.

    v5:
        - Do not use a temporary variable to hold the machine address:
          use the "maddr" function parameter itself.
        - Increment the machine address also when first and second level
          mappings are not valid.
        - Get the actual machine frame number mapped to the guest frame
          number given as parameter to the function directly in the
          REMOVE case of the switch construct, as it might not be valid
          in other cases and its value might be uncorrectly used in the
          future.
        - Remove useless and/or harmful ASSERT; check however if the
          mapping is valid and skip the page if it is not.

    v4:
        - Remove useless and slow lookup and use already-available
          data from pte instead.
        - Correctly increment the local variable used to keep the
          machine address whose mapping is currently being removed.
        - Return with an error upon finding a mismatch between the
          actual machine address mapped to the guest address and
          the machine address passed as parameter, instead of just
          skipping the page.

---
 xen/arch/arm/p2m.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c0c011a..5a77189 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -596,12 +596,30 @@ static int apply_one_level(struct domain *d,
 
         break;
 
-    case RELINQUISH:
     case REMOVE:
+        if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) )
+        {
+            unsigned long mfn = orig_pte.p2m.base;
+            /*
+             * Ensure that the guest address addr currently being
+             * handled (that is in the range given as argument to
+             * this function) is actually mapped to the corresponding
+             * machine address in the specified range. maddr here is
+             * the machine address given to the function, while mfn
+             * is the machine frame number actually mapped to the
+             * guest address: check if the two correspond.
+             */
+             if ( p2m_valid(orig_pte) && *maddr != pfn_to_paddr(mfn) )
+                 printk("p2m_remove: mapping at %"PRIpaddr" is of maddr 
%"PRIpaddr" not %"PRIpaddr" as expected\n",
+                        *addr, pfn_to_paddr(mfn), *maddr);
+        }
+        /* fall through */
+    case RELINQUISH:
         if ( !p2m_valid(orig_pte) )
         {
             /* Progress up to next boundary */
             *addr = (*addr + level_size) & level_mask;
+            *maddr = (*maddr + level_size) & level_mask;
             return P2M_ONE_PROGRESS_NOP;
         }
 
@@ -614,6 +632,7 @@ static int apply_one_level(struct domain *d,
         p2m_write_pte(entry, pte, flush_cache);
 
         *addr += level_size;
+        *maddr += level_size;
 
         p2m->stats.mappings[level]--;
 
-- 
1.9.3


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