diff mbox series

[1/3] arm/arm64/xen: use C inlines for privcmd_call

Message ID 20191127184453.229321-2-pasha.tatashin@soleen.com (mailing list archive)
State Superseded
Headers show
Series Use C inlines for uaccess | expand

Commit Message

Pasha Tatashin Nov. 27, 2019, 6:44 p.m. UTC 
privcmd_call requires to enable access to userspace for the
duration of the hypercall.

Currently, this is done via assembly macros. Change it to C
inlines instead.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
 arch/arm/include/asm/assembler.h       |  2 +-
 arch/arm/include/asm/xen/hypercall.h   | 10 +++++++++
 arch/arm/xen/enlighten.c               |  2 +-
 arch/arm/xen/hypercall.S               |  4 ++--
 arch/arm64/include/asm/xen/hypercall.h | 28 ++++++++++++++++++++++++++
 arch/arm64/xen/hypercall.S             | 19 ++---------------
 include/xen/arm/hypercall.h            | 12 +++++------
 7 files changed, 50 insertions(+), 27 deletions(-)

Comments

Julien Grall Nov. 29, 2019, 3:05 p.m. UTC | #1 
Hi,

On 27/11/2019 18:44, Pavel Tatashin wrote:
> diff --git a/arch/arm64/include/asm/xen/hypercall.h b/arch/arm64/include/asm/xen/hypercall.h
> index 3522cbaed316..1a74fb28607f 100644
> --- a/arch/arm64/include/asm/xen/hypercall.h
> +++ b/arch/arm64/include/asm/xen/hypercall.h
> @@ -1 +1,29 @@
> +#ifndef _ASM_ARM64_XEN_HYPERCALL_H
> +#define _ASM_ARM64_XEN_HYPERCALL_H
>   #include <xen/arm/hypercall.h>
> +#include <linux/uaccess.h>
> +
> +static inline long privcmd_call(unsigned int call, unsigned long a1,
> +				unsigned long a2, unsigned long a3,
> +				unsigned long a4, unsigned long a5)

I realize that privcmd_call is the only hypercall using Software PAN at 
the moment. However, dm_op needs the same as hypercall will be issued 
from userspace as well.

So I was wondering whether we should create a generic function (e.g. 
do_xen_hypercall() or do_xen_user_hypercall()) to cover the two hypercalls?

> diff --git a/include/xen/arm/hypercall.h b/include/xen/arm/hypercall.h
> index b40485e54d80..624c8ad7e42a 100644
> --- a/include/xen/arm/hypercall.h
> +++ b/include/xen/arm/hypercall.h
> @@ -30,8 +30,8 @@
>    * IN THE SOFTWARE.
>    */
>   
> -#ifndef _ASM_ARM_XEN_HYPERCALL_H
> -#define _ASM_ARM_XEN_HYPERCALL_H
> +#ifndef _ARM_XEN_HYPERCALL_H
> +#define _ARM_XEN_HYPERCALL_H

This change feels a bit out of context. Could you split it in a separate 
patch?

>   
>   #include <linux/bug.h>
>   
> @@ -41,9 +41,9 @@
>   
>   struct xen_dm_op_buf;
>   
> -long privcmd_call(unsigned call, unsigned long a1,
> -		unsigned long a2, unsigned long a3,
> -		unsigned long a4, unsigned long a5);
> +long arch_privcmd_call(unsigned int call, unsigned long a1,
> +		       unsigned long a2, unsigned long a3,
> +		       unsigned long a4, unsigned long a5);
>   int HYPERVISOR_xen_version(int cmd, void *arg);
>   int HYPERVISOR_console_io(int cmd, int count, char *str);
>   int HYPERVISOR_grant_table_op(unsigned int cmd, void *uop, unsigned int count);
> @@ -88,4 +88,4 @@ MULTI_mmu_update(struct multicall_entry *mcl, struct mmu_update *req,
>   	BUG();
>   }
>   
> -#endif /* _ASM_ARM_XEN_HYPERCALL_H */
> +#endif /* _ARM_XEN_HYPERCALL_H */
> 

Cheers,
Andrew Cooper Nov. 29, 2019, 3:09 p.m. UTC | #2 
On 29/11/2019 15:05, Julien Grall wrote:
> Hi,
>
> On 27/11/2019 18:44, Pavel Tatashin wrote:
>> diff --git a/arch/arm64/include/asm/xen/hypercall.h
>> b/arch/arm64/include/asm/xen/hypercall.h
>> index 3522cbaed316..1a74fb28607f 100644
>> --- a/arch/arm64/include/asm/xen/hypercall.h
>> +++ b/arch/arm64/include/asm/xen/hypercall.h
>> @@ -1 +1,29 @@
>> +#ifndef _ASM_ARM64_XEN_HYPERCALL_H
>> +#define _ASM_ARM64_XEN_HYPERCALL_H
>>   #include <xen/arm/hypercall.h>
>> +#include <linux/uaccess.h>
>> +
>> +static inline long privcmd_call(unsigned int call, unsigned long a1,
>> +                unsigned long a2, unsigned long a3,
>> +                unsigned long a4, unsigned long a5)
>
> I realize that privcmd_call is the only hypercall using Software PAN
> at the moment. However, dm_op needs the same as hypercall will be
> issued from userspace as well.

And dm_op() won't be the only example as we continue in cleaning up the
gaping hole that is privcmd.

> So I was wondering whether we should create a generic function (e.g.
> do_xen_hypercall() or do_xen_user_hypercall()) to cover the two
> hypercalls?

Probably a good idea.

~Andrew
Pasha Tatashin Dec. 4, 2019, 5:55 p.m. UTC | #3 
On Fri, Nov 29, 2019 at 10:10 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 29/11/2019 15:05, Julien Grall wrote:
> > Hi,
> >
> > On 27/11/2019 18:44, Pavel Tatashin wrote:
> >> diff --git a/arch/arm64/include/asm/xen/hypercall.h
> >> b/arch/arm64/include/asm/xen/hypercall.h
> >> index 3522cbaed316..1a74fb28607f 100644
> >> --- a/arch/arm64/include/asm/xen/hypercall.h
> >> +++ b/arch/arm64/include/asm/xen/hypercall.h
> >> @@ -1 +1,29 @@
> >> +#ifndef _ASM_ARM64_XEN_HYPERCALL_H
> >> +#define _ASM_ARM64_XEN_HYPERCALL_H
> >>   #include <xen/arm/hypercall.h>
> >> +#include <linux/uaccess.h>
> >> +
> >> +static inline long privcmd_call(unsigned int call, unsigned long a1,
> >> +                unsigned long a2, unsigned long a3,
> >> +                unsigned long a4, unsigned long a5)
> >
> > I realize that privcmd_call is the only hypercall using Software PAN
> > at the moment. However, dm_op needs the same as hypercall will be
> > issued from userspace as well.
>
> And dm_op() won't be the only example as we continue in cleaning up the
> gaping hole that is privcmd.
>
> > So I was wondering whether we should create a generic function (e.g.
> > do_xen_hypercall() or do_xen_user_hypercall()) to cover the two
> > hypercalls?
>
> Probably a good idea.

It sounds good to me, but let's keep it outside of this series.

Thank you,
Pasha
Pasha Tatashin Dec. 4, 2019, 5:58 p.m. UTC | #4 
On Fri, Nov 29, 2019 at 10:05 AM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 27/11/2019 18:44, Pavel Tatashin wrote:
> > diff --git a/arch/arm64/include/asm/xen/hypercall.h b/arch/arm64/include/asm/xen/hypercall.h
> > index 3522cbaed316..1a74fb28607f 100644
> > --- a/arch/arm64/include/asm/xen/hypercall.h
> > +++ b/arch/arm64/include/asm/xen/hypercall.h
> > @@ -1 +1,29 @@
> > +#ifndef _ASM_ARM64_XEN_HYPERCALL_H
> > +#define _ASM_ARM64_XEN_HYPERCALL_H
> >   #include <xen/arm/hypercall.h>
> > +#include <linux/uaccess.h>
> > +
> > +static inline long privcmd_call(unsigned int call, unsigned long a1,
> > +                             unsigned long a2, unsigned long a3,
> > +                             unsigned long a4, unsigned long a5)
>
> I realize that privcmd_call is the only hypercall using Software PAN at
> the moment. However, dm_op needs the same as hypercall will be issued
> from userspace as well.

The clean-up I am working on now is specific to moving current PAN
useage to C wraps. Once dm_op requires to use PAN it will need to be
used the C variants, because ASM versions are going to be removed by
this series.

>
> So I was wondering whether we should create a generic function (e.g.
> do_xen_hypercall() or do_xen_user_hypercall()) to cover the two hypercalls?
>
> > diff --git a/include/xen/arm/hypercall.h b/include/xen/arm/hypercall.h
> > index b40485e54d80..624c8ad7e42a 100644
> > --- a/include/xen/arm/hypercall.h
> > +++ b/include/xen/arm/hypercall.h
> > @@ -30,8 +30,8 @@
> >    * IN THE SOFTWARE.
> >    */
> >
> > -#ifndef _ASM_ARM_XEN_HYPERCALL_H
> > -#define _ASM_ARM_XEN_HYPERCALL_H
> > +#ifndef _ARM_XEN_HYPERCALL_H
> > +#define _ARM_XEN_HYPERCALL_H
>
> This change feels a bit out of context. Could you split it in a separate
> patch?

Makes sense, I am splitting this into a separate patch.

Thank you,
Pasha
diff mbox series

Patch

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 99929122dad7..8e9262a0f016 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -480,7 +480,7 @@  THUMB(	orr	\reg , \reg , #PSR_T_BIT	)
 	.macro	uaccess_disable, tmp, isb=1
 #ifdef CONFIG_CPU_SW_DOMAIN_PAN
 	/*
-	 * Whenever we re-enter userspace, the domains should always be
+	 * Whenever we re-enter kernel, the domains should always be
 	 * set appropriately.
 	 */
 	mov	\tmp, #DACR_UACCESS_DISABLE
diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index 3522cbaed316..cac5bd9ef519 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -1 +1,11 @@ 
+#ifndef _ASM_ARM_XEN_HYPERCALL_H
+#define _ASM_ARM_XEN_HYPERCALL_H
 #include <xen/arm/hypercall.h>
+
+static inline long privcmd_call(unsigned int call, unsigned long a1,
+				unsigned long a2, unsigned long a3,
+				unsigned long a4, unsigned long a5)
+{
+	return arch_privcmd_call(call, a1, a2, a3, a4, a5);
+}
+#endif /* _ASM_ARM_XEN_HYPERCALL_H */
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index dd6804a64f1a..e87280c6d25d 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -440,4 +440,4 @@  EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op_raw);
 EXPORT_SYMBOL_GPL(HYPERVISOR_multicall);
 EXPORT_SYMBOL_GPL(HYPERVISOR_vm_assist);
 EXPORT_SYMBOL_GPL(HYPERVISOR_dm_op);
-EXPORT_SYMBOL_GPL(privcmd_call);
+EXPORT_SYMBOL_GPL(arch_privcmd_call);
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index b11bba542fac..277078c7da49 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -94,7 +94,7 @@  HYPERCALL2(multicall);
 HYPERCALL2(vm_assist);
 HYPERCALL3(dm_op);
 
-ENTRY(privcmd_call)
+ENTRY(arch_privcmd_call)
 	stmdb sp!, {r4}
 	mov r12, r0
 	mov r0, r1
@@ -119,4 +119,4 @@  ENTRY(privcmd_call)
 
 	ldm sp!, {r4}
 	ret lr
-ENDPROC(privcmd_call);
+ENDPROC(arch_privcmd_call);
diff --git a/arch/arm64/include/asm/xen/hypercall.h b/arch/arm64/include/asm/xen/hypercall.h
index 3522cbaed316..1a74fb28607f 100644
--- a/arch/arm64/include/asm/xen/hypercall.h
+++ b/arch/arm64/include/asm/xen/hypercall.h
@@ -1 +1,29 @@ 
+#ifndef _ASM_ARM64_XEN_HYPERCALL_H
+#define _ASM_ARM64_XEN_HYPERCALL_H
 #include <xen/arm/hypercall.h>
+#include <linux/uaccess.h>
+
+static inline long privcmd_call(unsigned int call, unsigned long a1,
+				unsigned long a2, unsigned long a3,
+				unsigned long a4, unsigned long a5)
+{
+	long rv;
+
+	/*
+	 * Privcmd calls are issued by the userspace. The kernel needs to
+	 * enable access to TTBR0_EL1 as the hypervisor would issue stage 1
+	 * translations to user memory via AT instructions. Since AT
+	 * instructions are not affected by the PAN bit (ARMv8.1), we only
+	 * need the explicit uaccess_enable/disable if the TTBR0 PAN emulation
+	 * is enabled (it implies that hardware UAO and PAN disabled).
+	 */
+	uaccess_ttbr0_enable();
+	rv = arch_privcmd_call(call, a1, a2, a3, a4, a5);
+	/*
+	 * Disable userspace access from kernel once the hyp call completed.
+	 */
+	uaccess_ttbr0_disable();
+
+	return rv;
+}
+#endif /* _ASM_ARM64_XEN_HYPERCALL_H */
diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S
index c5f05c4a4d00..921611778d2a 100644
--- a/arch/arm64/xen/hypercall.S
+++ b/arch/arm64/xen/hypercall.S
@@ -49,7 +49,6 @@ 
 
 #include <linux/linkage.h>
 #include <asm/assembler.h>
-#include <asm/asm-uaccess.h>
 #include <xen/interface/xen.h>
 
 
@@ -86,27 +85,13 @@  HYPERCALL2(multicall);
 HYPERCALL2(vm_assist);
 HYPERCALL3(dm_op);
 
-ENTRY(privcmd_call)
+ENTRY(arch_privcmd_call)
 	mov x16, x0
 	mov x0, x1
 	mov x1, x2
 	mov x2, x3
 	mov x3, x4
 	mov x4, x5
-	/*
-	 * Privcmd calls are issued by the userspace. The kernel needs to
-	 * enable access to TTBR0_EL1 as the hypervisor would issue stage 1
-	 * translations to user memory via AT instructions. Since AT
-	 * instructions are not affected by the PAN bit (ARMv8.1), we only
-	 * need the explicit uaccess_enable/disable if the TTBR0 PAN emulation
-	 * is enabled (it implies that hardware UAO and PAN disabled).
-	 */
-	uaccess_ttbr0_enable x6, x7, x8
 	hvc XEN_IMM
-
-	/*
-	 * Disable userspace access from kernel once the hyp call completed.
-	 */
-	uaccess_ttbr0_disable x6, x7
 	ret
-ENDPROC(privcmd_call);
+ENDPROC(arch_privcmd_call);
diff --git a/include/xen/arm/hypercall.h b/include/xen/arm/hypercall.h
index b40485e54d80..624c8ad7e42a 100644
--- a/include/xen/arm/hypercall.h
+++ b/include/xen/arm/hypercall.h
@@ -30,8 +30,8 @@ 
  * IN THE SOFTWARE.
  */
 
-#ifndef _ASM_ARM_XEN_HYPERCALL_H
-#define _ASM_ARM_XEN_HYPERCALL_H
+#ifndef _ARM_XEN_HYPERCALL_H
+#define _ARM_XEN_HYPERCALL_H
 
 #include <linux/bug.h>
 
@@ -41,9 +41,9 @@ 
 
 struct xen_dm_op_buf;
 
-long privcmd_call(unsigned call, unsigned long a1,
-		unsigned long a2, unsigned long a3,
-		unsigned long a4, unsigned long a5);
+long arch_privcmd_call(unsigned int call, unsigned long a1,
+		       unsigned long a2, unsigned long a3,
+		       unsigned long a4, unsigned long a5);
 int HYPERVISOR_xen_version(int cmd, void *arg);
 int HYPERVISOR_console_io(int cmd, int count, char *str);
 int HYPERVISOR_grant_table_op(unsigned int cmd, void *uop, unsigned int count);
@@ -88,4 +88,4 @@  MULTI_mmu_update(struct multicall_entry *mcl, struct mmu_update *req,
 	BUG();
 }
 
-#endif /* _ASM_ARM_XEN_HYPERCALL_H */
+#endif /* _ARM_XEN_HYPERCALL_H */