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

2017-06-09 Thread Julien Grall 



On 08/06/2017 13:43, Sergej Proskurin wrote:

Hi Julien,


Hi Sergej,



[...]


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?



No problem at all. I will gladly move the functionality into a separate
file.




+ 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...



Yes, I agree that we should return more permissions back to the caller.
I suggest that we agree on required permissions at this point, as do not
only have the execution bit (!XN) but also we distinguish between the
Privileged XN (PXN) bit in the long-descriptor format, as well as the
access permissions bits (AP[2:x]), which inform which EL/PL is allowed
to access (RWX) the particular memory region.

Or do you think returning an additional execute bit (!XN) would suffice
for now, as we don't really care about execution permissions at
different EL's/PL's at this point?


I think Read/Write/eXecute should be enough for now. We can add 
additional one later on.


My main point here is not about the permission returned but the 
interface. At the moment, you return a boolean-like value and it would 
be tedious to update the callers. What I would like to see is a set of 
flags that we can easily extend. Something like:


#define PERM_READ  (1 << 0)
#define PERM_WRITE (1 << 1)
#define PERM_EXEC  (1 << 2)

We probably want to make them a bit less generic or re-use something 
already existing (I haven't checked what we currently have).





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


Sounds good to me. I have changed the names to guest_walk_(tables|sd|ld).




+ 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.



[...]


+
+#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.



True. The reason for this #ifdef is that since is_32bit_domain(d)
resolves to a (1) on ARMv7, the compiler complained about the fact that
the variable struct domain *d was not used. To resolve this, I can
simply use p2m->domain at this point and remove the local variable
completely. Alternatively, I could use struct domain *d as a function
parameter instead of struct p2m_domain *p2m. I believe it would be
cleaner to provide the domain instead of the p2m as parameter, as we
don't really need the p2m. What would you prefer?


Technically this should be a vCPU as the guest page-table may be 
different on each vCPU. So I would prefer to use a vCPU here.


Also, it would allow us to make sure the function has been called with 
vCPU == current (i.e an ASSERT).


Cheers,

--
Julien Grall

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

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

2017-06-08 Thread Sergej Proskurin 
Hi Julien,

[...]

> 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?
>

No problem at all. I will gladly move the functionality into a separate
file.

>
>> + 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...
>

Yes, I agree that we should return more permissions back to the caller.
I suggest that we agree on required permissions at this point, as do not
only have the execution bit (!XN) but also we distinguish between the
Privileged XN (PXN) bit in the long-descriptor format, as well as the
access permissions bits (AP[2:x]), which inform which EL/PL is allowed
to access (RWX) the particular memory region.

Or do you think returning an additional execute bit (!XN) would suffice
for now, as we don't really care about execution permissions at
different EL's/PL's at this point?

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

Sounds good to me. I have changed the names to guest_walk_(tables|sd|ld).

>
>> + 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.
>

[...]

>> +
>> +#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.
>

True. The reason for this #ifdef is that since is_32bit_domain(d)
resolves to a (1) on ARMv7, the compiler complained about the fact that
the variable struct domain *d was not used. To resolve this, I can
simply use p2m->domain at this point and remove the local variable
completely. Alternatively, I could use struct domain *d as a function
parameter instead of struct p2m_domain *p2m. I believe it would be
cleaner to provide the domain instead of the p2m as parameter, as we
don't really need the p2m. What would you prefer?

Cheers,
~Sergej


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

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

2017-06-02 Thread Julien Grall 

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 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
---
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

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

2017-06-01 Thread Sergej Proskurin 
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 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
---
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 +++
 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,
+ vaddr_t gva, paddr_t *ipa,
+ unsigned int *perm_ro)
+{
+/* 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,
+ 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,
+ 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
+
+/* If the MMU is disabled, there is no need to translate the gva. */
+if ( !(sctlr & SCTLR_M) )
+{
+*ipa = gva;
+
+return 0;
+}
+
+#ifdef CONFIG_ARM_64
+if ( is_32bit_domain(d) )
+#endif
+{
+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
  */
-- 
2.12.2


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