[PATCH 0/2] x86/shadow: further code size reduction

[PATCH 0/2] x86/shadow: further code size reduction

[Jan Beulich]

Following some earlier work in this regard, the first patch here
constitutes what I recognized as further potential back then, but
which needed a little more time to consider the options of how to
reasonably split/move things. The second patch is just another
small step recognized as being possible in the process of putting
together the first one.

1: compile most write emulation code just once
2: avoid extra local array variable

Signed-off-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH 1/2] x86/shadow: compile most write emulation code just once

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 18060 bytes --]

No need to compile all of this code three times, as most of it really
is guest mode independent. The savings are between 3k and 4k of binary
code in my builds.

No functional change (i.e. only formatting and naming changes) except
for
- sh_emulate_map_dest()'s user mode check corrected for the PV case
  (affecting debugging mode only, this isn't being split out)
- simplifying the vaddr argument to emulate_gva_to_mfn() for the second
  part in the cross page write case

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1679,6 +1679,211 @@ static unsigned int shadow_get_allocatio
 }
 
 /**************************************************************************/
+/* Handling guest writes to pagetables. */
+
+/* Translate a VA to an MFN, injecting a page-fault if we fail. */
+#define BAD_GVA_TO_GFN (~0UL)
+#define BAD_GFN_TO_MFN (~1UL)
+#define READONLY_GFN   (~2UL)
+static mfn_t emulate_gva_to_mfn(struct vcpu *v, unsigned long vaddr,
+                                struct sh_emulate_ctxt *sh_ctxt)
+{
+    unsigned long gfn;
+    struct page_info *page;
+    mfn_t mfn;
+    p2m_type_t p2mt;
+    uint32_t pfec = PFEC_page_present | PFEC_write_access;
+
+    /* Translate the VA to a GFN. */
+    gfn = paging_get_hostmode(v)->gva_to_gfn(v, NULL, vaddr, &pfec);
+    if ( gfn == INVALID_GFN )
+    {
+        if ( is_hvm_vcpu(v) )
+            hvm_inject_page_fault(pfec, vaddr);
+        else
+            propagate_page_fault(vaddr, pfec);
+        return _mfn(BAD_GVA_TO_GFN);
+    }
+
+    /* Translate the GFN to an MFN. */
+    ASSERT(!paging_locked_by_me(v->domain));
+
+    page = get_page_from_gfn(v->domain, gfn, &p2mt, P2M_ALLOC);
+
+    /* Sanity checking. */
+    if ( page == NULL )
+    {
+        return _mfn(BAD_GFN_TO_MFN);
+    }
+    if ( p2m_is_discard_write(p2mt) )
+    {
+        put_page(page);
+        return _mfn(READONLY_GFN);
+    }
+    if ( !p2m_is_ram(p2mt) )
+    {
+        put_page(page);
+        return _mfn(BAD_GFN_TO_MFN);
+    }
+    mfn = page_to_mfn(page);
+    ASSERT(mfn_valid(mfn));
+
+    v->arch.paging.last_write_was_pt = !!sh_mfn_is_a_page_table(mfn);
+    /*
+     * Note shadow cannot page out or unshare this mfn, so the map won't
+     * disappear. Otherwise, caller must hold onto page until done.
+     */
+    put_page(page);
+
+    return mfn;
+}
+
+/* Check that the user is allowed to perform this write. */
+void *sh_emulate_map_dest(struct vcpu *v, unsigned long vaddr,
+                          unsigned int bytes,
+                          struct sh_emulate_ctxt *sh_ctxt)
+{
+    struct domain *d = v->domain;
+    void *map;
+
+    sh_ctxt->mfn1 = emulate_gva_to_mfn(v, vaddr, sh_ctxt);
+    if ( !mfn_valid(sh_ctxt->mfn1) )
+        return ((mfn_x(sh_ctxt->mfn1) == BAD_GVA_TO_GFN) ?
+                MAPPING_EXCEPTION :
+                (mfn_x(sh_ctxt->mfn1) == READONLY_GFN) ?
+                MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
+
+#ifndef NDEBUG
+    /* We don't emulate user-mode writes to page tables. */
+    if ( has_hvm_container_domain(d)
+         ? hvm_get_seg_reg(x86_seg_ss, sh_ctxt)->attr.fields.dpl == 3
+         : !guest_kernel_mode(v, guest_cpu_user_regs()) )
+    {
+        gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached "
+                 "emulate_map_dest(). This should never happen!\n");
+        return MAPPING_UNHANDLEABLE;
+    }
+#endif
+
+    /* Unaligned writes mean probably this isn't a pagetable. */
+    if ( vaddr & (bytes - 1) )
+        sh_remove_shadows(d, sh_ctxt->mfn1, 0, 0 /* Slow, can fail. */ );
+
+    if ( likely(((vaddr + bytes - 1) & PAGE_MASK) == (vaddr & PAGE_MASK)) )
+    {
+        /* Whole write fits on a single page. */
+        sh_ctxt->mfn2 = _mfn(INVALID_MFN);
+        map = map_domain_page(sh_ctxt->mfn1) + (vaddr & ~PAGE_MASK);
+    }
+    else
+    {
+        mfn_t mfns[2];
+
+        /*
+         * Cross-page emulated writes are only supported for HVM guests;
+         * PV guests ought to know better.
+         */
+        if ( !is_hvm_domain(d) )
+            return MAPPING_UNHANDLEABLE;
+
+        /* This write crosses a page boundary. Translate the second page. */
+        sh_ctxt->mfn2 = emulate_gva_to_mfn(v, vaddr + bytes, sh_ctxt);
+        if ( !mfn_valid(sh_ctxt->mfn2) )
+            return ((mfn_x(sh_ctxt->mfn2) == BAD_GVA_TO_GFN) ?
+                    MAPPING_EXCEPTION :
+                    (mfn_x(sh_ctxt->mfn2) == READONLY_GFN) ?
+                    MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
+
+        /* Cross-page writes mean probably not a pagetable. */
+        sh_remove_shadows(d, sh_ctxt->mfn2, 0, 0 /* Slow, can fail. */ );
+
+        mfns[0] = sh_ctxt->mfn1;
+        mfns[1] = sh_ctxt->mfn2;
+        map = vmap(mfns, 2);
+        if ( !map )
+            return MAPPING_UNHANDLEABLE;
+        map += (vaddr & ~PAGE_MASK);
+    }
+
+#if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
+    /*
+     * Remember if the bottom bit was clear, so we can choose not to run
+     * the change through the verify code if it's still clear afterwards.
+     */
+    sh_ctxt->low_bit_was_clear = map != NULL && !(*(u8 *)map & _PAGE_PRESENT);
+#endif
+
+    return map;
+}
+
+/*
+ * Tidy up after the emulated write: mark pages dirty, verify the new
+ * contents, and undo the mapping.
+ */
+void sh_emulate_unmap_dest(struct vcpu *v, void *addr, unsigned int bytes,
+                           struct sh_emulate_ctxt *sh_ctxt)
+{
+    u32 b1 = bytes, b2 = 0, shflags;
+
+    /*
+     * We can avoid re-verifying the page contents after the write if:
+     *  - it was no larger than the PTE type of this pagetable;
+     *  - it was aligned to the PTE boundaries; and
+     *  - _PAGE_PRESENT was clear before and after the write.
+     */
+    shflags = mfn_to_page(sh_ctxt->mfn1)->shadow_flags;
+#if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
+    if ( sh_ctxt->low_bit_was_clear
+         && !(*(u8 *)addr & _PAGE_PRESENT)
+         && ((!(shflags & SHF_32)
+              /*
+               * Not shadowed 32-bit: aligned 64-bit writes that leave
+               * the present bit unset are safe to ignore.
+               */
+              && ((unsigned long)addr & 7) == 0
+              && bytes <= 8)
+             ||
+             (!(shflags & (SHF_PAE|SHF_64))
+              /*
+               * Not shadowed PAE/64-bit: aligned 32-bit writes that
+               * leave the present bit unset are safe to ignore.
+               */
+              && ((unsigned long)addr & 3) == 0
+              && bytes <= 4)) )
+    {
+        /* Writes with this alignment constraint can't possibly cross pages. */
+        ASSERT(!mfn_valid(sh_ctxt->mfn2));
+    }
+    else
+#endif /* SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY */
+    {
+        if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
+        {
+            /* Validate as two writes, one to each page. */
+            b1 = PAGE_SIZE - (((unsigned long)addr) & ~PAGE_MASK);
+            b2 = bytes - b1;
+            ASSERT(b2 < bytes);
+        }
+        if ( likely(b1 > 0) )
+            sh_validate_guest_pt_write(v, sh_ctxt->mfn1, addr, b1);
+        if ( unlikely(b2 > 0) )
+            sh_validate_guest_pt_write(v, sh_ctxt->mfn2, addr + b1, b2);
+    }
+
+    paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn1));
+
+    if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
+    {
+        paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn2));
+        vunmap((void *)((unsigned long)addr & PAGE_MASK));
+    }
+    else
+        unmap_domain_page(addr);
+
+    atomic_inc(&v->domain->arch.paging.shadow.gtable_dirty_version);
+}
+
+/**************************************************************************/
 /* Hash table for storing the guest->shadow mappings.
  * The table itself is an array of pointers to shadows; the shadows are then
  * threaded on a singly-linked list of shadows with the same hash value */
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4606,141 +4606,7 @@ static void sh_pagetable_dying(struct vc
 #endif
 
 /**************************************************************************/
-/* Handling HVM guest writes to pagetables  */
-
-/* Translate a VA to an MFN, injecting a page-fault if we fail */
-#define BAD_GVA_TO_GFN (~0UL)
-#define BAD_GFN_TO_MFN (~1UL)
-#define READONLY_GFN   (~2UL)
-static mfn_t emulate_gva_to_mfn(struct vcpu *v,
-                                unsigned long vaddr,
-                                struct sh_emulate_ctxt *sh_ctxt)
-{
-    unsigned long gfn;
-    struct page_info *page;
-    mfn_t mfn;
-    p2m_type_t p2mt;
-    uint32_t pfec = PFEC_page_present | PFEC_write_access;
-
-    /* Translate the VA to a GFN */
-    gfn = sh_gva_to_gfn(v, NULL, vaddr, &pfec);
-    if ( gfn == INVALID_GFN )
-    {
-        if ( is_hvm_vcpu(v) )
-            hvm_inject_page_fault(pfec, vaddr);
-        else
-            propagate_page_fault(vaddr, pfec);
-        return _mfn(BAD_GVA_TO_GFN);
-    }
-
-    /* Translate the GFN to an MFN */
-    ASSERT(!paging_locked_by_me(v->domain));
-
-    page = get_page_from_gfn(v->domain, gfn, &p2mt, P2M_ALLOC);
-
-    /* Sanity checking */
-    if ( page == NULL )
-    {
-        return _mfn(BAD_GFN_TO_MFN);
-    }
-    if ( p2m_is_discard_write(p2mt) )
-    {
-        put_page(page);
-        return _mfn(READONLY_GFN);
-    }
-    if ( !p2m_is_ram(p2mt) )
-    {
-        put_page(page);
-        return _mfn(BAD_GFN_TO_MFN);
-    }
-    mfn = page_to_mfn(page);
-    ASSERT(mfn_valid(mfn));
-
-    v->arch.paging.last_write_was_pt = !!sh_mfn_is_a_page_table(mfn);
-    /* Note shadow cannot page out or unshare this mfn, so the map won't
-     * disappear. Otherwise, caller must hold onto page until done. */
-    put_page(page);
-    return mfn;
-}
-
-/* Check that the user is allowed to perform this write.
- * Returns a mapped pointer to write to, or NULL for error. */
-#define MAPPING_UNHANDLEABLE ((void *)(unsigned long)X86EMUL_UNHANDLEABLE)
-#define MAPPING_EXCEPTION    ((void *)(unsigned long)X86EMUL_EXCEPTION)
-#define MAPPING_SILENT_FAIL  ((void *)(unsigned long)X86EMUL_OKAY)
-#define emulate_map_dest_failed(rc) ((unsigned long)(rc) <= 3)
-static void *emulate_map_dest(struct vcpu *v,
-                              unsigned long vaddr,
-                              u32 bytes,
-                              struct sh_emulate_ctxt *sh_ctxt)
-{
-    struct domain *d = v->domain;
-    void *map = NULL;
-
-    sh_ctxt->mfn1 = emulate_gva_to_mfn(v, vaddr, sh_ctxt);
-    if ( !mfn_valid(sh_ctxt->mfn1) )
-        return ((mfn_x(sh_ctxt->mfn1) == BAD_GVA_TO_GFN) ?
-                MAPPING_EXCEPTION :
-                (mfn_x(sh_ctxt->mfn1) == READONLY_GFN) ?
-                MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
-
-#ifndef NDEBUG
-    /* We don't emulate user-mode writes to page tables */
-    if ( hvm_get_seg_reg(x86_seg_ss, sh_ctxt)->attr.fields.dpl == 3 )
-    {
-        gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached "
-                 "emulate_map_dest(). This should never happen!\n");
-        return MAPPING_UNHANDLEABLE;
-    }
-#endif
-
-    /* Unaligned writes mean probably this isn't a pagetable */
-    if ( vaddr & (bytes - 1) )
-        sh_remove_shadows(d, sh_ctxt->mfn1, 0, 0 /* Slow, can fail */ );
-
-    if ( likely(((vaddr + bytes - 1) & PAGE_MASK) == (vaddr & PAGE_MASK)) )
-    {
-        /* Whole write fits on a single page */
-        sh_ctxt->mfn2 = _mfn(INVALID_MFN);
-        map = map_domain_page(sh_ctxt->mfn1) + (vaddr & ~PAGE_MASK);
-    }
-    else
-    {
-        mfn_t mfns[2];
-
-        /* Cross-page emulated writes are only supported for HVM guests;
-         * PV guests ought to know better */
-        if ( !is_hvm_domain(d) )
-            return MAPPING_UNHANDLEABLE;
-
-        /* This write crosses a page boundary.  Translate the second page */
-        sh_ctxt->mfn2 = emulate_gva_to_mfn(v, (vaddr + bytes - 1) & PAGE_MASK,
-                                           sh_ctxt);
-        if ( !mfn_valid(sh_ctxt->mfn2) )
-            return ((mfn_x(sh_ctxt->mfn2) == BAD_GVA_TO_GFN) ?
-                    MAPPING_EXCEPTION :
-                    (mfn_x(sh_ctxt->mfn2) == READONLY_GFN) ?
-                    MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
-
-        /* Cross-page writes mean probably not a pagetable */
-        sh_remove_shadows(d, sh_ctxt->mfn2, 0, 0 /* Slow, can fail */ );
-
-        mfns[0] = sh_ctxt->mfn1;
-        mfns[1] = sh_ctxt->mfn2;
-        map = vmap(mfns, 2);
-        if ( !map )
-            return MAPPING_UNHANDLEABLE;
-        map += (vaddr & ~PAGE_MASK);
-    }
-
-#if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
-    /* Remember if the bottom bit was clear, so we can choose not to run
-     * the change through the verify code if it's still clear afterwards */
-    sh_ctxt->low_bit_was_clear = map != NULL && !(*(u8 *)map & _PAGE_PRESENT);
-#endif
-
-    return map;
-}
+/* Handling guest writes to pagetables. */
 
 /* Tidy up after the emulated write: mark pages dirty, verify the new
  * contents, and undo the mapping */
@@ -4749,8 +4615,6 @@ static void emulate_unmap_dest(struct vc
                                u32 bytes,
                                struct sh_emulate_ctxt *sh_ctxt)
 {
-    u32 b1 = bytes, b2 = 0, shflags;
-
     ASSERT(mfn_valid(sh_ctxt->mfn1));
 
     /* If we are writing lots of PTE-aligned zeros, might want to unshadow */
@@ -4764,56 +4628,7 @@ static void emulate_unmap_dest(struct vc
     else
         reset_early_unshadow(v);
 
-    /* We can avoid re-verifying the page contents after the write if:
-     *  - it was no larger than the PTE type of this pagetable;
-     *  - it was aligned to the PTE boundaries; and
-     *  - _PAGE_PRESENT was clear before and after the write. */
-    shflags = mfn_to_page(sh_ctxt->mfn1)->shadow_flags;
-#if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
-    if ( sh_ctxt->low_bit_was_clear
-         && !(*(u8 *)addr & _PAGE_PRESENT)
-         && ((!(shflags & SHF_32)
-              /* Not shadowed 32-bit: aligned 64-bit writes that leave
-               * the present bit unset are safe to ignore. */
-              && ((unsigned long)addr & 7) == 0
-              && bytes <= 8)
-             ||
-             (!(shflags & (SHF_PAE|SHF_64))
-              /* Not shadowed PAE/64-bit: aligned 32-bit writes that
-               * leave the present bit unset are safe to ignore. */
-              && ((unsigned long)addr & 3) == 0
-              && bytes <= 4)) )
-    {
-        /* Writes with this alignment constraint can't possibly cross pages */
-        ASSERT(!mfn_valid(sh_ctxt->mfn2));
-    }
-    else
-#endif /* SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY */
-    {
-        if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
-        {
-            /* Validate as two writes, one to each page */
-            b1 = PAGE_SIZE - (((unsigned long)addr) & ~PAGE_MASK);
-            b2 = bytes - b1;
-            ASSERT(b2 < bytes);
-        }
-        if ( likely(b1 > 0) )
-            sh_validate_guest_pt_write(v, sh_ctxt->mfn1, addr, b1);
-        if ( unlikely(b2 > 0) )
-            sh_validate_guest_pt_write(v, sh_ctxt->mfn2, addr + b1, b2);
-    }
-
-    paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn1));
-
-    if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
-    {
-        paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn2));
-        vunmap((void *)((unsigned long)addr & PAGE_MASK));
-    }
-    else
-        unmap_domain_page(addr);
-
-    atomic_inc(&v->domain->arch.paging.shadow.gtable_dirty_version);
+    sh_emulate_unmap_dest(v, addr, bytes, sh_ctxt);
 }
 
 static int
@@ -4826,8 +4641,8 @@ sh_x86_emulate_write(struct vcpu *v, uns
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
-    addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
-    if ( emulate_map_dest_failed(addr) )
+    addr = sh_emulate_map_dest(v, vaddr, bytes, sh_ctxt);
+    if ( sh_emulate_map_dest_failed(addr) )
         return (long)addr;
 
     paging_lock(v->domain);
@@ -4868,8 +4683,8 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
-    addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
-    if ( emulate_map_dest_failed(addr) )
+    addr = sh_emulate_map_dest(v, vaddr, bytes, sh_ctxt);
+    if ( sh_emulate_map_dest_failed(addr) )
         return (long)addr;
 
     paging_lock(v->domain);
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -393,6 +393,17 @@ void shadow_update_paging_modes(struct v
  * With user_only == 1, unhooks only the user-mode mappings. */
 void shadow_unhook_mappings(struct domain *d, mfn_t smfn, int user_only);
 
+/* Returns a mapped pointer to write to, or one of the following error
+ * indicators. */
+#define MAPPING_UNHANDLEABLE ((void *)(unsigned long)X86EMUL_UNHANDLEABLE)
+#define MAPPING_EXCEPTION    ((void *)(unsigned long)X86EMUL_EXCEPTION)
+#define MAPPING_SILENT_FAIL  ((void *)(unsigned long)X86EMUL_OKAY)
+#define sh_emulate_map_dest_failed(rc) ((unsigned long)(rc) <= 3)
+void *sh_emulate_map_dest(struct vcpu *v, unsigned long vaddr,
+                          unsigned int bytes, struct sh_emulate_ctxt *sh_ctxt);
+void sh_emulate_unmap_dest(struct vcpu *v, void *addr, unsigned int bytes,
+                           struct sh_emulate_ctxt *sh_ctxt);
+
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
 /* Allow a shadowed page to go out of sync */
 int sh_unsync(struct vcpu *v, mfn_t gmfn);



[-- Attachment #2: x86-sh-emul-writes.patch --]
[-- Type: text/plain, Size: 18115 bytes --]

x86/shadow: compile most write emulation code just once

No need to compile all of this code three times, as most of it really
is guest mode independent. The savings are between 3k and 4k of binary
code in my builds.

No functional change (i.e. only formatting and naming changes) except
for
- sh_emulate_map_dest()'s user mode check corrected for the PV case
  (affecting debugging mode only, this isn't being split out)
- simplifying the vaddr argument to emulate_gva_to_mfn() for the second
  part in the cross page write case

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1679,6 +1679,211 @@ static unsigned int shadow_get_allocatio
 }
 
 /**************************************************************************/
+/* Handling guest writes to pagetables. */
+
+/* Translate a VA to an MFN, injecting a page-fault if we fail. */
+#define BAD_GVA_TO_GFN (~0UL)
+#define BAD_GFN_TO_MFN (~1UL)
+#define READONLY_GFN   (~2UL)
+static mfn_t emulate_gva_to_mfn(struct vcpu *v, unsigned long vaddr,
+                                struct sh_emulate_ctxt *sh_ctxt)
+{
+    unsigned long gfn;
+    struct page_info *page;
+    mfn_t mfn;
+    p2m_type_t p2mt;
+    uint32_t pfec = PFEC_page_present | PFEC_write_access;
+
+    /* Translate the VA to a GFN. */
+    gfn = paging_get_hostmode(v)->gva_to_gfn(v, NULL, vaddr, &pfec);
+    if ( gfn == INVALID_GFN )
+    {
+        if ( is_hvm_vcpu(v) )
+            hvm_inject_page_fault(pfec, vaddr);
+        else
+            propagate_page_fault(vaddr, pfec);
+        return _mfn(BAD_GVA_TO_GFN);
+    }
+
+    /* Translate the GFN to an MFN. */
+    ASSERT(!paging_locked_by_me(v->domain));
+
+    page = get_page_from_gfn(v->domain, gfn, &p2mt, P2M_ALLOC);
+
+    /* Sanity checking. */
+    if ( page == NULL )
+    {
+        return _mfn(BAD_GFN_TO_MFN);
+    }
+    if ( p2m_is_discard_write(p2mt) )
+    {
+        put_page(page);
+        return _mfn(READONLY_GFN);
+    }
+    if ( !p2m_is_ram(p2mt) )
+    {
+        put_page(page);
+        return _mfn(BAD_GFN_TO_MFN);
+    }
+    mfn = page_to_mfn(page);
+    ASSERT(mfn_valid(mfn));
+
+    v->arch.paging.last_write_was_pt = !!sh_mfn_is_a_page_table(mfn);
+    /*
+     * Note shadow cannot page out or unshare this mfn, so the map won't
+     * disappear. Otherwise, caller must hold onto page until done.
+     */
+    put_page(page);
+
+    return mfn;
+}
+
+/* Check that the user is allowed to perform this write. */
+void *sh_emulate_map_dest(struct vcpu *v, unsigned long vaddr,
+                          unsigned int bytes,
+                          struct sh_emulate_ctxt *sh_ctxt)
+{
+    struct domain *d = v->domain;
+    void *map;
+
+    sh_ctxt->mfn1 = emulate_gva_to_mfn(v, vaddr, sh_ctxt);
+    if ( !mfn_valid(sh_ctxt->mfn1) )
+        return ((mfn_x(sh_ctxt->mfn1) == BAD_GVA_TO_GFN) ?
+                MAPPING_EXCEPTION :
+                (mfn_x(sh_ctxt->mfn1) == READONLY_GFN) ?
+                MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
+
+#ifndef NDEBUG
+    /* We don't emulate user-mode writes to page tables. */
+    if ( has_hvm_container_domain(d)
+         ? hvm_get_seg_reg(x86_seg_ss, sh_ctxt)->attr.fields.dpl == 3
+         : !guest_kernel_mode(v, guest_cpu_user_regs()) )
+    {
+        gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached "
+                 "emulate_map_dest(). This should never happen!\n");
+        return MAPPING_UNHANDLEABLE;
+    }
+#endif
+
+    /* Unaligned writes mean probably this isn't a pagetable. */
+    if ( vaddr & (bytes - 1) )
+        sh_remove_shadows(d, sh_ctxt->mfn1, 0, 0 /* Slow, can fail. */ );
+
+    if ( likely(((vaddr + bytes - 1) & PAGE_MASK) == (vaddr & PAGE_MASK)) )
+    {
+        /* Whole write fits on a single page. */
+        sh_ctxt->mfn2 = _mfn(INVALID_MFN);
+        map = map_domain_page(sh_ctxt->mfn1) + (vaddr & ~PAGE_MASK);
+    }
+    else
+    {
+        mfn_t mfns[2];
+
+        /*
+         * Cross-page emulated writes are only supported for HVM guests;
+         * PV guests ought to know better.
+         */
+        if ( !is_hvm_domain(d) )
+            return MAPPING_UNHANDLEABLE;
+
+        /* This write crosses a page boundary. Translate the second page. */
+        sh_ctxt->mfn2 = emulate_gva_to_mfn(v, vaddr + bytes, sh_ctxt);
+        if ( !mfn_valid(sh_ctxt->mfn2) )
+            return ((mfn_x(sh_ctxt->mfn2) == BAD_GVA_TO_GFN) ?
+                    MAPPING_EXCEPTION :
+                    (mfn_x(sh_ctxt->mfn2) == READONLY_GFN) ?
+                    MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
+
+        /* Cross-page writes mean probably not a pagetable. */
+        sh_remove_shadows(d, sh_ctxt->mfn2, 0, 0 /* Slow, can fail. */ );
+
+        mfns[0] = sh_ctxt->mfn1;
+        mfns[1] = sh_ctxt->mfn2;
+        map = vmap(mfns, 2);
+        if ( !map )
+            return MAPPING_UNHANDLEABLE;
+        map += (vaddr & ~PAGE_MASK);
+    }
+
+#if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
+    /*
+     * Remember if the bottom bit was clear, so we can choose not to run
+     * the change through the verify code if it's still clear afterwards.
+     */
+    sh_ctxt->low_bit_was_clear = map != NULL && !(*(u8 *)map & _PAGE_PRESENT);
+#endif
+
+    return map;
+}
+
+/*
+ * Tidy up after the emulated write: mark pages dirty, verify the new
+ * contents, and undo the mapping.
+ */
+void sh_emulate_unmap_dest(struct vcpu *v, void *addr, unsigned int bytes,
+                           struct sh_emulate_ctxt *sh_ctxt)
+{
+    u32 b1 = bytes, b2 = 0, shflags;
+
+    /*
+     * We can avoid re-verifying the page contents after the write if:
+     *  - it was no larger than the PTE type of this pagetable;
+     *  - it was aligned to the PTE boundaries; and
+     *  - _PAGE_PRESENT was clear before and after the write.
+     */
+    shflags = mfn_to_page(sh_ctxt->mfn1)->shadow_flags;
+#if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
+    if ( sh_ctxt->low_bit_was_clear
+         && !(*(u8 *)addr & _PAGE_PRESENT)
+         && ((!(shflags & SHF_32)
+              /*
+               * Not shadowed 32-bit: aligned 64-bit writes that leave
+               * the present bit unset are safe to ignore.
+               */
+              && ((unsigned long)addr & 7) == 0
+              && bytes <= 8)
+             ||
+             (!(shflags & (SHF_PAE|SHF_64))
+              /*
+               * Not shadowed PAE/64-bit: aligned 32-bit writes that
+               * leave the present bit unset are safe to ignore.
+               */
+              && ((unsigned long)addr & 3) == 0
+              && bytes <= 4)) )
+    {
+        /* Writes with this alignment constraint can't possibly cross pages. */
+        ASSERT(!mfn_valid(sh_ctxt->mfn2));
+    }
+    else
+#endif /* SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY */
+    {
+        if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
+        {
+            /* Validate as two writes, one to each page. */
+            b1 = PAGE_SIZE - (((unsigned long)addr) & ~PAGE_MASK);
+            b2 = bytes - b1;
+            ASSERT(b2 < bytes);
+        }
+        if ( likely(b1 > 0) )
+            sh_validate_guest_pt_write(v, sh_ctxt->mfn1, addr, b1);
+        if ( unlikely(b2 > 0) )
+            sh_validate_guest_pt_write(v, sh_ctxt->mfn2, addr + b1, b2);
+    }
+
+    paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn1));
+
+    if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
+    {
+        paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn2));
+        vunmap((void *)((unsigned long)addr & PAGE_MASK));
+    }
+    else
+        unmap_domain_page(addr);
+
+    atomic_inc(&v->domain->arch.paging.shadow.gtable_dirty_version);
+}
+
+/**************************************************************************/
 /* Hash table for storing the guest->shadow mappings.
  * The table itself is an array of pointers to shadows; the shadows are then
  * threaded on a singly-linked list of shadows with the same hash value */
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4606,141 +4606,7 @@ static void sh_pagetable_dying(struct vc
 #endif
 
 /**************************************************************************/
-/* Handling HVM guest writes to pagetables  */
-
-/* Translate a VA to an MFN, injecting a page-fault if we fail */
-#define BAD_GVA_TO_GFN (~0UL)
-#define BAD_GFN_TO_MFN (~1UL)
-#define READONLY_GFN   (~2UL)
-static mfn_t emulate_gva_to_mfn(struct vcpu *v,
-                                unsigned long vaddr,
-                                struct sh_emulate_ctxt *sh_ctxt)
-{
-    unsigned long gfn;
-    struct page_info *page;
-    mfn_t mfn;
-    p2m_type_t p2mt;
-    uint32_t pfec = PFEC_page_present | PFEC_write_access;
-
-    /* Translate the VA to a GFN */
-    gfn = sh_gva_to_gfn(v, NULL, vaddr, &pfec);
-    if ( gfn == INVALID_GFN )
-    {
-        if ( is_hvm_vcpu(v) )
-            hvm_inject_page_fault(pfec, vaddr);
-        else
-            propagate_page_fault(vaddr, pfec);
-        return _mfn(BAD_GVA_TO_GFN);
-    }
-
-    /* Translate the GFN to an MFN */
-    ASSERT(!paging_locked_by_me(v->domain));
-
-    page = get_page_from_gfn(v->domain, gfn, &p2mt, P2M_ALLOC);
-
-    /* Sanity checking */
-    if ( page == NULL )
-    {
-        return _mfn(BAD_GFN_TO_MFN);
-    }
-    if ( p2m_is_discard_write(p2mt) )
-    {
-        put_page(page);
-        return _mfn(READONLY_GFN);
-    }
-    if ( !p2m_is_ram(p2mt) )
-    {
-        put_page(page);
-        return _mfn(BAD_GFN_TO_MFN);
-    }
-    mfn = page_to_mfn(page);
-    ASSERT(mfn_valid(mfn));
-
-    v->arch.paging.last_write_was_pt = !!sh_mfn_is_a_page_table(mfn);
-    /* Note shadow cannot page out or unshare this mfn, so the map won't
-     * disappear. Otherwise, caller must hold onto page until done. */
-    put_page(page);
-    return mfn;
-}
-
-/* Check that the user is allowed to perform this write.
- * Returns a mapped pointer to write to, or NULL for error. */
-#define MAPPING_UNHANDLEABLE ((void *)(unsigned long)X86EMUL_UNHANDLEABLE)
-#define MAPPING_EXCEPTION    ((void *)(unsigned long)X86EMUL_EXCEPTION)
-#define MAPPING_SILENT_FAIL  ((void *)(unsigned long)X86EMUL_OKAY)
-#define emulate_map_dest_failed(rc) ((unsigned long)(rc) <= 3)
-static void *emulate_map_dest(struct vcpu *v,
-                              unsigned long vaddr,
-                              u32 bytes,
-                              struct sh_emulate_ctxt *sh_ctxt)
-{
-    struct domain *d = v->domain;
-    void *map = NULL;
-
-    sh_ctxt->mfn1 = emulate_gva_to_mfn(v, vaddr, sh_ctxt);
-    if ( !mfn_valid(sh_ctxt->mfn1) )
-        return ((mfn_x(sh_ctxt->mfn1) == BAD_GVA_TO_GFN) ?
-                MAPPING_EXCEPTION :
-                (mfn_x(sh_ctxt->mfn1) == READONLY_GFN) ?
-                MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
-
-#ifndef NDEBUG
-    /* We don't emulate user-mode writes to page tables */
-    if ( hvm_get_seg_reg(x86_seg_ss, sh_ctxt)->attr.fields.dpl == 3 )
-    {
-        gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached "
-                 "emulate_map_dest(). This should never happen!\n");
-        return MAPPING_UNHANDLEABLE;
-    }
-#endif
-
-    /* Unaligned writes mean probably this isn't a pagetable */
-    if ( vaddr & (bytes - 1) )
-        sh_remove_shadows(d, sh_ctxt->mfn1, 0, 0 /* Slow, can fail */ );
-
-    if ( likely(((vaddr + bytes - 1) & PAGE_MASK) == (vaddr & PAGE_MASK)) )
-    {
-        /* Whole write fits on a single page */
-        sh_ctxt->mfn2 = _mfn(INVALID_MFN);
-        map = map_domain_page(sh_ctxt->mfn1) + (vaddr & ~PAGE_MASK);
-    }
-    else
-    {
-        mfn_t mfns[2];
-
-        /* Cross-page emulated writes are only supported for HVM guests;
-         * PV guests ought to know better */
-        if ( !is_hvm_domain(d) )
-            return MAPPING_UNHANDLEABLE;
-
-        /* This write crosses a page boundary.  Translate the second page */
-        sh_ctxt->mfn2 = emulate_gva_to_mfn(v, (vaddr + bytes - 1) & PAGE_MASK,
-                                           sh_ctxt);
-        if ( !mfn_valid(sh_ctxt->mfn2) )
-            return ((mfn_x(sh_ctxt->mfn2) == BAD_GVA_TO_GFN) ?
-                    MAPPING_EXCEPTION :
-                    (mfn_x(sh_ctxt->mfn2) == READONLY_GFN) ?
-                    MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
-
-        /* Cross-page writes mean probably not a pagetable */
-        sh_remove_shadows(d, sh_ctxt->mfn2, 0, 0 /* Slow, can fail */ );
-
-        mfns[0] = sh_ctxt->mfn1;
-        mfns[1] = sh_ctxt->mfn2;
-        map = vmap(mfns, 2);
-        if ( !map )
-            return MAPPING_UNHANDLEABLE;
-        map += (vaddr & ~PAGE_MASK);
-    }
-
-#if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
-    /* Remember if the bottom bit was clear, so we can choose not to run
-     * the change through the verify code if it's still clear afterwards */
-    sh_ctxt->low_bit_was_clear = map != NULL && !(*(u8 *)map & _PAGE_PRESENT);
-#endif
-
-    return map;
-}
+/* Handling guest writes to pagetables. */
 
 /* Tidy up after the emulated write: mark pages dirty, verify the new
  * contents, and undo the mapping */
@@ -4749,8 +4615,6 @@ static void emulate_unmap_dest(struct vc
                                u32 bytes,
                                struct sh_emulate_ctxt *sh_ctxt)
 {
-    u32 b1 = bytes, b2 = 0, shflags;
-
     ASSERT(mfn_valid(sh_ctxt->mfn1));
 
     /* If we are writing lots of PTE-aligned zeros, might want to unshadow */
@@ -4764,56 +4628,7 @@ static void emulate_unmap_dest(struct vc
     else
         reset_early_unshadow(v);
 
-    /* We can avoid re-verifying the page contents after the write if:
-     *  - it was no larger than the PTE type of this pagetable;
-     *  - it was aligned to the PTE boundaries; and
-     *  - _PAGE_PRESENT was clear before and after the write. */
-    shflags = mfn_to_page(sh_ctxt->mfn1)->shadow_flags;
-#if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
-    if ( sh_ctxt->low_bit_was_clear
-         && !(*(u8 *)addr & _PAGE_PRESENT)
-         && ((!(shflags & SHF_32)
-              /* Not shadowed 32-bit: aligned 64-bit writes that leave
-               * the present bit unset are safe to ignore. */
-              && ((unsigned long)addr & 7) == 0
-              && bytes <= 8)
-             ||
-             (!(shflags & (SHF_PAE|SHF_64))
-              /* Not shadowed PAE/64-bit: aligned 32-bit writes that
-               * leave the present bit unset are safe to ignore. */
-              && ((unsigned long)addr & 3) == 0
-              && bytes <= 4)) )
-    {
-        /* Writes with this alignment constraint can't possibly cross pages */
-        ASSERT(!mfn_valid(sh_ctxt->mfn2));
-    }
-    else
-#endif /* SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY */
-    {
-        if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
-        {
-            /* Validate as two writes, one to each page */
-            b1 = PAGE_SIZE - (((unsigned long)addr) & ~PAGE_MASK);
-            b2 = bytes - b1;
-            ASSERT(b2 < bytes);
-        }
-        if ( likely(b1 > 0) )
-            sh_validate_guest_pt_write(v, sh_ctxt->mfn1, addr, b1);
-        if ( unlikely(b2 > 0) )
-            sh_validate_guest_pt_write(v, sh_ctxt->mfn2, addr + b1, b2);
-    }
-
-    paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn1));
-
-    if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
-    {
-        paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn2));
-        vunmap((void *)((unsigned long)addr & PAGE_MASK));
-    }
-    else
-        unmap_domain_page(addr);
-
-    atomic_inc(&v->domain->arch.paging.shadow.gtable_dirty_version);
+    sh_emulate_unmap_dest(v, addr, bytes, sh_ctxt);
 }
 
 static int
@@ -4826,8 +4641,8 @@ sh_x86_emulate_write(struct vcpu *v, uns
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
-    addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
-    if ( emulate_map_dest_failed(addr) )
+    addr = sh_emulate_map_dest(v, vaddr, bytes, sh_ctxt);
+    if ( sh_emulate_map_dest_failed(addr) )
         return (long)addr;
 
     paging_lock(v->domain);
@@ -4868,8 +4683,8 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
-    addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
-    if ( emulate_map_dest_failed(addr) )
+    addr = sh_emulate_map_dest(v, vaddr, bytes, sh_ctxt);
+    if ( sh_emulate_map_dest_failed(addr) )
         return (long)addr;
 
     paging_lock(v->domain);
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -393,6 +393,17 @@ void shadow_update_paging_modes(struct v
  * With user_only == 1, unhooks only the user-mode mappings. */
 void shadow_unhook_mappings(struct domain *d, mfn_t smfn, int user_only);
 
+/* Returns a mapped pointer to write to, or one of the following error
+ * indicators. */
+#define MAPPING_UNHANDLEABLE ((void *)(unsigned long)X86EMUL_UNHANDLEABLE)
+#define MAPPING_EXCEPTION    ((void *)(unsigned long)X86EMUL_EXCEPTION)
+#define MAPPING_SILENT_FAIL  ((void *)(unsigned long)X86EMUL_OKAY)
+#define sh_emulate_map_dest_failed(rc) ((unsigned long)(rc) <= 3)
+void *sh_emulate_map_dest(struct vcpu *v, unsigned long vaddr,
+                          unsigned int bytes, struct sh_emulate_ctxt *sh_ctxt);
+void sh_emulate_unmap_dest(struct vcpu *v, void *addr, unsigned int bytes,
+                           struct sh_emulate_ctxt *sh_ctxt);
+
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
 /* Allow a shadowed page to go out of sync */
 int sh_unsync(struct vcpu *v, mfn_t gmfn);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/2] x86/shadow: compile most write emulation code just once

[Andrew Cooper]

On 10/03/16 10:13, Jan Beulich wrote:
> No need to compile all of this code three times, as most of it really
> is guest mode independent. The savings are between 3k and 4k of binary
> code in my builds.
>
> No functional change (i.e. only formatting and naming changes) except
> for
> - sh_emulate_map_dest()'s user mode check corrected for the PV case
>   (affecting debugging mode only, this isn't being split out)
> - simplifying the vaddr argument to emulate_gva_to_mfn() for the second
>   part in the cross page write case
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 1/2] x86/shadow: compile most write emulation code just once

[Jan Beulich]

>>> On 10.03.16 at 13:17, <tim@xen.org> wrote:
> Hi,
> 
> At 03:13 -0700 on 10 Mar (1457579586), Jan Beulich wrote:
>> No need to compile all of this code three times, as most of it really
>> is guest mode independent. The savings are between 3k and 4k of binary
>> code in my builds.
> 
> Thanks for this.  This would have been _much_ easier to review as (at
> least) two patches, with one being motion-only.  As it is I had to
> effectively diff by eye to see the changes.

Well, a precisely code motion only patch wouldn't have worked -
some changes were unavoidable while moving, for the build to not
break.

>> No functional change (i.e. only formatting and naming changes)
> 
> There is one other change, that emulate_gva_to_mfn()'s call to
> gva_to_gfn() now indirects through the paging mode table.  I think
> that's fine, but it's not obvious from this description.

That's not a functional change (as it still results in the same function
getting called), but I've added a sentence to the earlier part of
description.

>> except
>> for
>> - sh_emulate_map_dest()'s user mode check corrected for the PV case
>>   (affecting debugging mode only, this isn't being split out)
>> - simplifying the vaddr argument to emulate_gva_to_mfn() for the second
>>   part in the cross page write case
> 
> Removing the mask with PAGE_MASK is fine, but please keep the '- 1' in
> calculating the final byte.  It is more clearly correct and can easily
> be folded with the earlier calculations.  If you want to make that
> change on committing, then
> 
> Reviewed-by: Tim Deegan <tim@xen.org>

Thanks. It didn't seem questionable to me that the code would be
correct with the "- 1" dropped, but I've added it back.

Jan


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

[PATCH 2/2] x86/shadow: avoid extra local array variable

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 6445 bytes --]

mfns[2] was there just because struct sh_emulate_ctxt's two MFN values
can't be used to hand to vmap(). Making the structure fields an array
avoids the extra copying.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1746,11 +1746,11 @@ void *sh_emulate_map_dest(struct vcpu *v
     struct domain *d = v->domain;
     void *map;
 
-    sh_ctxt->mfn1 = emulate_gva_to_mfn(v, vaddr, sh_ctxt);
-    if ( !mfn_valid(sh_ctxt->mfn1) )
-        return ((mfn_x(sh_ctxt->mfn1) == BAD_GVA_TO_GFN) ?
+    sh_ctxt->mfn[0] = emulate_gva_to_mfn(v, vaddr, sh_ctxt);
+    if ( !mfn_valid(sh_ctxt->mfn[0]) )
+        return ((mfn_x(sh_ctxt->mfn[0]) == BAD_GVA_TO_GFN) ?
                 MAPPING_EXCEPTION :
-                (mfn_x(sh_ctxt->mfn1) == READONLY_GFN) ?
+                (mfn_x(sh_ctxt->mfn[0]) == READONLY_GFN) ?
                 MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
 
 #ifndef NDEBUG
@@ -1767,39 +1767,36 @@ void *sh_emulate_map_dest(struct vcpu *v
 
     /* Unaligned writes mean probably this isn't a pagetable. */
     if ( vaddr & (bytes - 1) )
-        sh_remove_shadows(d, sh_ctxt->mfn1, 0, 0 /* Slow, can fail. */ );
+        sh_remove_shadows(d, sh_ctxt->mfn[0], 0, 0 /* Slow, can fail. */ );
 
     if ( likely(((vaddr + bytes - 1) & PAGE_MASK) == (vaddr & PAGE_MASK)) )
     {
         /* Whole write fits on a single page. */
-        sh_ctxt->mfn2 = _mfn(INVALID_MFN);
-        map = map_domain_page(sh_ctxt->mfn1) + (vaddr & ~PAGE_MASK);
+        sh_ctxt->mfn[1] = _mfn(INVALID_MFN);
+        map = map_domain_page(sh_ctxt->mfn[0]) + (vaddr & ~PAGE_MASK);
     }
-    else
+    else if ( !is_hvm_domain(d) )
     {
-        mfn_t mfns[2];
-
         /*
          * Cross-page emulated writes are only supported for HVM guests;
          * PV guests ought to know better.
          */
-        if ( !is_hvm_domain(d) )
-            return MAPPING_UNHANDLEABLE;
-
+        return MAPPING_UNHANDLEABLE;
+    }
+    else
+    {
         /* This write crosses a page boundary. Translate the second page. */
-        sh_ctxt->mfn2 = emulate_gva_to_mfn(v, vaddr + bytes, sh_ctxt);
-        if ( !mfn_valid(sh_ctxt->mfn2) )
-            return ((mfn_x(sh_ctxt->mfn2) == BAD_GVA_TO_GFN) ?
+        sh_ctxt->mfn[1] = emulate_gva_to_mfn(v, vaddr + bytes, sh_ctxt);
+        if ( !mfn_valid(sh_ctxt->mfn[1]) )
+            return ((mfn_x(sh_ctxt->mfn[1]) == BAD_GVA_TO_GFN) ?
                     MAPPING_EXCEPTION :
-                    (mfn_x(sh_ctxt->mfn2) == READONLY_GFN) ?
+                    (mfn_x(sh_ctxt->mfn[1]) == READONLY_GFN) ?
                     MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
 
         /* Cross-page writes mean probably not a pagetable. */
-        sh_remove_shadows(d, sh_ctxt->mfn2, 0, 0 /* Slow, can fail. */ );
+        sh_remove_shadows(d, sh_ctxt->mfn[1], 0, 0 /* Slow, can fail. */ );
 
-        mfns[0] = sh_ctxt->mfn1;
-        mfns[1] = sh_ctxt->mfn2;
-        map = vmap(mfns, 2);
+        map = vmap(sh_ctxt->mfn, 2);
         if ( !map )
             return MAPPING_UNHANDLEABLE;
         map += (vaddr & ~PAGE_MASK);
@@ -1831,7 +1828,7 @@ void sh_emulate_unmap_dest(struct vcpu *
      *  - it was aligned to the PTE boundaries; and
      *  - _PAGE_PRESENT was clear before and after the write.
      */
-    shflags = mfn_to_page(sh_ctxt->mfn1)->shadow_flags;
+    shflags = mfn_to_page(sh_ctxt->mfn[0])->shadow_flags;
 #if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
     if ( sh_ctxt->low_bit_was_clear
          && !(*(u8 *)addr & _PAGE_PRESENT)
@@ -1852,12 +1849,12 @@ void sh_emulate_unmap_dest(struct vcpu *
               && bytes <= 4)) )
     {
         /* Writes with this alignment constraint can't possibly cross pages. */
-        ASSERT(!mfn_valid(sh_ctxt->mfn2));
+        ASSERT(!mfn_valid(sh_ctxt->mfn[1]));
     }
     else
 #endif /* SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY */
     {
-        if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
+        if ( unlikely(mfn_valid(sh_ctxt->mfn[1])) )
         {
             /* Validate as two writes, one to each page. */
             b1 = PAGE_SIZE - (((unsigned long)addr) & ~PAGE_MASK);
@@ -1865,16 +1862,16 @@ void sh_emulate_unmap_dest(struct vcpu *
             ASSERT(b2 < bytes);
         }
         if ( likely(b1 > 0) )
-            sh_validate_guest_pt_write(v, sh_ctxt->mfn1, addr, b1);
+            sh_validate_guest_pt_write(v, sh_ctxt->mfn[0], addr, b1);
         if ( unlikely(b2 > 0) )
-            sh_validate_guest_pt_write(v, sh_ctxt->mfn2, addr + b1, b2);
+            sh_validate_guest_pt_write(v, sh_ctxt->mfn[1], addr + b1, b2);
     }
 
-    paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn1));
+    paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn[0]));
 
-    if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
+    if ( unlikely(mfn_valid(sh_ctxt->mfn[1])) )
     {
-        paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn2));
+        paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn[1]));
         vunmap((void *)((unsigned long)addr & PAGE_MASK));
     }
     else
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4615,13 +4615,13 @@ static void emulate_unmap_dest(struct vc
                                u32 bytes,
                                struct sh_emulate_ctxt *sh_ctxt)
 {
-    ASSERT(mfn_valid(sh_ctxt->mfn1));
+    ASSERT(mfn_valid(sh_ctxt->mfn[0]));
 
     /* If we are writing lots of PTE-aligned zeros, might want to unshadow */
     if ( likely(bytes >= 4) && (*(u32 *)addr == 0) )
     {
         if ( ((unsigned long) addr & ((sizeof (guest_intpte_t)) - 1)) == 0 )
-            check_for_early_unshadow(v, sh_ctxt->mfn1);
+            check_for_early_unshadow(v, sh_ctxt->mfn[0]);
         /* Don't reset the heuristic if we're writing zeros at non-aligned
          * addresses, otherwise it doesn't catch REP MOVSD on PAE guests */
     }
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -727,7 +727,7 @@ struct sh_emulate_ctxt {
     struct segment_register seg_reg[6];
 
     /* MFNs being written to in write/cmpxchg callbacks */
-    mfn_t mfn1, mfn2;
+    mfn_t mfn[2];
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
     /* Special case for avoiding having to verify writes: remember



[-- Attachment #2: x86-sh-mfn-array.patch --]
[-- Type: text/plain, Size: 6489 bytes --]

x86/shadow: avoid extra local array variable

mfns[2] was there just because struct sh_emulate_ctxt's two MFN values
can't be used to hand to vmap(). Making the structure fields an array
avoids the extra copying.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1746,11 +1746,11 @@ void *sh_emulate_map_dest(struct vcpu *v
     struct domain *d = v->domain;
     void *map;
 
-    sh_ctxt->mfn1 = emulate_gva_to_mfn(v, vaddr, sh_ctxt);
-    if ( !mfn_valid(sh_ctxt->mfn1) )
-        return ((mfn_x(sh_ctxt->mfn1) == BAD_GVA_TO_GFN) ?
+    sh_ctxt->mfn[0] = emulate_gva_to_mfn(v, vaddr, sh_ctxt);
+    if ( !mfn_valid(sh_ctxt->mfn[0]) )
+        return ((mfn_x(sh_ctxt->mfn[0]) == BAD_GVA_TO_GFN) ?
                 MAPPING_EXCEPTION :
-                (mfn_x(sh_ctxt->mfn1) == READONLY_GFN) ?
+                (mfn_x(sh_ctxt->mfn[0]) == READONLY_GFN) ?
                 MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
 
 #ifndef NDEBUG
@@ -1767,39 +1767,36 @@ void *sh_emulate_map_dest(struct vcpu *v
 
     /* Unaligned writes mean probably this isn't a pagetable. */
     if ( vaddr & (bytes - 1) )
-        sh_remove_shadows(d, sh_ctxt->mfn1, 0, 0 /* Slow, can fail. */ );
+        sh_remove_shadows(d, sh_ctxt->mfn[0], 0, 0 /* Slow, can fail. */ );
 
     if ( likely(((vaddr + bytes - 1) & PAGE_MASK) == (vaddr & PAGE_MASK)) )
     {
         /* Whole write fits on a single page. */
-        sh_ctxt->mfn2 = _mfn(INVALID_MFN);
-        map = map_domain_page(sh_ctxt->mfn1) + (vaddr & ~PAGE_MASK);
+        sh_ctxt->mfn[1] = _mfn(INVALID_MFN);
+        map = map_domain_page(sh_ctxt->mfn[0]) + (vaddr & ~PAGE_MASK);
     }
-    else
+    else if ( !is_hvm_domain(d) )
     {
-        mfn_t mfns[2];
-
         /*
          * Cross-page emulated writes are only supported for HVM guests;
          * PV guests ought to know better.
          */
-        if ( !is_hvm_domain(d) )
-            return MAPPING_UNHANDLEABLE;
-
+        return MAPPING_UNHANDLEABLE;
+    }
+    else
+    {
         /* This write crosses a page boundary. Translate the second page. */
-        sh_ctxt->mfn2 = emulate_gva_to_mfn(v, vaddr + bytes, sh_ctxt);
-        if ( !mfn_valid(sh_ctxt->mfn2) )
-            return ((mfn_x(sh_ctxt->mfn2) == BAD_GVA_TO_GFN) ?
+        sh_ctxt->mfn[1] = emulate_gva_to_mfn(v, vaddr + bytes, sh_ctxt);
+        if ( !mfn_valid(sh_ctxt->mfn[1]) )
+            return ((mfn_x(sh_ctxt->mfn[1]) == BAD_GVA_TO_GFN) ?
                     MAPPING_EXCEPTION :
-                    (mfn_x(sh_ctxt->mfn2) == READONLY_GFN) ?
+                    (mfn_x(sh_ctxt->mfn[1]) == READONLY_GFN) ?
                     MAPPING_SILENT_FAIL : MAPPING_UNHANDLEABLE);
 
         /* Cross-page writes mean probably not a pagetable. */
-        sh_remove_shadows(d, sh_ctxt->mfn2, 0, 0 /* Slow, can fail. */ );
+        sh_remove_shadows(d, sh_ctxt->mfn[1], 0, 0 /* Slow, can fail. */ );
 
-        mfns[0] = sh_ctxt->mfn1;
-        mfns[1] = sh_ctxt->mfn2;
-        map = vmap(mfns, 2);
+        map = vmap(sh_ctxt->mfn, 2);
         if ( !map )
             return MAPPING_UNHANDLEABLE;
         map += (vaddr & ~PAGE_MASK);
@@ -1831,7 +1828,7 @@ void sh_emulate_unmap_dest(struct vcpu *
      *  - it was aligned to the PTE boundaries; and
      *  - _PAGE_PRESENT was clear before and after the write.
      */
-    shflags = mfn_to_page(sh_ctxt->mfn1)->shadow_flags;
+    shflags = mfn_to_page(sh_ctxt->mfn[0])->shadow_flags;
 #if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
     if ( sh_ctxt->low_bit_was_clear
          && !(*(u8 *)addr & _PAGE_PRESENT)
@@ -1852,12 +1849,12 @@ void sh_emulate_unmap_dest(struct vcpu *
               && bytes <= 4)) )
     {
         /* Writes with this alignment constraint can't possibly cross pages. */
-        ASSERT(!mfn_valid(sh_ctxt->mfn2));
+        ASSERT(!mfn_valid(sh_ctxt->mfn[1]));
     }
     else
 #endif /* SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY */
     {
-        if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
+        if ( unlikely(mfn_valid(sh_ctxt->mfn[1])) )
         {
             /* Validate as two writes, one to each page. */
             b1 = PAGE_SIZE - (((unsigned long)addr) & ~PAGE_MASK);
@@ -1865,16 +1862,16 @@ void sh_emulate_unmap_dest(struct vcpu *
             ASSERT(b2 < bytes);
         }
         if ( likely(b1 > 0) )
-            sh_validate_guest_pt_write(v, sh_ctxt->mfn1, addr, b1);
+            sh_validate_guest_pt_write(v, sh_ctxt->mfn[0], addr, b1);
         if ( unlikely(b2 > 0) )
-            sh_validate_guest_pt_write(v, sh_ctxt->mfn2, addr + b1, b2);
+            sh_validate_guest_pt_write(v, sh_ctxt->mfn[1], addr + b1, b2);
     }
 
-    paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn1));
+    paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn[0]));
 
-    if ( unlikely(mfn_valid(sh_ctxt->mfn2)) )
+    if ( unlikely(mfn_valid(sh_ctxt->mfn[1])) )
     {
-        paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn2));
+        paging_mark_dirty(v->domain, mfn_x(sh_ctxt->mfn[1]));
         vunmap((void *)((unsigned long)addr & PAGE_MASK));
     }
     else
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4615,13 +4615,13 @@ static void emulate_unmap_dest(struct vc
                                u32 bytes,
                                struct sh_emulate_ctxt *sh_ctxt)
 {
-    ASSERT(mfn_valid(sh_ctxt->mfn1));
+    ASSERT(mfn_valid(sh_ctxt->mfn[0]));
 
     /* If we are writing lots of PTE-aligned zeros, might want to unshadow */
     if ( likely(bytes >= 4) && (*(u32 *)addr == 0) )
     {
         if ( ((unsigned long) addr & ((sizeof (guest_intpte_t)) - 1)) == 0 )
-            check_for_early_unshadow(v, sh_ctxt->mfn1);
+            check_for_early_unshadow(v, sh_ctxt->mfn[0]);
         /* Don't reset the heuristic if we're writing zeros at non-aligned
          * addresses, otherwise it doesn't catch REP MOVSD on PAE guests */
     }
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -727,7 +727,7 @@ struct sh_emulate_ctxt {
     struct segment_register seg_reg[6];
 
     /* MFNs being written to in write/cmpxchg callbacks */
-    mfn_t mfn1, mfn2;
+    mfn_t mfn[2];
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
     /* Special case for avoiding having to verify writes: remember

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] x86/shadow: avoid extra local array variable

[Andrew Cooper]

On 10/03/16 10:13, Jan Beulich wrote:
> mfns[2] was there just because struct sh_emulate_ctxt's two MFN values
> can't be used to hand to vmap(). Making the structure fields an array
> avoids the extra copying.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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