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

Re: [Xen-devel] [RFC PATCH v2 5/8] arm/mem_access: Add software guest-page-table walk



Hi Sergej,

On 06/01/2017 04:18 PM, Sergej Proskurin wrote:
The function p2m_mem_access_check_and_get_page in mem_access.c translates a gva
to an ipa by means of the hardware functionality of the ARM architecture. This
is implemented in the function gva_to_ipa. If mem_access is active,
hardware-based gva to ipa translation might fail, as gva_to_ipa uses the
guest's translation tables, access to which might be restricted by the active
VTTBR. To address this issue, in this commit we add a software-based
guest-page-table walk, which will be used by the function
p2m_mem_access_check_and_get_page perform the gva to ipa translation in
software in one of the following commits.

Note: This function p2m_walk_gpt assumes that the domain, the gva of
which is to be translated, is running on the currently active vCPU. To
walk the guest's page table on a different vCPU, the following registers
would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.

Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
---
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
---
v2: Rename p2m_gva_to_ipa to p2m_walk_gpt and move it to p2m.c.

     Move the functionality responsible for walking long-descriptor based
     translation tables out of the function p2m_walk_gpt. Also move out the
     long-descriptor based translation out of this commit.

     Change function parameters in order to return access access rights to a
     requested gva.

     Cosmetic fixes.
---
  xen/arch/arm/p2m.c        | 58 +++++++++++++++++++++++++++++++++++++++++++++++

I know I suggested to move in p2m.c. Looking at the diff stat, this will increase quite a lot p2m.c which is already big.

How about introducing a file guest_walk.c which contain the new functions?

  xen/include/asm-arm/p2m.h |  6 +++++
  2 files changed, 64 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 752e948070..0337d83581 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1548,6 +1548,64 @@ void __init setup_virt_paging(void)
  }
/*
+ * The function __p2m_walk_gpt_sd translates a given GVA into an IPA using the
+ * short-descriptor translation table format in software. This function assumes
+ * that the domain is running on the currently active vCPU. To walk the guest's
+ * page table on a different vCPU, the following registers would need to be
+ * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
+ */
+static int __p2m_walk_gpt_sd(struct p2m_domain *p2m,

The __ are not necessary here. You don't use the name p2m_walk_gpt_sd anywhere.

+                             vaddr_t gva, paddr_t *ipa,
+                             unsigned int *perm_ro)

I am a bit confused with perm_ro. Will you only return 0/1? If so it should be a bool.

But we likely want to know more permission such as the execution bit...

+{
+    /* Not implemented yet. */
+    return -EFAULT;
+}
+
+/*
+ * The function __p2m_walk_gpt_ld translates a given GVA into an IPA using the
+ * long-descriptor translation table format in software. This function assumes
+ * that the domain is running on the currently active vCPU. To walk the guest's
+ * page table on a different vCPU, the following registers would need to be
+ * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
+ */
+static int __p2m_walk_gpt_ld(struct p2m_domain *p2m,

Ditto.

+                             vaddr_t gva, paddr_t *ipa,
+                             unsigned int *perm_ro)
+{
+    /* Not implemented yet. */
+    return -EFAULT;
+}
+
+int p2m_walk_gpt(struct p2m_domain *p2m, vaddr_t gva,

So you mix 2 things, p2m and gpt. P2M is used to refer to stage-2 page table. Whilst gpt stands I guess for guest page table.

The name gpt is not very used in Xen and would prefer a clearer name such as the x86 one "guest_walk_tables".

+                 paddr_t *ipa, unsigned int *perm_ro)
+{
+    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
+    register_t tcr = READ_SYSREG(TCR_EL1);
+#ifdef CONFIG_ARM_64
+    struct domain *d = p2m->domain;
+#endif

The only place use *d is in the is_32bit_domain, so no need to add some #ifdef and define the variable.

+
+    /* If the MMU is disabled, there is no need to translate the gva. */
+    if ( !(sctlr & SCTLR_M) )
+    {
+        *ipa = gva;

Why *perm_ro is not set here?

+
+        return 0;
+    }
+
+#ifdef CONFIG_ARM_64
+    if ( is_32bit_domain(d) )
+#endif

is_32bit_domain exists for 32-bit Xen. So not need to have this #ifdef.

+    {
+        if ( !(tcr & TTBCR_EAE) )
+            return __p2m_walk_gpt_sd(p2m, gva, ipa, perm_ro);
+    }
+
+    return __p2m_walk_gpt_ld(p2m, gva, ipa, perm_ro);
+}
+
+/*
   * Local variables:
   * mode: C
   * c-file-style: "BSD"
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 18c57f936e..cc9f4bf225 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -264,6 +264,12 @@ void guest_physmap_remove_page(struct domain *d,
mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); +/* Walk the guest's page tables in software. */
+int p2m_walk_gpt(struct p2m_domain *p2m,
+                 vaddr_t gva,
+                 paddr_t *ipa,
+                 unsigned int *perm_ro);
+
  /*
   * Populate-on-demand
   */


Cheers,

--
Julien Grall

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

 


Rackspace

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