[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for-4.5 v10 15/19] xen/arm: Temporarily disable mem_access for hypervisor access



Hello Tamas,

On 25/09/2014 13:56, Tamas K Lengyel wrote:
The guestcopy helpers use the MMU to verify that the given guest has read/write
access to a given page during hypercalls. As we may have custom mem_access
permissions set on these pages, we temporarily disable them to allow Xen to
finish the hypercalls. This is permissible as mem_access events are only
reported for events when the guest directly accesses protected memory on x86
as well.

IHMO, copying data from/to the guest could be consider as a guest access.

How does x86 handle this case?

Signed-off-by: Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx>
---
  xen/arch/arm/guestcopy.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 61 insertions(+)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 0173597..4aa041f 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -6,6 +6,43 @@

  #include <asm/mm.h>
  #include <asm/guest_access.h>
+#include <asm/p2m.h>
+
+/*
+ * Temporarily disable mem_access permission restrictions.
+ * Note: In the future, events generated by the hypervisor accessing
+ * protected memory regions could be added here.
+ */
+static long temp_disable_mem_access(vaddr_t gva, unsigned long *gfn,
+                                    xenmem_access_t *xma)
+{
+    long rc;
+    paddr_t gpa;
+
+    rc = gva_to_ipa((vaddr_t) gva, &gpa);
+    if ( rc < 0 )
+        return rc;
+
+    *gfn = paddr_to_pfn(gpa);
+
+    rc = p2m_get_mem_access(current->domain, *gfn, xma);
+    if ( rc < 0 )
+        return rc;
+
+    if ( *xma != XENMEM_access_rwx )
+        rc = p2m_set_mem_access(current->domain, *gfn, 1, 0, ~0,
+                                XENMEM_access_rwx);
+
+    return rc;
+}

When mem_access is not in use you are adding another translation and therefore slowing down hypercall for everyone.

I don't think that modifying temporary the permission is the right thing to do because: - p2m_set_mem_access is called 2 times which means 2 TLB flush (and I'm not counting the table mapping), ie it's very slow - The other VCPU of the guest are still running. So you may not catch unwanted access.

IHMO, the best solution would be smth like:

     page = get_page_from_gva(...)
     if ( !page )
     {
        check mem access and getting the page
        if ( !page )
          return rc;
     }

     copy the data

Regards,

--
Julien Grall

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.