* [PATCH 0/9] x86: Move the pv hypercall into C
@ 2016-07-18 9:51 Andrew Cooper
2016-07-18 9:51 ` [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h Andrew Cooper
` (8 more replies)
0 siblings, 9 replies; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Tim Deegan, Julien Grall, Jan Beulich
I decided to dust off this series, and found it had been accumulating dust for
18 months. Oops.
There is no practical change from the point of view of guests, but:
* There is a net reduction in LoC
* There is a net reduction in compiled size (according to bloat-o-meter)
* Easier to maintain/edit/hotpatch
I think there is also now room to simplify how continuations are handled,
but I have tried not to get too carried away yet.
Andrew Cooper (9):
x86/hypercall: Move some of the hvm hypercall infrastructure into
hypercall.h
x86/pv: Support do_set_segment_base() for compat guests
x86/hypercall: Move the hypercall arg tables into C
x86/pv: Implement pv_hypercall() in C
x86/hypercall: Move the hypercall tables into C
xen/multicall: Rework arch multicall handling
x86/pv: Merge the pv hypercall tables
x86/hypercall: Merge the hypercall arg tables
x86/hypercall: Reduce the size of the hypercall tables
xen/arch/arm/traps.c | 3 +-
xen/arch/x86/Makefile | 1 +
xen/arch/x86/hvm/hvm.c | 33 ++---
xen/arch/x86/hypercall.c | 295 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/trace.c | 27 ----
xen/arch/x86/x86_64/asm-offsets.c | 1 -
xen/arch/x86/x86_64/compat.c | 1 -
xen/arch/x86/x86_64/compat/entry.S | 184 +----------------------
xen/arch/x86/x86_64/compat/traps.c | 2 -
xen/arch/x86/x86_64/entry.S | 173 +---------------------
xen/arch/x86/x86_64/mm.c | 3 +
xen/arch/x86/x86_64/traps.c | 1 +
xen/common/kernel.c | 6 -
xen/common/multicall.c | 2 +-
xen/include/asm-arm/multicall.h | 14 --
xen/include/asm-x86/hypercall.h | 65 +++++++-
xen/include/asm-x86/multicall.h | 72 ---------
xen/include/xen/hypercall.h | 18 ++-
xen/include/xen/multicall.h | 3 +-
19 files changed, 399 insertions(+), 505 deletions(-)
create mode 100644 xen/arch/x86/hypercall.c
delete mode 100644 xen/include/asm-arm/multicall.h
delete mode 100644 xen/include/asm-x86/multicall.h
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-08-02 12:50 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests Andrew Cooper
` (7 subsequent siblings)
8 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
It will be reused for PV hypercalls in subsequent changes.
* Rename hvm_hypercall_t to hypercall_fn_t
* Introduce hypercall_table_t
Finally, rework the #includes for hypercall.h so it may be included in
isolation.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/hvm/hvm.c | 19 +++++--------------
xen/include/asm-x86/hypercall.h | 14 +++++++++++++-
2 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index daaee1d..0966bd3 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4078,17 +4078,13 @@ static long hvm_physdev_op_compat32(
}
}
-typedef unsigned long hvm_hypercall_t(
- unsigned long, unsigned long, unsigned long, unsigned long, unsigned long,
- unsigned long);
-
#define HYPERCALL(x) \
- [ __HYPERVISOR_ ## x ] = { (hvm_hypercall_t *) do_ ## x, \
- (hvm_hypercall_t *) do_ ## x }
+ [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
+ (hypercall_fn_t *) do_ ## x }
#define COMPAT_CALL(x) \
- [ __HYPERVISOR_ ## x ] = { (hvm_hypercall_t *) do_ ## x, \
- (hvm_hypercall_t *) compat_ ## x }
+ [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
+ (hypercall_fn_t *) compat_ ## x }
#define do_memory_op hvm_memory_op
#define compat_memory_op hvm_memory_op_compat32
@@ -4098,10 +4094,7 @@ typedef unsigned long hvm_hypercall_t(
#define compat_grant_table_op hvm_grant_table_op_compat32
#define do_arch_1 paging_domctl_continuation
-static const struct {
- hvm_hypercall_t *native;
- hvm_hypercall_t *compat;
-} hvm_hypercall_table[NR_hypercalls] = {
+static const hypercall_table_t hvm_hypercall_table[NR_hypercalls] = {
COMPAT_CALL(memory_op),
COMPAT_CALL(grant_table_op),
COMPAT_CALL(vcpu_op),
@@ -4133,8 +4126,6 @@ static const struct {
#undef HYPERCALL
#undef COMPAT_CALL
-extern const uint8_t hypercall_args_table[], compat_hypercall_args_table[];
-
int hvm_do_hypercall(struct cpu_user_regs *regs)
{
struct vcpu *curr = current;
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index 945d58a..731f3a8 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -5,9 +5,21 @@
#ifndef __ASM_X86_HYPERCALL_H__
#define __ASM_X86_HYPERCALL_H__
+#include <xen/types.h>
#include <public/physdev.h>
+#include <public/event_channel.h>
#include <public/arch-x86/xen-mca.h> /* for do_mca */
-#include <xen/types.h>
+
+typedef unsigned long hypercall_fn_t(
+ unsigned long, unsigned long, unsigned long,
+ unsigned long, unsigned long, unsigned long);
+
+typedef struct {
+ hypercall_fn_t *native, *compat;
+} hypercall_table_t;
+
+extern const uint8_t hypercall_args_table[NR_hypercalls],
+ compat_hypercall_args_table[NR_hypercalls];
/*
* Both do_mmuext_op() and do_mmu_update():
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
2016-07-18 9:51 ` [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-08-02 12:52 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 3/9] x86/hypercall: Move the hypercall arg tables into C Andrew Cooper
` (6 subsequent siblings)
8 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
set_segment_base is the only hypercall exists in only one of the two modes
guests might run in; all other hypercalls are either implemented, or
unimplemented in both modes.
Remove this split, by allowing do_set_segment_base() to be called in the
compat hypercall path. This change will simplify the verification logic in a
later change.
No behavioural change from a guests point of view.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/x86_64/compat/entry.S | 2 +-
xen/arch/x86/x86_64/mm.c | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 7f02afd..89673c4 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table)
.quad compat_update_va_mapping_otherdomain
.quad compat_iret
.quad compat_vcpu_op
- .quad compat_ni_hypercall /* 25 */
+ .quad do_set_segment_base /* 25 */
.quad compat_mmuext_op
.quad compat_xsm_op
.quad compat_nmi_op
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index ff9fc43..512d855 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1031,6 +1031,9 @@ long do_set_segment_base(unsigned int which, unsigned long base)
struct vcpu *v = current;
long ret = 0;
+ if ( is_pv_32bit_vcpu(v) )
+ return -ENOSYS; /* x86/64 only. */
+
switch ( which )
{
case SEGBASE_FS:
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 3/9] x86/hypercall: Move the hypercall arg tables into C
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
2016-07-18 9:51 ` [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h Andrew Cooper
2016-07-18 9:51 ` [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-08-02 12:59 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 4/9] x86/pv: Implement pv_hypercall() in C Andrew Cooper
` (5 subsequent siblings)
8 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Editing (and indeed, finding) the hypercall args tables can be tricky,
especially towards the end where .rept's are used to maintain the correct
layout.
Move this all into C, and let the compiler do the hard work. As 0 is the
default value, drop all explicit 0's.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/Makefile | 1 +
xen/arch/x86/hypercall.c | 123 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/x86_64/compat/entry.S | 51 ---------------
xen/arch/x86/x86_64/entry.S | 51 ---------------
4 files changed, 124 insertions(+), 102 deletions(-)
create mode 100644 xen/arch/x86/hypercall.c
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b18f033..6dab907 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -28,6 +28,7 @@ obj-y += e820.o
obj-y += extable.o
obj-y += flushtlb.o
obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
+obj-y += hypercall.o
obj-y += i387.o
obj-y += i8259.o
obj-y += io_apic.o
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
new file mode 100644
index 0000000..4b42f86
--- /dev/null
+++ b/xen/arch/x86/hypercall.c
@@ -0,0 +1,123 @@
+/******************************************************************************
+ * arch/x86/hypercall.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2015,2016 Citrix Systems Ltd.
+ */
+
+#include <xen/hypercall.h>
+
+#define ARGS(x, n) \
+ [ __HYPERVISOR_ ## x ] = (n)
+
+const uint8_t hypercall_args_table[NR_hypercalls] =
+{
+ ARGS(set_trap_table, 1),
+ ARGS(mmu_update, 4),
+ ARGS(set_gdt, 2),
+ ARGS(stack_switch, 2),
+ ARGS(set_callbacks, 3),
+ ARGS(fpu_taskswitch, 1),
+ ARGS(sched_op_compat, 2),
+ ARGS(platform_op, 1),
+ ARGS(set_debugreg, 2),
+ ARGS(get_debugreg, 1),
+ ARGS(update_descriptor, 2),
+ ARGS(memory_op, 2),
+ ARGS(multicall, 2),
+ ARGS(update_va_mapping, 3),
+ ARGS(set_timer_op, 1),
+ ARGS(event_channel_op_compat, 1),
+ ARGS(xen_version, 2),
+ ARGS(console_io, 3),
+ ARGS(physdev_op_compat, 1),
+ ARGS(grant_table_op, 3),
+ ARGS(vm_assist, 2),
+ ARGS(update_va_mapping_otherdomain, 4),
+ ARGS(vcpu_op, 3),
+ ARGS(set_segment_base, 2),
+ ARGS(mmuext_op, 4),
+ ARGS(xsm_op, 1),
+ ARGS(nmi_op, 2),
+ ARGS(sched_op, 2),
+ ARGS(callback_op, 2),
+ ARGS(xenoprof_op, 2),
+ ARGS(event_channel_op, 2),
+ ARGS(physdev_op, 2),
+ ARGS(hvm_op, 2),
+ ARGS(sysctl, 1),
+ ARGS(domctl, 1),
+ ARGS(kexec_op, 2),
+ ARGS(tmem_op, 1),
+ ARGS(xenpmu_op, 2),
+ ARGS(mca, 1),
+ ARGS(arch_1, 1),
+};
+
+const uint8_t compat_hypercall_args_table[NR_hypercalls] =
+{
+ ARGS(set_trap_table, 1),
+ ARGS(mmu_update, 4),
+ ARGS(set_gdt, 2),
+ ARGS(stack_switch, 2),
+ ARGS(set_callbacks, 4),
+ ARGS(fpu_taskswitch, 1),
+ ARGS(sched_op_compat, 2),
+ ARGS(platform_op, 1),
+ ARGS(set_debugreg, 2),
+ ARGS(get_debugreg, 1),
+ ARGS(update_descriptor, 4),
+ ARGS(memory_op, 2),
+ ARGS(multicall, 2),
+ ARGS(update_va_mapping, 4),
+ ARGS(set_timer_op, 2),
+ ARGS(event_channel_op_compat, 1),
+ ARGS(xen_version, 2),
+ ARGS(console_io, 3),
+ ARGS(physdev_op_compat, 1),
+ ARGS(grant_table_op, 3),
+ ARGS(vm_assist, 2),
+ ARGS(update_va_mapping_otherdomain, 5),
+ ARGS(vcpu_op, 3),
+ ARGS(mmuext_op, 4),
+ ARGS(xsm_op, 1),
+ ARGS(nmi_op, 2),
+ ARGS(sched_op, 2),
+ ARGS(callback_op, 2),
+ ARGS(xenoprof_op, 2),
+ ARGS(event_channel_op, 2),
+ ARGS(physdev_op, 2),
+ ARGS(hvm_op, 2),
+ ARGS(sysctl, 1),
+ ARGS(domctl, 1),
+ ARGS(kexec_op, 2),
+ ARGS(tmem_op, 1),
+ ARGS(xenpmu_op, 2),
+ ARGS(mca, 1),
+ ARGS(arch_1, 1),
+};
+
+#undef ARGS
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 89673c4..1a8e085 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -477,54 +477,3 @@ ENTRY(compat_hypercall_table)
.rept NR_hypercalls-((.-compat_hypercall_table)/8)
.quad compat_ni_hypercall
.endr
-
-ENTRY(compat_hypercall_args_table)
- .byte 1 /* compat_set_trap_table */ /* 0 */
- .byte 4 /* compat_mmu_update */
- .byte 2 /* compat_set_gdt */
- .byte 2 /* compat_stack_switch */
- .byte 4 /* compat_set_callbacks */
- .byte 1 /* compat_fpu_taskswitch */ /* 5 */
- .byte 2 /* compat_sched_op_compat */
- .byte 1 /* compat_platform_op */
- .byte 2 /* compat_set_debugreg */
- .byte 1 /* compat_get_debugreg */
- .byte 4 /* compat_update_descriptor */ /* 10 */
- .byte 0 /* compat_ni_hypercall */
- .byte 2 /* compat_memory_op */
- .byte 2 /* compat_multicall */
- .byte 4 /* compat_update_va_mapping */
- .byte 2 /* compat_set_timer_op */ /* 15 */
- .byte 1 /* compat_event_channel_op_compat */
- .byte 2 /* compat_xen_version */
- .byte 3 /* compat_console_io */
- .byte 1 /* compat_physdev_op_compat */
- .byte 3 /* compat_grant_table_op */ /* 20 */
- .byte 2 /* compat_vm_assist */
- .byte 5 /* compat_update_va_mapping_otherdomain */
- .byte 0 /* compat_iret */
- .byte 3 /* compat_vcpu_op */
- .byte 0 /* compat_ni_hypercall */ /* 25 */
- .byte 4 /* compat_mmuext_op */
- .byte 1 /* do_xsm_op */
- .byte 2 /* compat_nmi_op */
- .byte 2 /* compat_sched_op */
- .byte 2 /* compat_callback_op */ /* 30 */
- .byte 2 /* compat_xenoprof_op */
- .byte 2 /* compat_event_channel_op */
- .byte 2 /* compat_physdev_op */
- .byte 2 /* do_hvm_op */
- .byte 1 /* do_sysctl */ /* 35 */
- .byte 1 /* do_domctl */
- .byte 2 /* compat_kexec_op */
- .byte 1 /* do_tmem_op */
- .byte 0 /* reserved for XenClient */
- .byte 2 /* do_xenpmu_op */ /* 40 */
- .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table)
- .byte 0 /* compat_ni_hypercall */
- .endr
- .byte 1 /* do_mca */
- .byte 1 /* paging_domctl_continuation */
- .rept NR_hypercalls-(.-compat_hypercall_args_table)
- .byte 0 /* compat_ni_hypercall */
- .endr
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index ad8c64c..5c6606b 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -801,57 +801,6 @@ ENTRY(hypercall_table)
.quad do_ni_hypercall
.endr
-ENTRY(hypercall_args_table)
- .byte 1 /* do_set_trap_table */ /* 0 */
- .byte 4 /* do_mmu_update */
- .byte 2 /* do_set_gdt */
- .byte 2 /* do_stack_switch */
- .byte 3 /* do_set_callbacks */
- .byte 1 /* do_fpu_taskswitch */ /* 5 */
- .byte 2 /* do_sched_op_compat */
- .byte 1 /* do_platform_op */
- .byte 2 /* do_set_debugreg */
- .byte 1 /* do_get_debugreg */
- .byte 2 /* do_update_descriptor */ /* 10 */
- .byte 0 /* do_ni_hypercall */
- .byte 2 /* do_memory_op */
- .byte 2 /* do_multicall */
- .byte 3 /* do_update_va_mapping */
- .byte 1 /* do_set_timer_op */ /* 15 */
- .byte 1 /* do_event_channel_op_compat */
- .byte 2 /* do_xen_version */
- .byte 3 /* do_console_io */
- .byte 1 /* do_physdev_op_compat */
- .byte 3 /* do_grant_table_op */ /* 20 */
- .byte 2 /* do_vm_assist */
- .byte 4 /* do_update_va_mapping_otherdomain */
- .byte 0 /* do_iret */
- .byte 3 /* do_vcpu_op */
- .byte 2 /* do_set_segment_base */ /* 25 */
- .byte 4 /* do_mmuext_op */
- .byte 1 /* do_xsm_op */
- .byte 2 /* do_nmi_op */
- .byte 2 /* do_sched_op */
- .byte 2 /* do_callback_op */ /* 30 */
- .byte 2 /* do_xenoprof_op */
- .byte 2 /* do_event_channel_op */
- .byte 2 /* do_physdev_op */
- .byte 2 /* do_hvm_op */
- .byte 1 /* do_sysctl */ /* 35 */
- .byte 1 /* do_domctl */
- .byte 2 /* do_kexec */
- .byte 1 /* do_tmem_op */
- .byte 0 /* reserved for XenClient */
- .byte 2 /* do_xenpmu_op */ /* 40 */
- .rept __HYPERVISOR_arch_0-(.-hypercall_args_table)
- .byte 0 /* do_ni_hypercall */
- .endr
- .byte 1 /* do_mca */ /* 48 */
- .byte 1 /* paging_domctl_continuation */
- .rept NR_hypercalls-(.-hypercall_args_table)
- .byte 0 /* do_ni_hypercall */
- .endr
-
/* Table of automatically generated entry points. One per vector. */
.section .init.rodata, "a", @progbits
GLOBAL(autogen_entrypoints)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 4/9] x86/pv: Implement pv_hypercall() in C
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
` (2 preceding siblings ...)
2016-07-18 9:51 ` [PATCH 3/9] x86/hypercall: Move the hypercall arg tables into C Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-08-02 13:12 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 5/9] x86/hypercall: Move the hypercall tables into C Andrew Cooper
` (4 subsequent siblings)
8 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
In a similar style to hvm_do_hypercall(). The C version is far easier to
understand and edit than the assembly versions.
There are a few small differences however. The register clobbering values
have changed (to match the HVM side), and in particular clobber the upper
32bits of 64bit arguments. The hypercall and performance counter record are
reordered to increase code sharing between the 32bit and 64bit cases.
The sole callers of __trace_hypercall_entry() were the assembly code. Given
the new C layout, it is more convenient to fold __trace_hypercall_entry() into
pv_hypercall(), and call __trace_hypercall() directly.
Finally, pv_hypercall() will treat a NULL hypercall function pointer as
-ENOSYS, allowing further cleanup.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/hypercall.c | 115 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/trace.c | 27 ---------
xen/arch/x86/x86_64/asm-offsets.c | 1 -
xen/arch/x86/x86_64/compat/entry.S | 68 +---------------------
xen/arch/x86/x86_64/entry.S | 60 +------------------
5 files changed, 119 insertions(+), 152 deletions(-)
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 4b42f86..2dc787e 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -17,7 +17,9 @@
* Copyright (c) 2015,2016 Citrix Systems Ltd.
*/
+#include <xen/compiler.h>
#include <xen/hypercall.h>
+#include <xen/trace.h>
#define ARGS(x, n) \
[ __HYPERVISOR_ ## x ] = (n)
@@ -111,6 +113,119 @@ const uint8_t compat_hypercall_args_table[NR_hypercalls] =
#undef ARGS
+long pv_hypercall(struct cpu_user_regs *regs)
+{
+ struct vcpu *curr = current;
+#ifndef NDEBUG
+ unsigned long old_rip = regs->rip;
+#endif
+ long ret;
+ uint32_t eax = regs->eax;
+
+ ASSERT(curr->arch.flags & TF_kernel_mode);
+
+ if ( (eax >= NR_hypercalls) || !hypercall_table[eax] )
+ return -ENOSYS;
+
+ if ( !is_pv_32bit_vcpu(curr) )
+ {
+ unsigned long rdi = regs->rdi;
+ unsigned long rsi = regs->rsi;
+ unsigned long rdx = regs->rdx;
+ unsigned long r10 = regs->r10;
+ unsigned long r8 = regs->r8;
+ unsigned long r9 = regs->r9;
+
+#ifndef NDEBUG
+ /* Deliberately corrupt parameter regs not used by this hypercall. */
+ switch ( hypercall_args_table[eax] )
+ {
+ case 0: rdi = 0xdeadbeefdeadf00dUL;
+ case 1: rsi = 0xdeadbeefdeadf00dUL;
+ case 2: rdx = 0xdeadbeefdeadf00dUL;
+ case 3: r10 = 0xdeadbeefdeadf00dUL;
+ case 4: r8 = 0xdeadbeefdeadf00dUL;
+ case 5: r9 = 0xdeadbeefdeadf00dUL;
+ }
+#endif
+ if ( unlikely(tb_init_done) )
+ {
+ unsigned long args[6] = { rdi, rsi, rdx, r10, r8, r9 };
+
+ __trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
+ }
+
+ ret = hypercall_table[eax](rdi, rsi, rdx, r10, r8, r9);
+
+#ifndef NDEBUG
+ if ( regs->rip == old_rip )
+ {
+ /* Deliberately corrupt parameter regs used by this hypercall. */
+ switch ( hypercall_args_table[eax] )
+ {
+ case 6: regs->r9 = 0xdeadbeefdeadf00dUL;
+ case 5: regs->r8 = 0xdeadbeefdeadf00dUL;
+ case 4: regs->r10 = 0xdeadbeefdeadf00dUL;
+ case 3: regs->edx = 0xdeadbeefdeadf00dUL;
+ case 2: regs->esi = 0xdeadbeefdeadf00dUL;
+ case 1: regs->edi = 0xdeadbeefdeadf00dUL;
+ }
+ }
+#endif
+ }
+ else
+ {
+ unsigned int ebx = regs->_ebx;
+ unsigned int ecx = regs->_ecx;
+ unsigned int edx = regs->_edx;
+ unsigned int esi = regs->_esi;
+ unsigned int edi = regs->_edi;
+ unsigned int ebp = regs->_ebp;
+
+#ifndef NDEBUG
+ /* Deliberately corrupt parameter regs not used by this hypercall. */
+ switch ( compat_hypercall_args_table[eax] )
+ {
+ case 0: ebx = 0xdeadf00d;
+ case 1: ecx = 0xdeadf00d;
+ case 2: edx = 0xdeadf00d;
+ case 3: esi = 0xdeadf00d;
+ case 4: edi = 0xdeadf00d;
+ case 5: ebp = 0xdeadf00d;
+ }
+#endif
+
+ if ( unlikely(tb_init_done) )
+ {
+ unsigned long args[6] = { ebx, ecx, edx, esi, edi, ebp };
+
+ __trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
+ }
+
+ ret = compat_hypercall_table[eax](ebx, ecx, edx, esi, edi, ebp);
+
+#ifndef NDEBUG
+ if ( regs->rip == old_rip )
+ {
+ /* Deliberately corrupt parameter regs used by this hypercall. */
+ switch ( compat_hypercall_args_table[eax] )
+ {
+ case 6: regs->ebp = 0xdeadf00d;
+ case 5: regs->edi = 0xdeadf00d;
+ case 4: regs->esi = 0xdeadf00d;
+ case 3: regs->edx = 0xdeadf00d;
+ case 2: regs->ecx = 0xdeadf00d;
+ case 1: regs->ebx = 0xdeadf00d;
+ }
+ }
+#endif
+ }
+
+ perfc_incr(hypercalls);
+
+ return ret;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c
index bd8596c..f11b309 100644
--- a/xen/arch/x86/trace.c
+++ b/xen/arch/x86/trace.c
@@ -6,33 +6,6 @@
#include <xen/sched.h>
#include <xen/trace.h>
-void __trace_hypercall_entry(void)
-{
- struct cpu_user_regs *regs = guest_cpu_user_regs();
- unsigned long args[6];
-
- if ( is_pv_32bit_vcpu(current) )
- {
- args[0] = regs->ebx;
- args[1] = regs->ecx;
- args[2] = regs->edx;
- args[3] = regs->esi;
- args[4] = regs->edi;
- args[5] = regs->ebp;
- }
- else
- {
- args[0] = regs->rdi;
- args[1] = regs->rsi;
- args[2] = regs->rdx;
- args[3] = regs->r10;
- args[4] = regs->r8;
- args[5] = regs->r9;
- }
-
- __trace_hypercall(TRC_PV_HYPERCALL_V2, regs->eax, args);
-}
-
void __trace_pv_trap(int trapnr, unsigned long eip,
int use_error_code, unsigned error_code)
{
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 05d2b85..64905c6 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -153,7 +153,6 @@ void __dummy__(void)
BLANK();
#ifdef CONFIG_PERF_COUNTERS
- DEFINE(ASM_PERFC_hypercalls, PERFC_hypercalls);
DEFINE(ASM_PERFC_exceptions, PERFC_exceptions);
BLANK();
#endif
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 1a8e085..521af48 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -25,70 +25,10 @@ UNLIKELY_START(ne, msi_check)
LOAD_C_CLOBBERED compat=1 ax=0
UNLIKELY_END(msi_check)
- movl UREGS_rax(%rsp),%eax
GET_CURRENT(bx)
- cmpl $NR_hypercalls,%eax
- jae compat_bad_hypercall
-#ifndef NDEBUG
- /* Deliberately corrupt parameter regs not used by this hypercall. */
- pushq UREGS_rbx(%rsp); pushq %rcx; pushq %rdx; pushq %rsi; pushq %rdi
- pushq UREGS_rbp+5*8(%rsp)
- leaq compat_hypercall_args_table(%rip),%r10
- movl $6,%ecx
- subb (%r10,%rax,1),%cl
- movq %rsp,%rdi
- movl $0xDEADBEEF,%eax
- rep stosq
- popq %r8 ; popq %r9 ; xchgl %r8d,%r9d /* Args 5&6: zero extend */
- popq %rdx; popq %rcx; xchgl %edx,%ecx /* Args 3&4: zero extend */
- popq %rdi; popq %rsi; xchgl %edi,%esi /* Args 1&2: zero extend */
- movl UREGS_rax(%rsp),%eax
- pushq %rax
- pushq UREGS_rip+8(%rsp)
-#define SHADOW_BYTES 16 /* Shadow EIP + shadow hypercall # */
-#else
- /* Relocate argument registers and zero-extend to 64 bits. */
- xchgl %ecx,%esi /* Arg 2, Arg 4 */
- movl %edx,%edx /* Arg 3 */
- movl %edi,%r8d /* Arg 5 */
- movl %ebp,%r9d /* Arg 6 */
- movl UREGS_rbx(%rsp),%edi /* Arg 1 */
-#define SHADOW_BYTES 0 /* No on-stack shadow state */
-#endif
- cmpb $0,tb_init_done(%rip)
-UNLIKELY_START(ne, compat_trace)
- call __trace_hypercall_entry
- /* Restore the registers that __trace_hypercall_entry clobbered. */
- movl UREGS_rax+SHADOW_BYTES(%rsp),%eax /* Hypercall # */
- movl UREGS_rbx+SHADOW_BYTES(%rsp),%edi /* Arg 1 */
- movl UREGS_rcx+SHADOW_BYTES(%rsp),%esi /* Arg 2 */
- movl UREGS_rdx+SHADOW_BYTES(%rsp),%edx /* Arg 3 */
- movl UREGS_rsi+SHADOW_BYTES(%rsp),%ecx /* Arg 4 */
- movl UREGS_rdi+SHADOW_BYTES(%rsp),%r8d /* Arg 5 */
- movl UREGS_rbp+SHADOW_BYTES(%rsp),%r9d /* Arg 6 */
-#undef SHADOW_BYTES
-UNLIKELY_END(compat_trace)
- leaq compat_hypercall_table(%rip),%r10
- PERFC_INCR(hypercalls, %rax, %rbx)
- callq *(%r10,%rax,8)
-#ifndef NDEBUG
- /* Deliberately corrupt parameter regs used by this hypercall. */
- popq %r10 # Shadow RIP
- cmpq %r10,UREGS_rip+8(%rsp)
- popq %rcx # Shadow hypercall index
- jne compat_skip_clobber /* If RIP has changed then don't clobber. */
- leaq compat_hypercall_args_table(%rip),%r10
- movb (%r10,%rcx,1),%cl
- movl $0xDEADBEEF,%r10d
- testb %cl,%cl; jz compat_skip_clobber; movl %r10d,UREGS_rbx(%rsp)
- cmpb $2, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rcx(%rsp)
- cmpb $3, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdx(%rsp)
- cmpb $4, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rsi(%rsp)
- cmpb $5, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdi(%rsp)
- cmpb $6, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rbp(%rsp)
-compat_skip_clobber:
-#endif
+ mov %rsp, %rdi
+ call pv_hypercall
movl %eax,UREGS_rax(%rsp) # save the return value
/* %rbx: struct vcpu */
@@ -167,10 +107,6 @@ compat_process_trap:
call compat_create_bounce_frame
jmp compat_test_all_events
-compat_bad_hypercall:
- movl $-ENOSYS,UREGS_rax(%rsp)
- jmp compat_test_all_events
-
/* %rbx: struct vcpu, interrupts disabled */
ENTRY(compat_restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 5c6606b..1a7f8b7 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -101,60 +101,8 @@ ENTRY(lstar_enter)
testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
jz switch_to_kernel
-/*hypercall:*/
- movq %r10,%rcx
- cmpq $NR_hypercalls,%rax
- jae bad_hypercall
-#ifndef NDEBUG
- /* Deliberately corrupt parameter regs not used by this hypercall. */
- pushq %rdi; pushq %rsi; pushq %rdx; pushq %rcx; pushq %r8 ; pushq %r9
- leaq hypercall_args_table(%rip),%r10
- movq $6,%rcx
- sub (%r10,%rax,1),%cl
- movq %rsp,%rdi
- movl $0xDEADBEEF,%eax
- rep stosq
- popq %r9 ; popq %r8 ; popq %rcx; popq %rdx; popq %rsi; popq %rdi
- movq UREGS_rax(%rsp),%rax
- pushq %rax
- pushq UREGS_rip+8(%rsp)
-#define SHADOW_BYTES 16 /* Shadow EIP + shadow hypercall # */
-#else
-#define SHADOW_BYTES 0 /* No on-stack shadow state */
-#endif
- cmpb $0,tb_init_done(%rip)
-UNLIKELY_START(ne, trace)
- call __trace_hypercall_entry
- /* Restore the registers that __trace_hypercall_entry clobbered. */
- movq UREGS_rax+SHADOW_BYTES(%rsp),%rax /* Hypercall # */
- movq UREGS_rdi+SHADOW_BYTES(%rsp),%rdi /* Arg 1 */
- movq UREGS_rsi+SHADOW_BYTES(%rsp),%rsi /* Arg 2 */
- movq UREGS_rdx+SHADOW_BYTES(%rsp),%rdx /* Arg 3 */
- movq UREGS_r10+SHADOW_BYTES(%rsp),%rcx /* Arg 4 */
- movq UREGS_r8 +SHADOW_BYTES(%rsp),%r8 /* Arg 5 */
- movq UREGS_r9 +SHADOW_BYTES(%rsp),%r9 /* Arg 6 */
-#undef SHADOW_BYTES
-UNLIKELY_END(trace)
- leaq hypercall_table(%rip),%r10
- PERFC_INCR(hypercalls, %rax, %rbx)
- callq *(%r10,%rax,8)
-#ifndef NDEBUG
- /* Deliberately corrupt parameter regs used by this hypercall. */
- popq %r10 # Shadow RIP
- cmpq %r10,UREGS_rip+8(%rsp)
- popq %rcx # Shadow hypercall index
- jne skip_clobber /* If RIP has changed then don't clobber. */
- leaq hypercall_args_table(%rip),%r10
- movb (%r10,%rcx,1),%cl
- movl $0xDEADBEEF,%r10d
- cmpb $1,%cl; jb skip_clobber; movq %r10,UREGS_rdi(%rsp)
- cmpb $2,%cl; jb skip_clobber; movq %r10,UREGS_rsi(%rsp)
- cmpb $3,%cl; jb skip_clobber; movq %r10,UREGS_rdx(%rsp)
- cmpb $4,%cl; jb skip_clobber; movq %r10,UREGS_r10(%rsp)
- cmpb $5,%cl; jb skip_clobber; movq %r10,UREGS_r8(%rsp)
- cmpb $6,%cl; jb skip_clobber; movq %r10,UREGS_r9(%rsp)
-skip_clobber:
-#endif
+ mov %rsp, %rdi
+ call pv_hypercall
movq %rax,UREGS_rax(%rsp) # save the return value
/* %rbx: struct vcpu */
@@ -231,10 +179,6 @@ process_trap:
call create_bounce_frame
jmp test_all_events
-bad_hypercall:
- movq $-ENOSYS,UREGS_rax(%rsp)
- jmp test_all_events
-
ENTRY(sysenter_entry)
sti
pushq $FLAT_USER_SS
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 5/9] x86/hypercall: Move the hypercall tables into C
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
` (3 preceding siblings ...)
2016-07-18 9:51 ` [PATCH 4/9] x86/pv: Implement pv_hypercall() in C Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-08-02 13:23 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 6/9] xen/multicall: Rework arch multicall handling Andrew Cooper
` (3 subsequent siblings)
8 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Editing (and indeed, finding) the hypercall tables can be tricky, especially
towards the end where .rept's are used to maintain the correct layout.
Move this all into C, and let the compiler do the hard work.
To do this, xen/hypercall.h and asm-x86/hypercall.h need to contain prototypes
for all the hypercalls; some were previously missing. This in turn requires
some shuffling of definitions and includes.
One difference is that NULL function pointers are used instead of
{,compat_}do_ni_hypercall(), which pv_hypercall() handles correctly. All
ni_hypercall() infrastructure is therefore dropped.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/hypercall.c | 112 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/x86_64/compat.c | 1 -
xen/arch/x86/x86_64/compat/entry.S | 65 ---------------------
xen/arch/x86/x86_64/compat/traps.c | 2 -
xen/arch/x86/x86_64/entry.S | 62 --------------------
xen/arch/x86/x86_64/traps.c | 1 +
xen/common/kernel.c | 6 --
xen/include/asm-x86/hypercall.h | 48 ++++++++++++++++
xen/include/xen/hypercall.h | 18 ++++--
9 files changed, 173 insertions(+), 142 deletions(-)
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 2dc787e..86c097a 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -113,6 +113,118 @@ const uint8_t compat_hypercall_args_table[NR_hypercalls] =
#undef ARGS
+#define HYPERCALL(x) \
+ [ __HYPERVISOR_ ## x ] = (hypercall_fn_t *) do_ ## x
+
+#define do_arch_1 paging_domctl_continuation
+
+hypercall_fn_t *const hypercall_table[NR_hypercalls] = {
+ HYPERCALL(set_trap_table),
+ HYPERCALL(mmu_update),
+ HYPERCALL(set_gdt),
+ HYPERCALL(stack_switch),
+ HYPERCALL(set_callbacks),
+ HYPERCALL(fpu_taskswitch),
+ HYPERCALL(sched_op_compat),
+ HYPERCALL(platform_op),
+ HYPERCALL(set_debugreg),
+ HYPERCALL(get_debugreg),
+ HYPERCALL(update_descriptor),
+ HYPERCALL(memory_op),
+ HYPERCALL(multicall),
+ HYPERCALL(update_va_mapping),
+ HYPERCALL(set_timer_op),
+ HYPERCALL(event_channel_op_compat),
+ HYPERCALL(xen_version),
+ HYPERCALL(console_io),
+ HYPERCALL(physdev_op_compat),
+ HYPERCALL(grant_table_op),
+ HYPERCALL(vm_assist),
+ HYPERCALL(update_va_mapping_otherdomain),
+ HYPERCALL(iret),
+ HYPERCALL(vcpu_op),
+ HYPERCALL(set_segment_base),
+ HYPERCALL(mmuext_op),
+ HYPERCALL(xsm_op),
+ HYPERCALL(nmi_op),
+ HYPERCALL(sched_op),
+ HYPERCALL(callback_op),
+#ifdef CONFIG_XENOPROF
+ HYPERCALL(xenoprof_op),
+#endif
+ HYPERCALL(event_channel_op),
+ HYPERCALL(physdev_op),
+ HYPERCALL(hvm_op),
+ HYPERCALL(sysctl),
+ HYPERCALL(domctl),
+#ifdef CONFIG_KEXEC
+ HYPERCALL(kexec_op),
+#endif
+#ifdef CONFIG_TMEM
+ HYPERCALL(tmem_op),
+#endif
+ HYPERCALL(xenpmu_op),
+ HYPERCALL(mca),
+ HYPERCALL(arch_1),
+};
+
+#define COMPAT_CALL(x) \
+ [ __HYPERVISOR_ ## x ] = (hypercall_fn_t *) compat_ ## x
+
+hypercall_fn_t *const compat_hypercall_table[NR_hypercalls] = {
+ COMPAT_CALL(set_trap_table),
+ HYPERCALL(mmu_update),
+ COMPAT_CALL(set_gdt),
+ HYPERCALL(stack_switch),
+ COMPAT_CALL(set_callbacks),
+ HYPERCALL(fpu_taskswitch),
+ HYPERCALL(sched_op_compat),
+ COMPAT_CALL(platform_op),
+ HYPERCALL(set_debugreg),
+ HYPERCALL(get_debugreg),
+ COMPAT_CALL(update_descriptor),
+ COMPAT_CALL(memory_op),
+ COMPAT_CALL(multicall),
+ COMPAT_CALL(update_va_mapping),
+ COMPAT_CALL(set_timer_op),
+ HYPERCALL(event_channel_op_compat),
+ COMPAT_CALL(xen_version),
+ HYPERCALL(console_io),
+ COMPAT_CALL(physdev_op_compat),
+ COMPAT_CALL(grant_table_op),
+ COMPAT_CALL(vm_assist),
+ COMPAT_CALL(update_va_mapping_otherdomain),
+ COMPAT_CALL(iret),
+ COMPAT_CALL(vcpu_op),
+ HYPERCALL(set_segment_base),
+ COMPAT_CALL(mmuext_op),
+ COMPAT_CALL(xsm_op),
+ COMPAT_CALL(nmi_op),
+ COMPAT_CALL(sched_op),
+ COMPAT_CALL(callback_op),
+#ifdef CONFIG_XENOPROF
+ COMPAT_CALL(xenoprof_op),
+#endif
+ HYPERCALL(event_channel_op),
+ COMPAT_CALL(physdev_op),
+ HYPERCALL(hvm_op),
+ HYPERCALL(sysctl),
+ HYPERCALL(domctl),
+#ifdef CONFIG_KEXEC
+ COMPAT_CALL(kexec_op),
+#endif
+#ifdef CONFIG_TMEM
+ HYPERCALL(tmem_op),
+#endif
+ HYPERCALL(xenpmu_op),
+ HYPERCALL(mca),
+ HYPERCALL(arch_1),
+};
+
+#undef do_arch_1
+#undef COMPAT_CALL
+#undef HYPERCALL
+
long pv_hypercall(struct cpu_user_regs *regs)
{
struct vcpu *curr = current;
diff --git a/xen/arch/x86/x86_64/compat.c b/xen/arch/x86/x86_64/compat.c
index f8f633b..edc3115 100644
--- a/xen/arch/x86/x86_64/compat.c
+++ b/xen/arch/x86/x86_64/compat.c
@@ -8,7 +8,6 @@ asm(".file \"" __FILE__ "\"");
#include <compat/xen.h>
#include <compat/physdev.h>
-DEFINE_XEN_GUEST_HANDLE(physdev_op_compat_t);
#define physdev_op compat_physdev_op
#define physdev_op_t physdev_op_compat_t
#define do_physdev_op compat_physdev_op
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 521af48..7d8e1f8 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -348,68 +348,3 @@ compat_crash_page_fault:
jmp .Lft14
.previous
_ASM_EXTABLE(.Lft14, .Lfx14)
-
-.section .rodata, "a", @progbits
-
-#ifndef CONFIG_KEXEC
-#define compat_kexec_op do_ni_hypercall
-#endif
-
-#ifndef CONFIG_TMEM
-#define do_tmem_op do_ni_hypercall
-#endif
-
-#ifndef CONFIG_XENOPROF
-#define compat_xenoprof_op do_ni_hypercall
-#endif
-
-ENTRY(compat_hypercall_table)
- .quad compat_set_trap_table /* 0 */
- .quad do_mmu_update
- .quad compat_set_gdt
- .quad do_stack_switch
- .quad compat_set_callbacks
- .quad do_fpu_taskswitch /* 5 */
- .quad do_sched_op_compat
- .quad compat_platform_op
- .quad do_set_debugreg
- .quad do_get_debugreg
- .quad compat_update_descriptor /* 10 */
- .quad compat_ni_hypercall
- .quad compat_memory_op
- .quad compat_multicall
- .quad compat_update_va_mapping
- .quad compat_set_timer_op /* 15 */
- .quad do_event_channel_op_compat
- .quad compat_xen_version
- .quad do_console_io
- .quad compat_physdev_op_compat
- .quad compat_grant_table_op /* 20 */
- .quad compat_vm_assist
- .quad compat_update_va_mapping_otherdomain
- .quad compat_iret
- .quad compat_vcpu_op
- .quad do_set_segment_base /* 25 */
- .quad compat_mmuext_op
- .quad compat_xsm_op
- .quad compat_nmi_op
- .quad compat_sched_op
- .quad compat_callback_op /* 30 */
- .quad compat_xenoprof_op
- .quad do_event_channel_op
- .quad compat_physdev_op
- .quad do_hvm_op
- .quad do_sysctl /* 35 */
- .quad do_domctl
- .quad compat_kexec_op
- .quad do_tmem_op
- .quad do_ni_hypercall /* reserved for XenClient */
- .quad do_xenpmu_op /* 40 */
- .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8)
- .quad compat_ni_hypercall
- .endr
- .quad do_mca /* 48 */
- .quad paging_domctl_continuation
- .rept NR_hypercalls-((.-compat_hypercall_table)/8)
- .quad compat_ni_hypercall
- .endr
diff --git a/xen/arch/x86/x86_64/compat/traps.c b/xen/arch/x86/x86_64/compat/traps.c
index a6afb26..95c5cb3 100644
--- a/xen/arch/x86/x86_64/compat/traps.c
+++ b/xen/arch/x86/x86_64/compat/traps.c
@@ -329,8 +329,6 @@ long compat_set_callbacks(unsigned long event_selector,
return 0;
}
-DEFINE_XEN_GUEST_HANDLE(trap_info_compat_t);
-
int compat_set_trap_table(XEN_GUEST_HANDLE(trap_info_compat_t) traps)
{
struct compat_trap_info cur;
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 1a7f8b7..89bedac 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -682,68 +682,6 @@ ENTRY(exception_table)
.endr
.size exception_table, . - exception_table
-#ifndef CONFIG_KEXEC
-#define do_kexec_op do_ni_hypercall
-#endif
-
-#ifndef CONFIG_TMEM
-#define do_tmem_op do_ni_hypercall
-#endif
-
-#ifndef CONFIG_XENOPROF
-#define do_xenoprof_op do_ni_hypercall
-#endif
-
-ENTRY(hypercall_table)
- .quad do_set_trap_table /* 0 */
- .quad do_mmu_update
- .quad do_set_gdt
- .quad do_stack_switch
- .quad do_set_callbacks
- .quad do_fpu_taskswitch /* 5 */
- .quad do_sched_op_compat
- .quad do_platform_op
- .quad do_set_debugreg
- .quad do_get_debugreg
- .quad do_update_descriptor /* 10 */
- .quad do_ni_hypercall
- .quad do_memory_op
- .quad do_multicall
- .quad do_update_va_mapping
- .quad do_set_timer_op /* 15 */
- .quad do_event_channel_op_compat
- .quad do_xen_version
- .quad do_console_io
- .quad do_physdev_op_compat
- .quad do_grant_table_op /* 20 */
- .quad do_vm_assist
- .quad do_update_va_mapping_otherdomain
- .quad do_iret
- .quad do_vcpu_op
- .quad do_set_segment_base /* 25 */
- .quad do_mmuext_op
- .quad do_xsm_op
- .quad do_nmi_op
- .quad do_sched_op
- .quad do_callback_op /* 30 */
- .quad do_xenoprof_op
- .quad do_event_channel_op
- .quad do_physdev_op
- .quad do_hvm_op
- .quad do_sysctl /* 35 */
- .quad do_domctl
- .quad do_kexec_op
- .quad do_tmem_op
- .quad do_ni_hypercall /* reserved for XenClient */
- .quad do_xenpmu_op /* 40 */
- .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8)
- .quad do_ni_hypercall
- .endr
- .quad do_mca /* 48 */
- .quad paging_domctl_continuation
- .rept NR_hypercalls-((.-hypercall_table)/8)
- .quad do_ni_hypercall
- .endr
/* Table of automatically generated entry points. One per vector. */
.section .init.rodata, "a", @progbits
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 19f58a1..92848f5 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -14,6 +14,7 @@
#include <xen/nmi.h>
#include <xen/guest_access.h>
#include <xen/watchdog.h>
+#include <xen/hypercall.h>
#include <asm/current.h>
#include <asm/flushtlb.h>
#include <asm/traps.h>
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 91cc4ea..430ad13 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -451,12 +451,6 @@ DO(vm_assist)(unsigned int cmd, unsigned int type)
}
#endif
-DO(ni_hypercall)(void)
-{
- /* No-op hypercall. */
- return -ENOSYS;
-}
-
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index 731f3a8..c00de2b 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -9,6 +9,7 @@
#include <public/physdev.h>
#include <public/event_channel.h>
#include <public/arch-x86/xen-mca.h> /* for do_mca */
+#include <asm/paging.h>
typedef unsigned long hypercall_fn_t(
unsigned long, unsigned long, unsigned long,
@@ -32,6 +33,14 @@ extern long
do_event_channel_op_compat(
XEN_GUEST_HANDLE_PARAM(evtchn_op_t) uop);
+/* Legacy hypercall (as of 0x00030202). */
+extern long do_physdev_op_compat(
+ XEN_GUEST_HANDLE(physdev_op_t) uop);
+
+/* Legacy hypercall (as of 0x00030101). */
+extern long do_sched_op_compat(
+ int cmd, unsigned long arg);
+
extern long
do_set_trap_table(
XEN_GUEST_HANDLE_PARAM(const_trap_info_t) traps);
@@ -98,6 +107,9 @@ do_mmuext_op(
XEN_GUEST_HANDLE_PARAM(uint) pdone,
unsigned int foreigndom);
+extern long do_callback_op(
+ int cmd, XEN_GUEST_HANDLE_PARAM(const_void) arg);
+
extern unsigned long
do_iret(
void);
@@ -113,6 +125,11 @@ do_set_segment_base(
unsigned int which,
unsigned long base);
+#ifdef CONFIG_COMPAT
+
+#include <compat/arch-x86/xen.h>
+#include <compat/physdev.h>
+
extern int
compat_physdev_op(
int cmd,
@@ -131,4 +148,35 @@ extern int compat_mmuext_op(
extern int compat_platform_op(
XEN_GUEST_HANDLE_PARAM(void) u_xenpf_op);
+extern long compat_callback_op(
+ int cmd, XEN_GUEST_HANDLE(void) arg);
+
+extern int compat_update_va_mapping(
+ unsigned int va, u32 lo, u32 hi, unsigned int flags);
+
+extern int compat_update_va_mapping_otherdomain(
+ unsigned long va, u32 lo, u32 hi, unsigned long flags, domid_t domid);
+
+DEFINE_XEN_GUEST_HANDLE(trap_info_compat_t);
+extern int compat_set_trap_table(XEN_GUEST_HANDLE(trap_info_compat_t) traps);
+
+extern int compat_set_gdt(
+ XEN_GUEST_HANDLE_PARAM(uint) frame_list, unsigned int entries);
+
+extern int compat_update_descriptor(
+ u32 pa_lo, u32 pa_hi, u32 desc_lo, u32 desc_hi);
+
+extern unsigned int compat_iret(void);
+
+extern int compat_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
+
+extern long compat_set_callbacks(
+ unsigned long event_selector, unsigned long event_address,
+ unsigned long failsafe_selector, unsigned long failsafe_address);
+
+DEFINE_XEN_GUEST_HANDLE(physdev_op_compat_t);
+extern int compat_physdev_op_compat(XEN_GUEST_HANDLE(physdev_op_compat_t) uop);
+
+#endif /* CONFIG_COMPAT */
+
#endif /* __ASM_X86_HYPERCALL_H__ */
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index 0c8ae0e..207a0e8 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -19,10 +19,6 @@
#include <xsm/xsm.h>
extern long
-do_ni_hypercall(
- void);
-
-extern long
do_sched_op(
int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg);
@@ -137,8 +133,6 @@ do_xsm_op(
extern long
do_tmem_op(
XEN_GUEST_HANDLE_PARAM(tmem_op_t) uops);
-#else
-#define do_tmem_op do_ni_hypercall
#endif
extern long
@@ -184,6 +178,18 @@ compat_set_timer_op(
u32 lo,
s32 hi);
+extern int compat_xsm_op(
+ XEN_GUEST_HANDLE_PARAM(xsm_op_t) op);
+
+extern int compat_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg);
+
+extern int compat_vm_assist(unsigned int cmd, unsigned int type);
+
+DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t);
+extern int compat_multicall(
+ XEN_GUEST_HANDLE_PARAM(multicall_entry_compat_t) call_list,
+ uint32_t nr_calls);
+
#endif
void arch_get_xen_caps(xen_capabilities_info_t *info);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 6/9] xen/multicall: Rework arch multicall handling
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
` (4 preceding siblings ...)
2016-07-18 9:51 ` [PATCH 5/9] x86/hypercall: Move the hypercall tables into C Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-07-20 12:35 ` Julien Grall
2016-08-03 15:02 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 7/9] x86/pv: Merge the pv hypercall tables Andrew Cooper
` (2 subsequent siblings)
8 siblings, 2 replies; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Jan Beulich
The x86 multicall handling was previously some very hairy inline assembly, and
is hard to follow and maintain.
Replace the existing do_multicall_call() with arch_do_multicall_call(). The
x86 side needs to handle both compat and non-compat calls, so pass the full
multicall state, rather than just the multicall_entry sub-structure.
On the ARM side, alter the prototype to match, but there is no resulting
functional change. On the x86 side, the implementation is now in plain C.
This allows the removal of both asm/multicall.h header files.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/traps.c | 3 +-
xen/arch/x86/hypercall.c | 28 ++++++++++++++++
xen/common/multicall.c | 2 +-
xen/include/asm-arm/multicall.h | 14 --------
xen/include/asm-x86/multicall.h | 72 -----------------------------------------
xen/include/xen/multicall.h | 3 +-
6 files changed, 33 insertions(+), 89 deletions(-)
delete mode 100644 xen/include/asm-arm/multicall.h
delete mode 100644 xen/include/asm-x86/multicall.h
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a2eb1da..d75a0a2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1515,8 +1515,9 @@ static bool_t check_multicall_32bit_clean(struct multicall_entry *multi)
return true;
}
-void do_multicall_call(struct multicall_entry *multi)
+void arch_do_multicall_call(struct mc_state *state)
{
+ struct multicall_entry *multi = &state->call;
arm_hypercall_fn_t call = NULL;
if ( multi->op >= ARRAY_SIZE(arm_hypercall_table) )
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 86c097a..892012d 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -338,6 +338,34 @@ long pv_hypercall(struct cpu_user_regs *regs)
return ret;
}
+void arch_do_multicall_call(struct mc_state *state)
+{
+ if ( !is_pv_32bit_vcpu(current) )
+ {
+ struct multicall_entry *call = &state->call;
+
+ if ( (call->op < NR_hypercalls) && hypercall_table[call->op] )
+ call->result = hypercall_table[call->op](
+ call->args[0], call->args[1], call->args[2],
+ call->args[3], call->args[4], call->args[5]);
+ else
+ call->result = -ENOSYS;
+ }
+#ifdef CONFIG_COMPAT
+ else
+ {
+ struct compat_multicall_entry *call = &state->compat_call;
+
+ if ( (call->op < NR_hypercalls) && compat_hypercall_table[call->op] )
+ call->result = compat_hypercall_table[call->op](
+ call->args[0], call->args[1], call->args[2],
+ call->args[3], call->args[4], call->args[5]);
+ else
+ call->result = -ENOSYS;
+ }
+#endif
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index 21661ee..524c9bf 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -63,7 +63,7 @@ do_multicall(
trace_multicall_call(&mcs->call);
- do_multicall_call(&mcs->call);
+ arch_do_multicall_call(mcs);
#ifndef NDEBUG
{
diff --git a/xen/include/asm-arm/multicall.h b/xen/include/asm-arm/multicall.h
deleted file mode 100644
index b959262..0000000
--- a/xen/include/asm-arm/multicall.h
+++ /dev/null
@@ -1,14 +0,0 @@
-#ifndef __ASM_ARM_MULTICALL_H__
-#define __ASM_ARM_MULTICALL_H__
-
-extern void do_multicall_call(struct multicall_entry *call);
-
-#endif /* __ASM_ARM_MULTICALL_H__ */
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/include/asm-x86/multicall.h b/xen/include/asm-x86/multicall.h
deleted file mode 100644
index fcd0ea5..0000000
--- a/xen/include/asm-x86/multicall.h
+++ /dev/null
@@ -1,72 +0,0 @@
-/******************************************************************************
- * asm-x86/multicall.h
- */
-
-#ifndef __ASM_X86_MULTICALL_H__
-#define __ASM_X86_MULTICALL_H__
-
-#include <xen/errno.h>
-
-#define do_multicall_call(_call) \
- do { \
- __asm__ __volatile__ ( \
- " movq %c1(%0),%%rax; " \
- " leaq hypercall_table(%%rip),%%rdi; " \
- " cmpq $("STR(NR_hypercalls)"),%%rax; " \
- " jae 2f; " \
- " movq (%%rdi,%%rax,8),%%rax; " \
- " movq %c2+0*%c3(%0),%%rdi; " \
- " movq %c2+1*%c3(%0),%%rsi; " \
- " movq %c2+2*%c3(%0),%%rdx; " \
- " movq %c2+3*%c3(%0),%%rcx; " \
- " movq %c2+4*%c3(%0),%%r8; " \
- " movq %c2+5*%c3(%0),%%r9; " \
- " callq *%%rax; " \
- "1: movq %%rax,%c4(%0)\n" \
- ".section .fixup,\"ax\"\n" \
- "2: movq %5,%%rax\n" \
- " jmp 1b\n" \
- ".previous\n" \
- : \
- : "b" (_call), \
- "i" (offsetof(__typeof__(*_call), op)), \
- "i" (offsetof(__typeof__(*_call), args)), \
- "i" (sizeof(*(_call)->args)), \
- "i" (offsetof(__typeof__(*_call), result)), \
- "i" (-ENOSYS) \
- /* all the caller-saves registers */ \
- : "rax", "rcx", "rdx", "rsi", "rdi", \
- "r8", "r9", "r10", "r11" ); \
- } while ( 0 )
-
-#define compat_multicall_call(_call) \
- __asm__ __volatile__ ( \
- " movl %c1(%0),%%eax; " \
- " leaq compat_hypercall_table(%%rip),%%rdi; "\
- " cmpl $("STR(NR_hypercalls)"),%%eax; " \
- " jae 2f; " \
- " movq (%%rdi,%%rax,8),%%rax; " \
- " movl %c2+0*%c3(%0),%%edi; " \
- " movl %c2+1*%c3(%0),%%esi; " \
- " movl %c2+2*%c3(%0),%%edx; " \
- " movl %c2+3*%c3(%0),%%ecx; " \
- " movl %c2+4*%c3(%0),%%r8d; " \
- " movl %c2+5*%c3(%0),%%r9d; " \
- " callq *%%rax; " \
- "1: movl %%eax,%c4(%0)\n" \
- ".section .fixup,\"ax\"\n" \
- "2: movl %5,%%eax\n" \
- " jmp 1b\n" \
- ".previous\n" \
- : \
- : "b" (_call), \
- "i" (offsetof(__typeof__(*_call), op)), \
- "i" (offsetof(__typeof__(*_call), args)), \
- "i" (sizeof(*(_call)->args)), \
- "i" (offsetof(__typeof__(*_call), result)), \
- "i" (-ENOSYS) \
- /* all the caller-saves registers */ \
- : "rax", "rcx", "rdx", "rsi", "rdi", \
- "r8", "r9", "r10", "r11" ) \
-
-#endif /* __ASM_X86_MULTICALL_H__ */
diff --git a/xen/include/xen/multicall.h b/xen/include/xen/multicall.h
index 0e8d8bb..fff15eb 100644
--- a/xen/include/xen/multicall.h
+++ b/xen/include/xen/multicall.h
@@ -6,7 +6,6 @@
#define __XEN_MULTICALL_H__
#include <xen/percpu.h>
-#include <asm/multicall.h>
#ifdef CONFIG_COMPAT
#include <compat/xen.h>
#endif
@@ -25,4 +24,6 @@ struct mc_state {
};
};
+void arch_do_multicall_call(struct mc_state *mc);
+
#endif /* __XEN_MULTICALL_H__ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 7/9] x86/pv: Merge the pv hypercall tables
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
` (5 preceding siblings ...)
2016-07-18 9:51 ` [PATCH 6/9] xen/multicall: Rework arch multicall handling Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-08-03 15:07 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 8/9] x86/hypercall: Merge the hypercall arg tables Andrew Cooper
2016-07-18 9:51 ` [PATCH 9/9] x86/hypercall: Reduce the size of the hypercall tables Andrew Cooper
8 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
For the same reason as c/s 33a231e3f "x86/HVM: fold hypercall tables", this
removes the risk of accidentally updating only one of the tables.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/hypercall.c | 75 +++++++++---------------------------------------
1 file changed, 13 insertions(+), 62 deletions(-)
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 892012d..b9bd58d 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -114,64 +114,15 @@ const uint8_t compat_hypercall_args_table[NR_hypercalls] =
#undef ARGS
#define HYPERCALL(x) \
- [ __HYPERVISOR_ ## x ] = (hypercall_fn_t *) do_ ## x
+ [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
+ (hypercall_fn_t *) do_ ## x }
+#define COMPAT_CALL(x) \
+ [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
+ (hypercall_fn_t *) compat_ ## x }
#define do_arch_1 paging_domctl_continuation
-hypercall_fn_t *const hypercall_table[NR_hypercalls] = {
- HYPERCALL(set_trap_table),
- HYPERCALL(mmu_update),
- HYPERCALL(set_gdt),
- HYPERCALL(stack_switch),
- HYPERCALL(set_callbacks),
- HYPERCALL(fpu_taskswitch),
- HYPERCALL(sched_op_compat),
- HYPERCALL(platform_op),
- HYPERCALL(set_debugreg),
- HYPERCALL(get_debugreg),
- HYPERCALL(update_descriptor),
- HYPERCALL(memory_op),
- HYPERCALL(multicall),
- HYPERCALL(update_va_mapping),
- HYPERCALL(set_timer_op),
- HYPERCALL(event_channel_op_compat),
- HYPERCALL(xen_version),
- HYPERCALL(console_io),
- HYPERCALL(physdev_op_compat),
- HYPERCALL(grant_table_op),
- HYPERCALL(vm_assist),
- HYPERCALL(update_va_mapping_otherdomain),
- HYPERCALL(iret),
- HYPERCALL(vcpu_op),
- HYPERCALL(set_segment_base),
- HYPERCALL(mmuext_op),
- HYPERCALL(xsm_op),
- HYPERCALL(nmi_op),
- HYPERCALL(sched_op),
- HYPERCALL(callback_op),
-#ifdef CONFIG_XENOPROF
- HYPERCALL(xenoprof_op),
-#endif
- HYPERCALL(event_channel_op),
- HYPERCALL(physdev_op),
- HYPERCALL(hvm_op),
- HYPERCALL(sysctl),
- HYPERCALL(domctl),
-#ifdef CONFIG_KEXEC
- HYPERCALL(kexec_op),
-#endif
-#ifdef CONFIG_TMEM
- HYPERCALL(tmem_op),
-#endif
- HYPERCALL(xenpmu_op),
- HYPERCALL(mca),
- HYPERCALL(arch_1),
-};
-
-#define COMPAT_CALL(x) \
- [ __HYPERVISOR_ ## x ] = (hypercall_fn_t *) compat_ ## x
-
-hypercall_fn_t *const compat_hypercall_table[NR_hypercalls] = {
+static const hypercall_table_t pv_hypercall_table[NR_hypercalls] = {
COMPAT_CALL(set_trap_table),
HYPERCALL(mmu_update),
COMPAT_CALL(set_gdt),
@@ -236,7 +187,7 @@ long pv_hypercall(struct cpu_user_regs *regs)
ASSERT(curr->arch.flags & TF_kernel_mode);
- if ( (eax >= NR_hypercalls) || !hypercall_table[eax] )
+ if ( (eax >= NR_hypercalls) || !pv_hypercall_table[eax].native )
return -ENOSYS;
if ( !is_pv_32bit_vcpu(curr) )
@@ -267,7 +218,7 @@ long pv_hypercall(struct cpu_user_regs *regs)
__trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
}
- ret = hypercall_table[eax](rdi, rsi, rdx, r10, r8, r9);
+ ret = pv_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8, r9);
#ifndef NDEBUG
if ( regs->rip == old_rip )
@@ -314,7 +265,7 @@ long pv_hypercall(struct cpu_user_regs *regs)
__trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
}
- ret = compat_hypercall_table[eax](ebx, ecx, edx, esi, edi, ebp);
+ ret = pv_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi, ebp);
#ifndef NDEBUG
if ( regs->rip == old_rip )
@@ -344,8 +295,8 @@ void arch_do_multicall_call(struct mc_state *state)
{
struct multicall_entry *call = &state->call;
- if ( (call->op < NR_hypercalls) && hypercall_table[call->op] )
- call->result = hypercall_table[call->op](
+ if ( (call->op < NR_hypercalls) && pv_hypercall_table[call->op].native )
+ call->result = pv_hypercall_table[call->op].native(
call->args[0], call->args[1], call->args[2],
call->args[3], call->args[4], call->args[5]);
else
@@ -356,8 +307,8 @@ void arch_do_multicall_call(struct mc_state *state)
{
struct compat_multicall_entry *call = &state->compat_call;
- if ( (call->op < NR_hypercalls) && compat_hypercall_table[call->op] )
- call->result = compat_hypercall_table[call->op](
+ if ( (call->op < NR_hypercalls) && pv_hypercall_table[call->op].compat )
+ call->result = pv_hypercall_table[call->op].compat(
call->args[0], call->args[1], call->args[2],
call->args[3], call->args[4], call->args[5]);
else
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 8/9] x86/hypercall: Merge the hypercall arg tables
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
` (6 preceding siblings ...)
2016-07-18 9:51 ` [PATCH 7/9] x86/pv: Merge the pv hypercall tables Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-08-03 15:12 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 9/9] x86/hypercall: Reduce the size of the hypercall tables Andrew Cooper
8 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
For the same reason as c/s 33a231e3f "x86/HVM: fold hypercall tables" and
(TODO - changeset) "x86/pv: Merge the pv hypercall tables", this removes the
risk of accidentally updating only one of the tables.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/hvm/hvm.c | 8 ++---
xen/arch/x86/hypercall.c | 70 +++++++++--------------------------------
xen/include/asm-x86/hypercall.h | 7 +++--
3 files changed, 24 insertions(+), 61 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0966bd3..198fe34 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4175,7 +4175,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
#ifndef NDEBUG
/* Deliberately corrupt parameter regs not used by this hypercall. */
- switch ( hypercall_args_table[eax] )
+ switch ( hypercall_args_table[eax].native )
{
case 0: rdi = 0xdeadbeefdeadf00dUL;
case 1: rsi = 0xdeadbeefdeadf00dUL;
@@ -4196,7 +4196,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
if ( !curr->arch.hvm_vcpu.hcall_preempted )
{
/* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( hypercall_args_table[eax] )
+ switch ( hypercall_args_table[eax].native )
{
case 6: regs->r9 = 0xdeadbeefdeadf00dUL;
case 5: regs->r8 = 0xdeadbeefdeadf00dUL;
@@ -4222,7 +4222,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
#ifndef NDEBUG
/* Deliberately corrupt parameter regs not used by this hypercall. */
- switch ( compat_hypercall_args_table[eax] )
+ switch ( hypercall_args_table[eax].compat )
{
case 0: ebx = 0xdeadf00d;
case 1: ecx = 0xdeadf00d;
@@ -4240,7 +4240,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
if ( !curr->arch.hvm_vcpu.hcall_preempted )
{
/* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( compat_hypercall_args_table[eax] )
+ switch ( hypercall_args_table[eax].compat )
{
case 6: regs->ebp = 0xdeadf00d;
case 5: regs->edi = 0xdeadf00d;
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index b9bd58d..373bedf 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -22,77 +22,36 @@
#include <xen/trace.h>
#define ARGS(x, n) \
- [ __HYPERVISOR_ ## x ] = (n)
+ [ __HYPERVISOR_ ## x ] = { n, n }
+#define COMP(x, n, c) \
+ [ __HYPERVISOR_ ## x ] = { n, c }
-const uint8_t hypercall_args_table[NR_hypercalls] =
+const hypercall_args_t hypercall_args_table[NR_hypercalls] =
{
ARGS(set_trap_table, 1),
ARGS(mmu_update, 4),
ARGS(set_gdt, 2),
ARGS(stack_switch, 2),
- ARGS(set_callbacks, 3),
+ COMP(set_callbacks, 3, 4),
ARGS(fpu_taskswitch, 1),
ARGS(sched_op_compat, 2),
ARGS(platform_op, 1),
ARGS(set_debugreg, 2),
ARGS(get_debugreg, 1),
- ARGS(update_descriptor, 2),
+ COMP(update_descriptor, 2, 4),
ARGS(memory_op, 2),
ARGS(multicall, 2),
- ARGS(update_va_mapping, 3),
- ARGS(set_timer_op, 1),
+ COMP(update_va_mapping, 3, 4),
+ COMP(set_timer_op, 1, 2),
ARGS(event_channel_op_compat, 1),
ARGS(xen_version, 2),
ARGS(console_io, 3),
ARGS(physdev_op_compat, 1),
ARGS(grant_table_op, 3),
ARGS(vm_assist, 2),
- ARGS(update_va_mapping_otherdomain, 4),
- ARGS(vcpu_op, 3),
- ARGS(set_segment_base, 2),
- ARGS(mmuext_op, 4),
- ARGS(xsm_op, 1),
- ARGS(nmi_op, 2),
- ARGS(sched_op, 2),
- ARGS(callback_op, 2),
- ARGS(xenoprof_op, 2),
- ARGS(event_channel_op, 2),
- ARGS(physdev_op, 2),
- ARGS(hvm_op, 2),
- ARGS(sysctl, 1),
- ARGS(domctl, 1),
- ARGS(kexec_op, 2),
- ARGS(tmem_op, 1),
- ARGS(xenpmu_op, 2),
- ARGS(mca, 1),
- ARGS(arch_1, 1),
-};
-
-const uint8_t compat_hypercall_args_table[NR_hypercalls] =
-{
- ARGS(set_trap_table, 1),
- ARGS(mmu_update, 4),
- ARGS(set_gdt, 2),
- ARGS(stack_switch, 2),
- ARGS(set_callbacks, 4),
- ARGS(fpu_taskswitch, 1),
- ARGS(sched_op_compat, 2),
- ARGS(platform_op, 1),
- ARGS(set_debugreg, 2),
- ARGS(get_debugreg, 1),
- ARGS(update_descriptor, 4),
- ARGS(memory_op, 2),
- ARGS(multicall, 2),
- ARGS(update_va_mapping, 4),
- ARGS(set_timer_op, 2),
- ARGS(event_channel_op_compat, 1),
- ARGS(xen_version, 2),
- ARGS(console_io, 3),
- ARGS(physdev_op_compat, 1),
- ARGS(grant_table_op, 3),
- ARGS(vm_assist, 2),
- ARGS(update_va_mapping_otherdomain, 5),
+ COMP(update_va_mapping_otherdomain, 4, 5),
ARGS(vcpu_op, 3),
+ COMP(set_segment_base, 2, 0),
ARGS(mmuext_op, 4),
ARGS(xsm_op, 1),
ARGS(nmi_op, 2),
@@ -111,6 +70,7 @@ const uint8_t compat_hypercall_args_table[NR_hypercalls] =
ARGS(arch_1, 1),
};
+#undef COMP
#undef ARGS
#define HYPERCALL(x) \
@@ -201,7 +161,7 @@ long pv_hypercall(struct cpu_user_regs *regs)
#ifndef NDEBUG
/* Deliberately corrupt parameter regs not used by this hypercall. */
- switch ( hypercall_args_table[eax] )
+ switch ( hypercall_args_table[eax].native )
{
case 0: rdi = 0xdeadbeefdeadf00dUL;
case 1: rsi = 0xdeadbeefdeadf00dUL;
@@ -224,7 +184,7 @@ long pv_hypercall(struct cpu_user_regs *regs)
if ( regs->rip == old_rip )
{
/* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( hypercall_args_table[eax] )
+ switch ( hypercall_args_table[eax].native )
{
case 6: regs->r9 = 0xdeadbeefdeadf00dUL;
case 5: regs->r8 = 0xdeadbeefdeadf00dUL;
@@ -247,7 +207,7 @@ long pv_hypercall(struct cpu_user_regs *regs)
#ifndef NDEBUG
/* Deliberately corrupt parameter regs not used by this hypercall. */
- switch ( compat_hypercall_args_table[eax] )
+ switch ( hypercall_args_table[eax].compat )
{
case 0: ebx = 0xdeadf00d;
case 1: ecx = 0xdeadf00d;
@@ -271,7 +231,7 @@ long pv_hypercall(struct cpu_user_regs *regs)
if ( regs->rip == old_rip )
{
/* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( compat_hypercall_args_table[eax] )
+ switch ( hypercall_args_table[eax].compat )
{
case 6: regs->ebp = 0xdeadf00d;
case 5: regs->edi = 0xdeadf00d;
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index c00de2b..c59aa69 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -19,8 +19,11 @@ typedef struct {
hypercall_fn_t *native, *compat;
} hypercall_table_t;
-extern const uint8_t hypercall_args_table[NR_hypercalls],
- compat_hypercall_args_table[NR_hypercalls];
+typedef struct {
+ uint8_t native, compat;
+} hypercall_args_t;
+
+extern const hypercall_args_t hypercall_args_table[NR_hypercalls];
/*
* Both do_mmuext_op() and do_mmu_update():
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 9/9] x86/hypercall: Reduce the size of the hypercall tables
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
` (7 preceding siblings ...)
2016-07-18 9:51 ` [PATCH 8/9] x86/hypercall: Merge the hypercall arg tables Andrew Cooper
@ 2016-07-18 9:51 ` Andrew Cooper
2016-08-03 15:17 ` Jan Beulich
8 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-07-18 9:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
The highest populated entry in each hypercall table is currently at index 49.
There is no need to extend both to tables to 64 entries.
Range check eax against the hypercall table array size, and use a
BUILD_BUG_ON() to ensure that the hypercall tables don't grow larger than the
args table.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/hvm/hvm.c | 8 ++++++--
xen/arch/x86/hypercall.c | 14 ++++++++++----
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 198fe34..df177da 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4094,7 +4094,7 @@ static long hvm_physdev_op_compat32(
#define compat_grant_table_op hvm_grant_table_op_compat32
#define do_arch_1 paging_domctl_continuation
-static const hypercall_table_t hvm_hypercall_table[NR_hypercalls] = {
+static const hypercall_table_t hvm_hypercall_table[] = {
COMPAT_CALL(memory_op),
COMPAT_CALL(grant_table_op),
COMPAT_CALL(vcpu_op),
@@ -4153,7 +4153,11 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
if ( (eax & 0x80000000) && is_viridian_domain(currd) )
return viridian_hypercall(regs);
- if ( (eax >= NR_hypercalls) || !hvm_hypercall_table[eax].native )
+ BUILD_BUG_ON(ARRAY_SIZE(hvm_hypercall_table) >
+ ARRAY_SIZE(hypercall_args_table));
+
+ if ( (eax >= ARRAY_SIZE(hvm_hypercall_table)) ||
+ !hvm_hypercall_table[eax].native )
{
regs->eax = -ENOSYS;
return HVM_HCALL_completed;
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 373bedf..1bf0a1c 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -82,7 +82,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
#define do_arch_1 paging_domctl_continuation
-static const hypercall_table_t pv_hypercall_table[NR_hypercalls] = {
+static const hypercall_table_t pv_hypercall_table[] = {
COMPAT_CALL(set_trap_table),
HYPERCALL(mmu_update),
COMPAT_CALL(set_gdt),
@@ -147,7 +147,11 @@ long pv_hypercall(struct cpu_user_regs *regs)
ASSERT(curr->arch.flags & TF_kernel_mode);
- if ( (eax >= NR_hypercalls) || !pv_hypercall_table[eax].native )
+ BUILD_BUG_ON(ARRAY_SIZE(pv_hypercall_table) >
+ ARRAY_SIZE(hypercall_args_table));
+
+ if ( (eax >= ARRAY_SIZE(pv_hypercall_table)) ||
+ !pv_hypercall_table[eax].native )
return -ENOSYS;
if ( !is_pv_32bit_vcpu(curr) )
@@ -255,7 +259,8 @@ void arch_do_multicall_call(struct mc_state *state)
{
struct multicall_entry *call = &state->call;
- if ( (call->op < NR_hypercalls) && pv_hypercall_table[call->op].native )
+ if ( (call->op < ARRAY_SIZE(pv_hypercall_table)) &&
+ pv_hypercall_table[call->op].native )
call->result = pv_hypercall_table[call->op].native(
call->args[0], call->args[1], call->args[2],
call->args[3], call->args[4], call->args[5]);
@@ -267,7 +272,8 @@ void arch_do_multicall_call(struct mc_state *state)
{
struct compat_multicall_entry *call = &state->compat_call;
- if ( (call->op < NR_hypercalls) && pv_hypercall_table[call->op].compat )
+ if ( (call->op < ARRAY_SIZE(pv_hypercall_table)) &&
+ pv_hypercall_table[call->op].compat )
call->result = pv_hypercall_table[call->op].compat(
call->args[0], call->args[1], call->args[2],
call->args[3], call->args[4], call->args[5]);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 6/9] xen/multicall: Rework arch multicall handling
2016-07-18 9:51 ` [PATCH 6/9] xen/multicall: Rework arch multicall handling Andrew Cooper
@ 2016-07-20 12:35 ` Julien Grall
2016-08-03 15:02 ` Jan Beulich
1 sibling, 0 replies; 48+ messages in thread
From: Julien Grall @ 2016-07-20 12:35 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Stefano Stabellini, Jan Beulich
Hi Andrew,
On 18/07/16 10:51, Andrew Cooper wrote:
> The x86 multicall handling was previously some very hairy inline assembly, and
> is hard to follow and maintain.
>
> Replace the existing do_multicall_call() with arch_do_multicall_call(). The
> x86 side needs to handle both compat and non-compat calls, so pass the full
> multicall state, rather than just the multicall_entry sub-structure.
>
> On the ARM side, alter the prototype to match, but there is no resulting
> functional change. On the x86 side, the implementation is now in plain C.
>
> This allows the removal of both asm/multicall.h header files.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
For the ARM bits:
Acked-by: Julien Grall <julien.grall@arm.com>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-07-18 9:51 ` [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h Andrew Cooper
@ 2016-08-02 12:50 ` Jan Beulich
2016-08-02 13:14 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 12:50 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/hypercall.h
> +++ b/xen/include/asm-x86/hypercall.h
> @@ -5,9 +5,21 @@
> #ifndef __ASM_X86_HYPERCALL_H__
> #define __ASM_X86_HYPERCALL_H__
>
> +#include <xen/types.h>
> #include <public/physdev.h>
> +#include <public/event_channel.h>
Why?
> #include <public/arch-x86/xen-mca.h> /* for do_mca */
> -#include <xen/types.h>
> +
> +typedef unsigned long hypercall_fn_t(
> + unsigned long, unsigned long, unsigned long,
> + unsigned long, unsigned long, unsigned long);
Wouldn't this better go into xen/hypercall.h?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests
2016-07-18 9:51 ` [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests Andrew Cooper
@ 2016-08-02 12:52 ` Jan Beulich
2016-08-02 13:25 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 12:52 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> set_segment_base is the only hypercall exists in only one of the two modes
> guests might run in; all other hypercalls are either implemented, or
> unimplemented in both modes.
>
> Remove this split, by allowing do_set_segment_base() to be called in the
> compat hypercall path. This change will simplify the verification logic in a
> later change.
>
> No behavioural change from a guests point of view.
Nevertheless I don't view this as a reasonable change on its own:
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table)
> .quad compat_update_va_mapping_otherdomain
> .quad compat_iret
> .quad compat_vcpu_op
> - .quad compat_ni_hypercall /* 25 */
> + .quad do_set_segment_base /* 25 */
This part will (I suppose) be deleted by a later change.
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1031,6 +1031,9 @@ long do_set_segment_base(unsigned int which, unsigned long base)
> struct vcpu *v = current;
> long ret = 0;
>
> + if ( is_pv_32bit_vcpu(v) )
> + return -ENOSYS; /* x86/64 only. */
And this addition could as well happen when that later change
does the re-org.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/9] x86/hypercall: Move the hypercall arg tables into C
2016-07-18 9:51 ` [PATCH 3/9] x86/hypercall: Move the hypercall arg tables into C Andrew Cooper
@ 2016-08-02 12:59 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 12:59 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> Editing (and indeed, finding) the hypercall args tables can be tricky,
> especially towards the end where .rept's are used to maintain the correct
> layout.
>
> Move this all into C, and let the compiler do the hard work. As 0 is the
> default value, drop all explicit 0's.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/9] x86/pv: Implement pv_hypercall() in C
2016-07-18 9:51 ` [PATCH 4/9] x86/pv: Implement pv_hypercall() in C Andrew Cooper
@ 2016-08-02 13:12 ` Jan Beulich
2016-08-02 14:06 ` Andrew Cooper
2016-08-11 11:57 ` Andrew Cooper
0 siblings, 2 replies; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 13:12 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> +long pv_hypercall(struct cpu_user_regs *regs)
> +{
> + struct vcpu *curr = current;
> +#ifndef NDEBUG
> + unsigned long old_rip = regs->rip;
> +#endif
> + long ret;
> + uint32_t eax = regs->eax;
> +
> + ASSERT(curr->arch.flags & TF_kernel_mode);
I'm afraid TF_kernel_mode can't be relied on for 32-bit guests, so
this needs to move into the if() below.
> + if ( (eax >= NR_hypercalls) || !hypercall_table[eax] )
> + return -ENOSYS;
> +
> + if ( !is_pv_32bit_vcpu(curr) )
> + {
> + unsigned long rdi = regs->rdi;
> + unsigned long rsi = regs->rsi;
> + unsigned long rdx = regs->rdx;
> + unsigned long r10 = regs->r10;
> + unsigned long r8 = regs->r8;
> + unsigned long r9 = regs->r9;
> +
> +#ifndef NDEBUG
> + /* Deliberately corrupt parameter regs not used by this hypercall. */
> + switch ( hypercall_args_table[eax] )
> + {
> + case 0: rdi = 0xdeadbeefdeadf00dUL;
> + case 1: rsi = 0xdeadbeefdeadf00dUL;
> + case 2: rdx = 0xdeadbeefdeadf00dUL;
> + case 3: r10 = 0xdeadbeefdeadf00dUL;
> + case 4: r8 = 0xdeadbeefdeadf00dUL;
> + case 5: r9 = 0xdeadbeefdeadf00dUL;
Without comments, aren't these going to become 5 new Coverity
issues?
> + }
> +#endif
> + if ( unlikely(tb_init_done) )
> + {
> + unsigned long args[6] = { rdi, rsi, rdx, r10, r8, r9 };
> +
> + __trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
> + }
> +
> + ret = hypercall_table[eax](rdi, rsi, rdx, r10, r8, r9);
> +
> +#ifndef NDEBUG
> + if ( regs->rip == old_rip )
> + {
> + /* Deliberately corrupt parameter regs used by this hypercall. */
> + switch ( hypercall_args_table[eax] )
> + {
> + case 6: regs->r9 = 0xdeadbeefdeadf00dUL;
> + case 5: regs->r8 = 0xdeadbeefdeadf00dUL;
> + case 4: regs->r10 = 0xdeadbeefdeadf00dUL;
> + case 3: regs->edx = 0xdeadbeefdeadf00dUL;
> + case 2: regs->esi = 0xdeadbeefdeadf00dUL;
> + case 1: regs->edi = 0xdeadbeefdeadf00dUL;
For consistency with earlier code, lease use rdx, rsi, and rdi here.
> +#ifndef NDEBUG
> + if ( regs->rip == old_rip )
> + {
> + /* Deliberately corrupt parameter regs used by this hypercall. */
> + switch ( compat_hypercall_args_table[eax] )
> + {
> + case 6: regs->ebp = 0xdeadf00d;
> + case 5: regs->edi = 0xdeadf00d;
> + case 4: regs->esi = 0xdeadf00d;
> + case 3: regs->edx = 0xdeadf00d;
> + case 2: regs->ecx = 0xdeadf00d;
> + case 1: regs->ebx = 0xdeadf00d;
Please use 32-bit stores here.
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -25,70 +25,10 @@ UNLIKELY_START(ne, msi_check)
> LOAD_C_CLOBBERED compat=1 ax=0
> UNLIKELY_END(msi_check)
>
> - movl UREGS_rax(%rsp),%eax
> GET_CURRENT(bx)
>
> - cmpl $NR_hypercalls,%eax
> - jae compat_bad_hypercall
> -#ifndef NDEBUG
> - /* Deliberately corrupt parameter regs not used by this hypercall. */
> - pushq UREGS_rbx(%rsp); pushq %rcx; pushq %rdx; pushq %rsi; pushq %rdi
> - pushq UREGS_rbp+5*8(%rsp)
> - leaq compat_hypercall_args_table(%rip),%r10
> - movl $6,%ecx
> - subb (%r10,%rax,1),%cl
> - movq %rsp,%rdi
> - movl $0xDEADBEEF,%eax
> - rep stosq
> - popq %r8 ; popq %r9 ; xchgl %r8d,%r9d /* Args 5&6: zero extend */
> - popq %rdx; popq %rcx; xchgl %edx,%ecx /* Args 3&4: zero extend */
> - popq %rdi; popq %rsi; xchgl %edi,%esi /* Args 1&2: zero extend */
> - movl UREGS_rax(%rsp),%eax
> - pushq %rax
> - pushq UREGS_rip+8(%rsp)
> -#define SHADOW_BYTES 16 /* Shadow EIP + shadow hypercall # */
> -#else
> - /* Relocate argument registers and zero-extend to 64 bits. */
> - xchgl %ecx,%esi /* Arg 2, Arg 4 */
> - movl %edx,%edx /* Arg 3 */
> - movl %edi,%r8d /* Arg 5 */
> - movl %ebp,%r9d /* Arg 6 */
> - movl UREGS_rbx(%rsp),%edi /* Arg 1 */
> -#define SHADOW_BYTES 0 /* No on-stack shadow state */
> -#endif
> - cmpb $0,tb_init_done(%rip)
> -UNLIKELY_START(ne, compat_trace)
> - call __trace_hypercall_entry
> - /* Restore the registers that __trace_hypercall_entry clobbered. */
> - movl UREGS_rax+SHADOW_BYTES(%rsp),%eax /* Hypercall # */
> - movl UREGS_rbx+SHADOW_BYTES(%rsp),%edi /* Arg 1 */
> - movl UREGS_rcx+SHADOW_BYTES(%rsp),%esi /* Arg 2 */
> - movl UREGS_rdx+SHADOW_BYTES(%rsp),%edx /* Arg 3 */
> - movl UREGS_rsi+SHADOW_BYTES(%rsp),%ecx /* Arg 4 */
> - movl UREGS_rdi+SHADOW_BYTES(%rsp),%r8d /* Arg 5 */
> - movl UREGS_rbp+SHADOW_BYTES(%rsp),%r9d /* Arg 6 */
> -#undef SHADOW_BYTES
> -UNLIKELY_END(compat_trace)
> - leaq compat_hypercall_table(%rip),%r10
> - PERFC_INCR(hypercalls, %rax, %rbx)
> - callq *(%r10,%rax,8)
> -#ifndef NDEBUG
> - /* Deliberately corrupt parameter regs used by this hypercall. */
> - popq %r10 # Shadow RIP
> - cmpq %r10,UREGS_rip+8(%rsp)
> - popq %rcx # Shadow hypercall index
> - jne compat_skip_clobber /* If RIP has changed then don't clobber. */
> - leaq compat_hypercall_args_table(%rip),%r10
> - movb (%r10,%rcx,1),%cl
> - movl $0xDEADBEEF,%r10d
> - testb %cl,%cl; jz compat_skip_clobber; movl %r10d,UREGS_rbx(%rsp)
> - cmpb $2, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rcx(%rsp)
> - cmpb $3, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdx(%rsp)
> - cmpb $4, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rsi(%rsp)
> - cmpb $5, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdi(%rsp)
> - cmpb $6, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rbp(%rsp)
> -compat_skip_clobber:
> -#endif
> + mov %rsp, %rdi
> + call pv_hypercall
> movl %eax,UREGS_rax(%rsp) # save the return value
To follow the HVM model, this should also move into C.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 12:50 ` Jan Beulich
@ 2016-08-02 13:14 ` Andrew Cooper
2016-08-02 13:28 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-08-02 13:14 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 02/08/16 13:50, Jan Beulich wrote:
>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/asm-x86/hypercall.h
>> +++ b/xen/include/asm-x86/hypercall.h
>> @@ -5,9 +5,21 @@
>> #ifndef __ASM_X86_HYPERCALL_H__
>> #define __ASM_X86_HYPERCALL_H__
>>
>> +#include <xen/types.h>
>> #include <public/physdev.h>
>> +#include <public/event_channel.h>
> Why?
You snipped the commit message, which justifies why. This header file
cannot currently be included in isolation, and I need it to be.
>
>> #include <public/arch-x86/xen-mca.h> /* for do_mca */
>> -#include <xen/types.h>
>> +
>> +typedef unsigned long hypercall_fn_t(
>> + unsigned long, unsigned long, unsigned long,
>> + unsigned long, unsigned long, unsigned long);
> Wouldn't this better go into xen/hypercall.h?
It is architecture specific.
ARM's version is
typedef register_t (*arm_hypercall_fn_t)(
register_t, register_t, register_t, register_t, register_t);
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/9] x86/hypercall: Move the hypercall tables into C
2016-07-18 9:51 ` [PATCH 5/9] x86/hypercall: Move the hypercall tables into C Andrew Cooper
@ 2016-08-02 13:23 ` Jan Beulich
2016-08-02 13:30 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 13:23 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> +hypercall_fn_t *const hypercall_table[NR_hypercalls] = {
> + HYPERCALL(set_trap_table),
> + HYPERCALL(mmu_update),
> + HYPERCALL(set_gdt),
> + HYPERCALL(stack_switch),
> + HYPERCALL(set_callbacks),
> + HYPERCALL(fpu_taskswitch),
> + HYPERCALL(sched_op_compat),
> + HYPERCALL(platform_op),
> + HYPERCALL(set_debugreg),
> + HYPERCALL(get_debugreg),
> + HYPERCALL(update_descriptor),
> + HYPERCALL(memory_op),
> + HYPERCALL(multicall),
> + HYPERCALL(update_va_mapping),
> + HYPERCALL(set_timer_op),
> + HYPERCALL(event_channel_op_compat),
> + HYPERCALL(xen_version),
> + HYPERCALL(console_io),
> + HYPERCALL(physdev_op_compat),
> + HYPERCALL(grant_table_op),
> + HYPERCALL(vm_assist),
> + HYPERCALL(update_va_mapping_otherdomain),
> + HYPERCALL(iret),
> + HYPERCALL(vcpu_op),
> + HYPERCALL(set_segment_base),
> + HYPERCALL(mmuext_op),
> + HYPERCALL(xsm_op),
> + HYPERCALL(nmi_op),
> + HYPERCALL(sched_op),
> + HYPERCALL(callback_op),
> +#ifdef CONFIG_XENOPROF
> + HYPERCALL(xenoprof_op),
> +#endif
> + HYPERCALL(event_channel_op),
> + HYPERCALL(physdev_op),
> + HYPERCALL(hvm_op),
> + HYPERCALL(sysctl),
> + HYPERCALL(domctl),
> +#ifdef CONFIG_KEXEC
> + HYPERCALL(kexec_op),
> +#endif
> +#ifdef CONFIG_TMEM
> + HYPERCALL(tmem_op),
> +#endif
To be honest I'd prefer the necessary #ifdef-ery to live in hypercall.h,
the more that then ARM could (if they want) benefit from that too.
> +hypercall_fn_t *const compat_hypercall_table[NR_hypercalls] = {
Is it really helpful to create two separate tables here, just to then
merge them in patch 7?
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -682,68 +682,6 @@ ENTRY(exception_table)
> .endr
> .size exception_table, . - exception_table
>
> -#ifndef CONFIG_KEXEC
>[...]
> - .endr
>
> /* Table of automatically generated entry points. One per vector. */
Please don't leave behind two blank lines.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests
2016-08-02 12:52 ` Jan Beulich
@ 2016-08-02 13:25 ` Andrew Cooper
2016-08-02 13:31 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-08-02 13:25 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 02/08/16 13:52, Jan Beulich wrote:
>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> set_segment_base is the only hypercall exists in only one of the two modes
>> guests might run in; all other hypercalls are either implemented, or
>> unimplemented in both modes.
>>
>> Remove this split, by allowing do_set_segment_base() to be called in the
>> compat hypercall path. This change will simplify the verification logic in a
>> later change.
>>
>> No behavioural change from a guests point of view.
> Nevertheless I don't view this as a reasonable change on its own:
Why not? It is far better to call it out in isolation, than to mix it
in with an already-existing complicated change.
>
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table)
>> .quad compat_update_va_mapping_otherdomain
>> .quad compat_iret
>> .quad compat_vcpu_op
>> - .quad compat_ni_hypercall /* 25 */
>> + .quad do_set_segment_base /* 25 */
> This part will (I suppose) be deleted by a later change.
When moved into C, it will be hidden behind the macros used to construct
both table values at once.
>
>> --- a/xen/arch/x86/x86_64/mm.c
>> +++ b/xen/arch/x86/x86_64/mm.c
>> @@ -1031,6 +1031,9 @@ long do_set_segment_base(unsigned int which, unsigned long base)
>> struct vcpu *v = current;
>> long ret = 0;
>>
>> + if ( is_pv_32bit_vcpu(v) )
>> + return -ENOSYS; /* x86/64 only. */
> And this addition could as well happen when that later change
> does the re-org.
Again, that would further complicate an already-complicated patch.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 13:14 ` Andrew Cooper
@ 2016-08-02 13:28 ` Jan Beulich
2016-08-02 14:04 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 13:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.08.16 at 15:14, <andrew.cooper3@citrix.com> wrote:
> On 02/08/16 13:50, Jan Beulich wrote:
>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/include/asm-x86/hypercall.h
>>> +++ b/xen/include/asm-x86/hypercall.h
>>> @@ -5,9 +5,21 @@
>>> #ifndef __ASM_X86_HYPERCALL_H__
>>> #define __ASM_X86_HYPERCALL_H__
>>>
>>> +#include <xen/types.h>
>>> #include <public/physdev.h>
>>> +#include <public/event_channel.h>
>> Why?
>
> You snipped the commit message, which justifies why. This header file
> cannot currently be included in isolation, and I need it to be.
Ah, that sentence there also relates to this addition.
>>> #include <public/arch-x86/xen-mca.h> /* for do_mca */
>>> -#include <xen/types.h>
>>> +
>>> +typedef unsigned long hypercall_fn_t(
>>> + unsigned long, unsigned long, unsigned long,
>>> + unsigned long, unsigned long, unsigned long);
>> Wouldn't this better go into xen/hypercall.h?
>
> It is architecture specific.
>
> ARM's version is
>
> typedef register_t (*arm_hypercall_fn_t)(
> register_t, register_t, register_t, register_t, register_t);
Which is bogus - they're lucky we so far don't have any 6-argument
hypercalls. Or the other way around - we could limit hypercalls to
just five arguments (for now) on x86 too, allowing things to get
unified. Anyway - that probably goes too far right now, so feel free
to add my ack to the patch.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/9] x86/hypercall: Move the hypercall tables into C
2016-08-02 13:23 ` Jan Beulich
@ 2016-08-02 13:30 ` Andrew Cooper
2016-08-02 13:40 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-08-02 13:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 02/08/16 14:23, Jan Beulich wrote:
>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> +hypercall_fn_t *const hypercall_table[NR_hypercalls] = {
>> + HYPERCALL(set_trap_table),
>> + HYPERCALL(mmu_update),
>> + HYPERCALL(set_gdt),
>> + HYPERCALL(stack_switch),
>> + HYPERCALL(set_callbacks),
>> + HYPERCALL(fpu_taskswitch),
>> + HYPERCALL(sched_op_compat),
>> + HYPERCALL(platform_op),
>> + HYPERCALL(set_debugreg),
>> + HYPERCALL(get_debugreg),
>> + HYPERCALL(update_descriptor),
>> + HYPERCALL(memory_op),
>> + HYPERCALL(multicall),
>> + HYPERCALL(update_va_mapping),
>> + HYPERCALL(set_timer_op),
>> + HYPERCALL(event_channel_op_compat),
>> + HYPERCALL(xen_version),
>> + HYPERCALL(console_io),
>> + HYPERCALL(physdev_op_compat),
>> + HYPERCALL(grant_table_op),
>> + HYPERCALL(vm_assist),
>> + HYPERCALL(update_va_mapping_otherdomain),
>> + HYPERCALL(iret),
>> + HYPERCALL(vcpu_op),
>> + HYPERCALL(set_segment_base),
>> + HYPERCALL(mmuext_op),
>> + HYPERCALL(xsm_op),
>> + HYPERCALL(nmi_op),
>> + HYPERCALL(sched_op),
>> + HYPERCALL(callback_op),
>> +#ifdef CONFIG_XENOPROF
>> + HYPERCALL(xenoprof_op),
>> +#endif
>> + HYPERCALL(event_channel_op),
>> + HYPERCALL(physdev_op),
>> + HYPERCALL(hvm_op),
>> + HYPERCALL(sysctl),
>> + HYPERCALL(domctl),
>> +#ifdef CONFIG_KEXEC
>> + HYPERCALL(kexec_op),
>> +#endif
>> +#ifdef CONFIG_TMEM
>> + HYPERCALL(tmem_op),
>> +#endif
> To be honest I'd prefer the necessary #ifdef-ery to live in hypercall.h,
> the more that then ARM could (if they want) benefit from that too.
Which #ifdefary?
HYPERCALL() can't be used by ARM.
>
>> +hypercall_fn_t *const compat_hypercall_table[NR_hypercalls] = {
> Is it really helpful to create two separate tables here, just to then
> merge them in patch 7?
This is necessary, so the untangling of multicalls can happen in patch 6.
The only other bisectable option is to merge patches 5-7, which is
definitely too complicated to review.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests
2016-08-02 13:25 ` Andrew Cooper
@ 2016-08-02 13:31 ` Jan Beulich
2016-08-02 13:39 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 13:31 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.08.16 at 15:25, <andrew.cooper3@citrix.com> wrote:
> On 02/08/16 13:52, Jan Beulich wrote:
>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> set_segment_base is the only hypercall exists in only one of the two modes
>>> guests might run in; all other hypercalls are either implemented, or
>>> unimplemented in both modes.
>>>
>>> Remove this split, by allowing do_set_segment_base() to be called in the
>>> compat hypercall path. This change will simplify the verification logic in a
>>> later change.
>>>
>>> No behavioural change from a guests point of view.
>> Nevertheless I don't view this as a reasonable change on its own:
>
> Why not? It is far better to call it out in isolation, than to mix it
> in with an already-existing complicated change.
If the changes here were anywhere near being themselves
complicated, I'd agree.
>>> --- a/xen/arch/x86/x86_64/compat/entry.S
>>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>>> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table)
>>> .quad compat_update_va_mapping_otherdomain
>>> .quad compat_iret
>>> .quad compat_vcpu_op
>>> - .quad compat_ni_hypercall /* 25 */
>>> + .quad do_set_segment_base /* 25 */
>> This part will (I suppose) be deleted by a later change.
>
> When moved into C, it will be hidden behind the macros used to construct
> both table values at once.
And compat_set_segment_base could then just be #define-d to NULL.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests
2016-08-02 13:31 ` Jan Beulich
@ 2016-08-02 13:39 ` Andrew Cooper
2016-08-02 13:47 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-08-02 13:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 02/08/16 14:31, Jan Beulich wrote:
>>>> On 02.08.16 at 15:25, <andrew.cooper3@citrix.com> wrote:
>> On 02/08/16 13:52, Jan Beulich wrote:
>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>> set_segment_base is the only hypercall exists in only one of the two modes
>>>> guests might run in; all other hypercalls are either implemented, or
>>>> unimplemented in both modes.
>>>>
>>>> Remove this split, by allowing do_set_segment_base() to be called in the
>>>> compat hypercall path. This change will simplify the verification logic in a
>>>> later change.
>>>>
>>>> No behavioural change from a guests point of view.
>>> Nevertheless I don't view this as a reasonable change on its own:
>> Why not? It is far better to call it out in isolation, than to mix it
>> in with an already-existing complicated change.
> If the changes here were anywhere near being themselves
> complicated, I'd agree.
>
>>>> --- a/xen/arch/x86/x86_64/compat/entry.S
>>>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>>>> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table)
>>>> .quad compat_update_va_mapping_otherdomain
>>>> .quad compat_iret
>>>> .quad compat_vcpu_op
>>>> - .quad compat_ni_hypercall /* 25 */
>>>> + .quad do_set_segment_base /* 25 */
>>> This part will (I suppose) be deleted by a later change.
>> When moved into C, it will be hidden behind the macros used to construct
>> both table values at once.
> And compat_set_segment_base could then just be #define-d to NULL.
The entire purpose of making this change is so we don't end up with NULL
in one half a pair of function pointers, and have to extend the runtime
logic to check different pointers.
This is the only hypercall with this problem, so I have opted to make it
not a problem, rather than causing an extra runtime hit on all hypercalls.
A 32bit PV guest doesn't have any reason to make this hypercall. It it
does, I really don't care that it takes a few instructions extra to
return -ENOSYS.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/9] x86/hypercall: Move the hypercall tables into C
2016-08-02 13:30 ` Andrew Cooper
@ 2016-08-02 13:40 ` Jan Beulich
2016-08-11 12:00 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 13:40 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.08.16 at 15:30, <andrew.cooper3@citrix.com> wrote:
> On 02/08/16 14:23, Jan Beulich wrote:
>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> +hypercall_fn_t *const hypercall_table[NR_hypercalls] = {
>>> + HYPERCALL(set_trap_table),
>>> + HYPERCALL(mmu_update),
>>> + HYPERCALL(set_gdt),
>>> + HYPERCALL(stack_switch),
>>> + HYPERCALL(set_callbacks),
>>> + HYPERCALL(fpu_taskswitch),
>>> + HYPERCALL(sched_op_compat),
>>> + HYPERCALL(platform_op),
>>> + HYPERCALL(set_debugreg),
>>> + HYPERCALL(get_debugreg),
>>> + HYPERCALL(update_descriptor),
>>> + HYPERCALL(memory_op),
>>> + HYPERCALL(multicall),
>>> + HYPERCALL(update_va_mapping),
>>> + HYPERCALL(set_timer_op),
>>> + HYPERCALL(event_channel_op_compat),
>>> + HYPERCALL(xen_version),
>>> + HYPERCALL(console_io),
>>> + HYPERCALL(physdev_op_compat),
>>> + HYPERCALL(grant_table_op),
>>> + HYPERCALL(vm_assist),
>>> + HYPERCALL(update_va_mapping_otherdomain),
>>> + HYPERCALL(iret),
>>> + HYPERCALL(vcpu_op),
>>> + HYPERCALL(set_segment_base),
>>> + HYPERCALL(mmuext_op),
>>> + HYPERCALL(xsm_op),
>>> + HYPERCALL(nmi_op),
>>> + HYPERCALL(sched_op),
>>> + HYPERCALL(callback_op),
>>> +#ifdef CONFIG_XENOPROF
>>> + HYPERCALL(xenoprof_op),
>>> +#endif
>>> + HYPERCALL(event_channel_op),
>>> + HYPERCALL(physdev_op),
>>> + HYPERCALL(hvm_op),
>>> + HYPERCALL(sysctl),
>>> + HYPERCALL(domctl),
>>> +#ifdef CONFIG_KEXEC
>>> + HYPERCALL(kexec_op),
>>> +#endif
>>> +#ifdef CONFIG_TMEM
>>> + HYPERCALL(tmem_op),
>>> +#endif
>> To be honest I'd prefer the necessary #ifdef-ery to live in hypercall.h,
>> the more that then ARM could (if they want) benefit from that too.
>
> Which #ifdefary?
>
> HYPERCALL() can't be used by ARM.
I mean just the #ifdef-s above, not the HYPERCALL() lines. Clearly
you can do
#ifndef CONFIG_TMEM
# define do_tmem_op NULL
#endif
and alike in the header?
>>> +hypercall_fn_t *const compat_hypercall_table[NR_hypercalls] = {
>> Is it really helpful to create two separate tables here, just to then
>> merge them in patch 7?
>
> This is necessary, so the untangling of multicalls can happen in patch 6.
>
> The only other bisectable option is to merge patches 5-7, which is
> definitely too complicated to review.
Okay, I guess I'll have to look at 6 before being able to judge.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests
2016-08-02 13:39 ` Andrew Cooper
@ 2016-08-02 13:47 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 13:47 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.08.16 at 15:39, <andrew.cooper3@citrix.com> wrote:
> On 02/08/16 14:31, Jan Beulich wrote:
>>>>> On 02.08.16 at 15:25, <andrew.cooper3@citrix.com> wrote:
>>> On 02/08/16 13:52, Jan Beulich wrote:
>>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/x86_64/compat/entry.S
>>>>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>>>>> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table)
>>>>> .quad compat_update_va_mapping_otherdomain
>>>>> .quad compat_iret
>>>>> .quad compat_vcpu_op
>>>>> - .quad compat_ni_hypercall /* 25 */
>>>>> + .quad do_set_segment_base /* 25 */
>>>> This part will (I suppose) be deleted by a later change.
>>> When moved into C, it will be hidden behind the macros used to construct
>>> both table values at once.
>> And compat_set_segment_base could then just be #define-d to NULL.
>
> The entire purpose of making this change is so we don't end up with NULL
> in one half a pair of function pointers, and have to extend the runtime
> logic to check different pointers.
Well, okay then - feel free to add by ack.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 13:28 ` Jan Beulich
@ 2016-08-02 14:04 ` Julien Grall
2016-08-02 14:17 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2016-08-02 14:04 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper; +Cc: Stefano Stabellini, Xen-devel
Hi Jan,
On 02/08/16 14:28, Jan Beulich wrote:
>>>> On 02.08.16 at 15:14, <andrew.cooper3@citrix.com> wrote:
>> On 02/08/16 13:50, Jan Beulich wrote:
>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/include/asm-x86/hypercall.h
>>>> +++ b/xen/include/asm-x86/hypercall.h
>>>> @@ -5,9 +5,21 @@
>>>> #ifndef __ASM_X86_HYPERCALL_H__
>>>> #define __ASM_X86_HYPERCALL_H__
>>>>
>>>> +#include <xen/types.h>
>>>> #include <public/physdev.h>
>>>> +#include <public/event_channel.h>
>>> Why?
>>
>> You snipped the commit message, which justifies why. This header file
>> cannot currently be included in isolation, and I need it to be.
>
> Ah, that sentence there also relates to this addition.
>
>>>> #include <public/arch-x86/xen-mca.h> /* for do_mca */
>>>> -#include <xen/types.h>
>>>> +
>>>> +typedef unsigned long hypercall_fn_t(
>>>> + unsigned long, unsigned long, unsigned long,
>>>> + unsigned long, unsigned long, unsigned long);
>>> Wouldn't this better go into xen/hypercall.h?
>>
>> It is architecture specific.
>>
>> ARM's version is
>>
>> typedef register_t (*arm_hypercall_fn_t)(
>> register_t, register_t, register_t, register_t, register_t);
>
> Which is bogus - they're lucky we so far don't have any 6-argument
> hypercalls. Or the other way around - we could limit hypercalls to
> just five arguments (for now) on x86 too, allowing things to get
> unified. Anyway - that probably goes too far right now, so feel free
> to add my ack to the patch.
I am not sure why you think we are lucky on ARM. The hypercall has been
defined on ARM to support up to 5 arguments (public/arch-arm.h):
"A hypercall can take up to 5 arguments. These are passed in
registers, the first argument in x0/r0 (for arm64/arm32 guests
respectively irrespective of whether the underlying hypervisor is
32- or 64-bit), the second argument in x1/r1, the third in x2/r2,
the forth in x3/r3 and the fifth in x4/r4."
So the prototype matches the ABI.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/9] x86/pv: Implement pv_hypercall() in C
2016-08-02 13:12 ` Jan Beulich
@ 2016-08-02 14:06 ` Andrew Cooper
2016-08-02 14:19 ` Jan Beulich
2016-08-11 11:57 ` Andrew Cooper
1 sibling, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-08-02 14:06 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 02/08/16 14:12, Jan Beulich wrote:
>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> +long pv_hypercall(struct cpu_user_regs *regs)
>> +{
>> + struct vcpu *curr = current;
>> +#ifndef NDEBUG
>> + unsigned long old_rip = regs->rip;
>> +#endif
>> + long ret;
>> + uint32_t eax = regs->eax;
>> +
>> + ASSERT(curr->arch.flags & TF_kernel_mode);
> I'm afraid TF_kernel_mode can't be relied on for 32-bit guests, so
> this needs to move into the if() below.
In which case it should become ASSERT(guest_mode_kernel(curr, regs))
>
>> + if ( (eax >= NR_hypercalls) || !hypercall_table[eax] )
>> + return -ENOSYS;
>> +
>> + if ( !is_pv_32bit_vcpu(curr) )
>> + {
>> + unsigned long rdi = regs->rdi;
>> + unsigned long rsi = regs->rsi;
>> + unsigned long rdx = regs->rdx;
>> + unsigned long r10 = regs->r10;
>> + unsigned long r8 = regs->r8;
>> + unsigned long r9 = regs->r9;
>> +
>> +#ifndef NDEBUG
>> + /* Deliberately corrupt parameter regs not used by this hypercall. */
>> + switch ( hypercall_args_table[eax] )
>> + {
>> + case 0: rdi = 0xdeadbeefdeadf00dUL;
>> + case 1: rsi = 0xdeadbeefdeadf00dUL;
>> + case 2: rdx = 0xdeadbeefdeadf00dUL;
>> + case 3: r10 = 0xdeadbeefdeadf00dUL;
>> + case 4: r8 = 0xdeadbeefdeadf00dUL;
>> + case 5: r9 = 0xdeadbeefdeadf00dUL;
> Without comments, aren't these going to become 5 new Coverity
> issues?
There are no current warnings from the HVM side, so I doubt it.
Coverities' logic is rather complicated, but in this case I think the
lack of any break statements at all is a sufficient hint that its fine.
>
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -25,70 +25,10 @@ UNLIKELY_START(ne, msi_check)
>> LOAD_C_CLOBBERED compat=1 ax=0
>> UNLIKELY_END(msi_check)
>>
>> - movl UREGS_rax(%rsp),%eax
>> GET_CURRENT(bx)
>>
>> - cmpl $NR_hypercalls,%eax
>> - jae compat_bad_hypercall
>> -#ifndef NDEBUG
>> - /* Deliberately corrupt parameter regs not used by this hypercall. */
>> - pushq UREGS_rbx(%rsp); pushq %rcx; pushq %rdx; pushq %rsi; pushq %rdi
>> - pushq UREGS_rbp+5*8(%rsp)
>> - leaq compat_hypercall_args_table(%rip),%r10
>> - movl $6,%ecx
>> - subb (%r10,%rax,1),%cl
>> - movq %rsp,%rdi
>> - movl $0xDEADBEEF,%eax
>> - rep stosq
>> - popq %r8 ; popq %r9 ; xchgl %r8d,%r9d /* Args 5&6: zero extend */
>> - popq %rdx; popq %rcx; xchgl %edx,%ecx /* Args 3&4: zero extend */
>> - popq %rdi; popq %rsi; xchgl %edi,%esi /* Args 1&2: zero extend */
>> - movl UREGS_rax(%rsp),%eax
>> - pushq %rax
>> - pushq UREGS_rip+8(%rsp)
>> -#define SHADOW_BYTES 16 /* Shadow EIP + shadow hypercall # */
>> -#else
>> - /* Relocate argument registers and zero-extend to 64 bits. */
>> - xchgl %ecx,%esi /* Arg 2, Arg 4 */
>> - movl %edx,%edx /* Arg 3 */
>> - movl %edi,%r8d /* Arg 5 */
>> - movl %ebp,%r9d /* Arg 6 */
>> - movl UREGS_rbx(%rsp),%edi /* Arg 1 */
>> -#define SHADOW_BYTES 0 /* No on-stack shadow state */
>> -#endif
>> - cmpb $0,tb_init_done(%rip)
>> -UNLIKELY_START(ne, compat_trace)
>> - call __trace_hypercall_entry
>> - /* Restore the registers that __trace_hypercall_entry clobbered. */
>> - movl UREGS_rax+SHADOW_BYTES(%rsp),%eax /* Hypercall # */
>> - movl UREGS_rbx+SHADOW_BYTES(%rsp),%edi /* Arg 1 */
>> - movl UREGS_rcx+SHADOW_BYTES(%rsp),%esi /* Arg 2 */
>> - movl UREGS_rdx+SHADOW_BYTES(%rsp),%edx /* Arg 3 */
>> - movl UREGS_rsi+SHADOW_BYTES(%rsp),%ecx /* Arg 4 */
>> - movl UREGS_rdi+SHADOW_BYTES(%rsp),%r8d /* Arg 5 */
>> - movl UREGS_rbp+SHADOW_BYTES(%rsp),%r9d /* Arg 6 */
>> -#undef SHADOW_BYTES
>> -UNLIKELY_END(compat_trace)
>> - leaq compat_hypercall_table(%rip),%r10
>> - PERFC_INCR(hypercalls, %rax, %rbx)
>> - callq *(%r10,%rax,8)
>> -#ifndef NDEBUG
>> - /* Deliberately corrupt parameter regs used by this hypercall. */
>> - popq %r10 # Shadow RIP
>> - cmpq %r10,UREGS_rip+8(%rsp)
>> - popq %rcx # Shadow hypercall index
>> - jne compat_skip_clobber /* If RIP has changed then don't clobber. */
>> - leaq compat_hypercall_args_table(%rip),%r10
>> - movb (%r10,%rcx,1),%cl
>> - movl $0xDEADBEEF,%r10d
>> - testb %cl,%cl; jz compat_skip_clobber; movl %r10d,UREGS_rbx(%rsp)
>> - cmpb $2, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rcx(%rsp)
>> - cmpb $3, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdx(%rsp)
>> - cmpb $4, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rsi(%rsp)
>> - cmpb $5, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdi(%rsp)
>> - cmpb $6, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rbp(%rsp)
>> -compat_skip_clobber:
>> -#endif
>> + mov %rsp, %rdi
>> + call pv_hypercall
>> movl %eax,UREGS_rax(%rsp) # save the return value
> To follow the HVM model, this should also move into C.
I can do for now.
I haven't quite decided yet whether it would be sensible simplify the
continuation logic in the tail end of C, or by using -ERESTART at this
point.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 14:04 ` Julien Grall
@ 2016-08-02 14:17 ` Jan Beulich
2016-08-02 14:26 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 14:17 UTC (permalink / raw)
To: Julien Grall; +Cc: Andrew Cooper, Stefano Stabellini, Xen-devel
>>> On 02.08.16 at 16:04, <julien.grall@arm.com> wrote:
> On 02/08/16 14:28, Jan Beulich wrote:
>>>>> On 02.08.16 at 15:14, <andrew.cooper3@citrix.com> wrote:
>>> On 02/08/16 13:50, Jan Beulich wrote:
>>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/include/asm-x86/hypercall.h
>>>>> +++ b/xen/include/asm-x86/hypercall.h
>>>>> @@ -5,9 +5,21 @@
>>>>> #ifndef __ASM_X86_HYPERCALL_H__
>>>>> #define __ASM_X86_HYPERCALL_H__
>>>>>
>>>>> +#include <xen/types.h>
>>>>> #include <public/physdev.h>
>>>>> +#include <public/event_channel.h>
>>>> Why?
>>>
>>> You snipped the commit message, which justifies why. This header file
>>> cannot currently be included in isolation, and I need it to be.
>>
>> Ah, that sentence there also relates to this addition.
>>
>>>>> #include <public/arch-x86/xen-mca.h> /* for do_mca */
>>>>> -#include <xen/types.h>
>>>>> +
>>>>> +typedef unsigned long hypercall_fn_t(
>>>>> + unsigned long, unsigned long, unsigned long,
>>>>> + unsigned long, unsigned long, unsigned long);
>>>> Wouldn't this better go into xen/hypercall.h?
>>>
>>> It is architecture specific.
>>>
>>> ARM's version is
>>>
>>> typedef register_t (*arm_hypercall_fn_t)(
>>> register_t, register_t, register_t, register_t, register_t);
>>
>> Which is bogus - they're lucky we so far don't have any 6-argument
>> hypercalls. Or the other way around - we could limit hypercalls to
>> just five arguments (for now) on x86 too, allowing things to get
>> unified. Anyway - that probably goes too far right now, so feel free
>> to add my ack to the patch.
>
> I am not sure why you think we are lucky on ARM. The hypercall has been
> defined on ARM to support up to 5 arguments (public/arch-arm.h):
>
> "A hypercall can take up to 5 arguments. These are passed in
> registers, the first argument in x0/r0 (for arm64/arm32 guests
> respectively irrespective of whether the underlying hypervisor is
> 32- or 64-bit), the second argument in x1/r1, the third in x2/r2,
> the forth in x3/r3 and the fifth in x4/r4."
>
> So the prototype matches the ABI.
Well, I find it quite odd for hypercall argument counts to differ
between arches. I.e. I'd conclude the ABI was mis-specified.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/9] x86/pv: Implement pv_hypercall() in C
2016-08-02 14:06 ` Andrew Cooper
@ 2016-08-02 14:19 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 14:19 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.08.16 at 16:06, <andrew.cooper3@citrix.com> wrote:
> On 02/08/16 14:12, Jan Beulich wrote:
>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> +long pv_hypercall(struct cpu_user_regs *regs)
>>> +{
>>> + struct vcpu *curr = current;
>>> +#ifndef NDEBUG
>>> + unsigned long old_rip = regs->rip;
>>> +#endif
>>> + long ret;
>>> + uint32_t eax = regs->eax;
>>> +
>>> + ASSERT(curr->arch.flags & TF_kernel_mode);
>> I'm afraid TF_kernel_mode can't be relied on for 32-bit guests, so
>> this needs to move into the if() below.
>
> In which case it should become ASSERT(guest_mode_kernel(curr, regs))
Ah, yes.
>>> + if ( (eax >= NR_hypercalls) || !hypercall_table[eax] )
>>> + return -ENOSYS;
>>> +
>>> + if ( !is_pv_32bit_vcpu(curr) )
>>> + {
>>> + unsigned long rdi = regs->rdi;
>>> + unsigned long rsi = regs->rsi;
>>> + unsigned long rdx = regs->rdx;
>>> + unsigned long r10 = regs->r10;
>>> + unsigned long r8 = regs->r8;
>>> + unsigned long r9 = regs->r9;
>>> +
>>> +#ifndef NDEBUG
>>> + /* Deliberately corrupt parameter regs not used by this hypercall. */
>>> + switch ( hypercall_args_table[eax] )
>>> + {
>>> + case 0: rdi = 0xdeadbeefdeadf00dUL;
>>> + case 1: rsi = 0xdeadbeefdeadf00dUL;
>>> + case 2: rdx = 0xdeadbeefdeadf00dUL;
>>> + case 3: r10 = 0xdeadbeefdeadf00dUL;
>>> + case 4: r8 = 0xdeadbeefdeadf00dUL;
>>> + case 5: r9 = 0xdeadbeefdeadf00dUL;
>> Without comments, aren't these going to become 5 new Coverity
>> issues?
>
> There are no current warnings from the HVM side, so I doubt it.
> Coverities' logic is rather complicated, but in this case I think the
> lack of any break statements at all is a sufficient hint that its fine.
Okay.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 14:17 ` Jan Beulich
@ 2016-08-02 14:26 ` Julien Grall
2016-08-02 14:54 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2016-08-02 14:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Stefano Stabellini, Xen-devel
On 02/08/16 15:17, Jan Beulich wrote:
>>>> On 02.08.16 at 16:04, <julien.grall@arm.com> wrote:
>> On 02/08/16 14:28, Jan Beulich wrote:
>>>>>> On 02.08.16 at 15:14, <andrew.cooper3@citrix.com> wrote:
>>>> On 02/08/16 13:50, Jan Beulich wrote:
>>>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/xen/include/asm-x86/hypercall.h
>>>>>> +++ b/xen/include/asm-x86/hypercall.h
>>>>>> @@ -5,9 +5,21 @@
>>>>>> #ifndef __ASM_X86_HYPERCALL_H__
>>>>>> #define __ASM_X86_HYPERCALL_H__
>>>>>>
>>>>>> +#include <xen/types.h>
>>>>>> #include <public/physdev.h>
>>>>>> +#include <public/event_channel.h>
>>>>> Why?
>>>>
>>>> You snipped the commit message, which justifies why. This header file
>>>> cannot currently be included in isolation, and I need it to be.
>>>
>>> Ah, that sentence there also relates to this addition.
>>>
>>>>>> #include <public/arch-x86/xen-mca.h> /* for do_mca */
>>>>>> -#include <xen/types.h>
>>>>>> +
>>>>>> +typedef unsigned long hypercall_fn_t(
>>>>>> + unsigned long, unsigned long, unsigned long,
>>>>>> + unsigned long, unsigned long, unsigned long);
>>>>> Wouldn't this better go into xen/hypercall.h?
>>>>
>>>> It is architecture specific.
>>>>
>>>> ARM's version is
>>>>
>>>> typedef register_t (*arm_hypercall_fn_t)(
>>>> register_t, register_t, register_t, register_t, register_t);
>>>
>>> Which is bogus - they're lucky we so far don't have any 6-argument
>>> hypercalls. Or the other way around - we could limit hypercalls to
>>> just five arguments (for now) on x86 too, allowing things to get
>>> unified. Anyway - that probably goes too far right now, so feel free
>>> to add my ack to the patch.
>>
>> I am not sure why you think we are lucky on ARM. The hypercall has been
>> defined on ARM to support up to 5 arguments (public/arch-arm.h):
>>
>> "A hypercall can take up to 5 arguments. These are passed in
>> registers, the first argument in x0/r0 (for arm64/arm32 guests
>> respectively irrespective of whether the underlying hypervisor is
>> 32- or 64-bit), the second argument in x1/r1, the third in x2/r2,
>> the forth in x3/r3 and the fifth in x4/r4."
>>
>> So the prototype matches the ABI.
>
> Well, I find it quite odd for hypercall argument counts to differ
> between arches. I.e. I'd conclude the ABI was mis-specified.
Is it documented somewhere for the x86 code? Looking at Linux, the
privcmd call is only passing 5 arguments on both ARM and x86.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 14:26 ` Julien Grall
@ 2016-08-02 14:54 ` Jan Beulich
2016-08-02 14:59 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 14:54 UTC (permalink / raw)
To: Julien Grall; +Cc: Andrew Cooper, Stefano Stabellini, Xen-devel
>>> On 02.08.16 at 16:26, <julien.grall@arm.com> wrote:
>
> On 02/08/16 15:17, Jan Beulich wrote:
>>>>> On 02.08.16 at 16:04, <julien.grall@arm.com> wrote:
>>> On 02/08/16 14:28, Jan Beulich wrote:
>>>>>>> On 02.08.16 at 15:14, <andrew.cooper3@citrix.com> wrote:
>>>>> On 02/08/16 13:50, Jan Beulich wrote:
>>>>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>>>> #include <public/arch-x86/xen-mca.h> /* for do_mca */
>>>>>>> -#include <xen/types.h>
>>>>>>> +
>>>>>>> +typedef unsigned long hypercall_fn_t(
>>>>>>> + unsigned long, unsigned long, unsigned long,
>>>>>>> + unsigned long, unsigned long, unsigned long);
>>>>>> Wouldn't this better go into xen/hypercall.h?
>>>>>
>>>>> It is architecture specific.
>>>>>
>>>>> ARM's version is
>>>>>
>>>>> typedef register_t (*arm_hypercall_fn_t)(
>>>>> register_t, register_t, register_t, register_t, register_t);
>>>>
>>>> Which is bogus - they're lucky we so far don't have any 6-argument
>>>> hypercalls. Or the other way around - we could limit hypercalls to
>>>> just five arguments (for now) on x86 too, allowing things to get
>>>> unified. Anyway - that probably goes too far right now, so feel free
>>>> to add my ack to the patch.
>>>
>>> I am not sure why you think we are lucky on ARM. The hypercall has been
>>> defined on ARM to support up to 5 arguments (public/arch-arm.h):
>>>
>>> "A hypercall can take up to 5 arguments. These are passed in
>>> registers, the first argument in x0/r0 (for arm64/arm32 guests
>>> respectively irrespective of whether the underlying hypervisor is
>>> 32- or 64-bit), the second argument in x1/r1, the third in x2/r2,
>>> the forth in x3/r3 and the fifth in x4/r4."
>>>
>>> So the prototype matches the ABI.
>>
>> Well, I find it quite odd for hypercall argument counts to differ
>> between arches. I.e. I'd conclude the ABI was mis-specified.
>
> Is it documented somewhere for the x86 code? Looking at Linux, the
> privcmd call is only passing 5 arguments on both ARM and x86.
arch-x86/xen-x86_32.h has
* Hypercall interface:
* Input: %ebx, %ecx, %edx, %esi, %edi, %ebp (arguments 1-6)
* Output: %eax
while arch-x86/xen-x86_64.h has
* Hypercall interface:
* Input: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
* Output: %rax
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 14:54 ` Jan Beulich
@ 2016-08-02 14:59 ` Andrew Cooper
2016-08-02 15:05 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-08-02 14:59 UTC (permalink / raw)
To: Jan Beulich, Julien Grall; +Cc: Stefano Stabellini, Xen-devel
On 02/08/16 15:54, Jan Beulich wrote:
>>>> On 02.08.16 at 16:26, <julien.grall@arm.com> wrote:
>> On 02/08/16 15:17, Jan Beulich wrote:
>>>>>> On 02.08.16 at 16:04, <julien.grall@arm.com> wrote:
>>>> On 02/08/16 14:28, Jan Beulich wrote:
>>>>>>>> On 02.08.16 at 15:14, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 02/08/16 13:50, Jan Beulich wrote:
>>>>>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>>>>> #include <public/arch-x86/xen-mca.h> /* for do_mca */
>>>>>>>> -#include <xen/types.h>
>>>>>>>> +
>>>>>>>> +typedef unsigned long hypercall_fn_t(
>>>>>>>> + unsigned long, unsigned long, unsigned long,
>>>>>>>> + unsigned long, unsigned long, unsigned long);
>>>>>>> Wouldn't this better go into xen/hypercall.h?
>>>>>> It is architecture specific.
>>>>>>
>>>>>> ARM's version is
>>>>>>
>>>>>> typedef register_t (*arm_hypercall_fn_t)(
>>>>>> register_t, register_t, register_t, register_t, register_t);
>>>>> Which is bogus - they're lucky we so far don't have any 6-argument
>>>>> hypercalls. Or the other way around - we could limit hypercalls to
>>>>> just five arguments (for now) on x86 too, allowing things to get
>>>>> unified. Anyway - that probably goes too far right now, so feel free
>>>>> to add my ack to the patch.
>>>> I am not sure why you think we are lucky on ARM. The hypercall has been
>>>> defined on ARM to support up to 5 arguments (public/arch-arm.h):
>>>>
>>>> "A hypercall can take up to 5 arguments. These are passed in
>>>> registers, the first argument in x0/r0 (for arm64/arm32 guests
>>>> respectively irrespective of whether the underlying hypervisor is
>>>> 32- or 64-bit), the second argument in x1/r1, the third in x2/r2,
>>>> the forth in x3/r3 and the fifth in x4/r4."
>>>>
>>>> So the prototype matches the ABI.
>>> Well, I find it quite odd for hypercall argument counts to differ
>>> between arches. I.e. I'd conclude the ABI was mis-specified.
>> Is it documented somewhere for the x86 code? Looking at Linux, the
>> privcmd call is only passing 5 arguments on both ARM and x86.
> arch-x86/xen-x86_32.h has
>
> * Hypercall interface:
> * Input: %ebx, %ecx, %edx, %esi, %edi, %ebp (arguments 1-6)
> * Output: %eax
>
> while arch-x86/xen-x86_64.h has
>
> * Hypercall interface:
> * Input: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
> * Output: %rax
The only actual 6 argument hypercall is the v4v hypercall, better known
as __HYPERVISOR_xc_reserved_op at index 39, but that isn't implemented
anywhere upstream.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 14:59 ` Andrew Cooper
@ 2016-08-02 15:05 ` Jan Beulich
2016-08-02 18:43 ` Stefano Stabellini
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-02 15:05 UTC (permalink / raw)
To: Julien Grall, Andrew Cooper; +Cc: Stefano Stabellini, Xen-devel
>>> On 02.08.16 at 16:59, <andrew.cooper3@citrix.com> wrote:
> On 02/08/16 15:54, Jan Beulich wrote:
>>>>> On 02.08.16 at 16:26, <julien.grall@arm.com> wrote:
>>> On 02/08/16 15:17, Jan Beulich wrote:
>>>> Well, I find it quite odd for hypercall argument counts to differ
>>>> between arches. I.e. I'd conclude the ABI was mis-specified.
>>> Is it documented somewhere for the x86 code? Looking at Linux, the
>>> privcmd call is only passing 5 arguments on both ARM and x86.
>> arch-x86/xen-x86_32.h has
>>
>> * Hypercall interface:
>> * Input: %ebx, %ecx, %edx, %esi, %edi, %ebp (arguments 1-6)
>> * Output: %eax
>>
>> while arch-x86/xen-x86_64.h has
>>
>> * Hypercall interface:
>> * Input: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
>> * Output: %rax
>
> The only actual 6 argument hypercall is the v4v hypercall, better known
> as __HYPERVISOR_xc_reserved_op at index 39, but that isn't implemented
> anywhere upstream.
But it serves as an example what now wouldn't work on ARM.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 15:05 ` Jan Beulich
@ 2016-08-02 18:43 ` Stefano Stabellini
2016-08-03 8:53 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Stefano Stabellini @ 2016-08-02 18:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Xen-devel
On Tue, 2 Aug 2016, Jan Beulich wrote:
> >>> On 02.08.16 at 16:59, <andrew.cooper3@citrix.com> wrote:
> > On 02/08/16 15:54, Jan Beulich wrote:
> >>>>> On 02.08.16 at 16:26, <julien.grall@arm.com> wrote:
> >>> On 02/08/16 15:17, Jan Beulich wrote:
> >>>> Well, I find it quite odd for hypercall argument counts to differ
> >>>> between arches. I.e. I'd conclude the ABI was mis-specified.
> >>> Is it documented somewhere for the x86 code? Looking at Linux, the
> >>> privcmd call is only passing 5 arguments on both ARM and x86.
> >> arch-x86/xen-x86_32.h has
> >>
> >> * Hypercall interface:
> >> * Input: %ebx, %ecx, %edx, %esi, %edi, %ebp (arguments 1-6)
> >> * Output: %eax
> >>
> >> while arch-x86/xen-x86_64.h has
> >>
> >> * Hypercall interface:
> >> * Input: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
> >> * Output: %rax
> >
> > The only actual 6 argument hypercall is the v4v hypercall, better known
> > as __HYPERVISOR_xc_reserved_op at index 39, but that isn't implemented
> > anywhere upstream.
>
> But it serves as an example what now wouldn't work on ARM.
At the time the arm hypercall ABI was published, it matched the x86
hypercall ABI, which had only 5 hypercall arguments.
The issue is that the x86 hypercall ABI changed, and now is out of sync
with ARM. The faulty commit being:
commit 4af64160c580b02f28c992c09d55957cb20a9b91
Author: David Vrabel <david.vrabel@citrix.com>
Date: Wed May 30 09:25:11 2012 +0100
x86: document register for 6th hypercall argument
From: David Vrabel <david.vrabel@citrix.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Committed-by: Keir Fraser <keir@xen.org>
While the ARM ABI is from few months earlier:
commit 40f20c4bfcd5d25c90f9419250ca8a229bf4c1e5
Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Date: Tue Mar 13 16:04:05 2012 +0000
arm: use r12 to pass the hypercall number
** This is a guest visible ABI change which requires an updated guest kernel **
Use r12 to pass the hypercall number and r0-r4 for the hypercall
arguments.
Use the ISS to pass an hypervisor specific tag.
Remove passing unused registers to arm_hypercall_table: we don't have 6
arguments hypercalls and we never use 64 bit values as hypercall
arguments, 64 bit values are only contained within structs passed as
arguments.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
[ use #ifndef NDEBUG, fix coding style, expand calling convention comment
slightly and added a big fat note about ABI change - ijc ]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-02 18:43 ` Stefano Stabellini
@ 2016-08-03 8:53 ` Jan Beulich
2016-08-03 10:55 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-03 8:53 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Andrew Cooper, Julien Grall, Xen-devel
>>> On 02.08.16 at 20:43, <sstabellini@kernel.org> wrote:
> On Tue, 2 Aug 2016, Jan Beulich wrote:
>> >>> On 02.08.16 at 16:59, <andrew.cooper3@citrix.com> wrote:
>> > On 02/08/16 15:54, Jan Beulich wrote:
>> >>>>> On 02.08.16 at 16:26, <julien.grall@arm.com> wrote:
>> >>> On 02/08/16 15:17, Jan Beulich wrote:
>> >>>> Well, I find it quite odd for hypercall argument counts to differ
>> >>>> between arches. I.e. I'd conclude the ABI was mis-specified.
>> >>> Is it documented somewhere for the x86 code? Looking at Linux, the
>> >>> privcmd call is only passing 5 arguments on both ARM and x86.
>> >> arch-x86/xen-x86_32.h has
>> >>
>> >> * Hypercall interface:
>> >> * Input: %ebx, %ecx, %edx, %esi, %edi, %ebp (arguments 1-6)
>> >> * Output: %eax
>> >>
>> >> while arch-x86/xen-x86_64.h has
>> >>
>> >> * Hypercall interface:
>> >> * Input: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
>> >> * Output: %rax
>> >
>> > The only actual 6 argument hypercall is the v4v hypercall, better known
>> > as __HYPERVISOR_xc_reserved_op at index 39, but that isn't implemented
>> > anywhere upstream.
>>
>> But it serves as an example what now wouldn't work on ARM.
>
> At the time the arm hypercall ABI was published, it matched the x86
> hypercall ABI, which had only 5 hypercall arguments.
>
> The issue is that the x86 hypercall ABI changed, and now is out of sync
> with ARM. The faulty commit being:
That's one way of viewing it, but I don't think an appropriate one.
6-argument hypercalls had always been possible on x86, just that
they might not have been documented in the public headers (but
instead only in the actual hypercall implementation).
Jan
> commit 4af64160c580b02f28c992c09d55957cb20a9b91
> Author: David Vrabel <david.vrabel@citrix.com>
> Date: Wed May 30 09:25:11 2012 +0100
>
> x86: document register for 6th hypercall argument
>
> From: David Vrabel <david.vrabel@citrix.com>
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Committed-by: Keir Fraser <keir@xen.org>
>
>
> While the ARM ABI is from few months earlier:
>
> commit 40f20c4bfcd5d25c90f9419250ca8a229bf4c1e5
> Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Date: Tue Mar 13 16:04:05 2012 +0000
>
> arm: use r12 to pass the hypercall number
>
> ** This is a guest visible ABI change which requires an updated guest
> kernel **
>
> Use r12 to pass the hypercall number and r0-r4 for the hypercall
> arguments.
>
> Use the ISS to pass an hypervisor specific tag.
>
> Remove passing unused registers to arm_hypercall_table: we don't have 6
> arguments hypercalls and we never use 64 bit values as hypercall
> arguments, 64 bit values are only contained within structs passed as
> arguments.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> [ use #ifndef NDEBUG, fix coding style, expand calling convention
> comment
> slightly and added a big fat note about ABI change - ijc ]
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-03 8:53 ` Jan Beulich
@ 2016-08-03 10:55 ` Julien Grall
2016-08-03 18:20 ` Stefano Stabellini
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2016-08-03 10:55 UTC (permalink / raw)
To: Jan Beulich, Stefano Stabellini; +Cc: Andrew Cooper, Xen-devel
Hi Jan,
On 03/08/16 09:53, Jan Beulich wrote:
>>>> On 02.08.16 at 20:43, <sstabellini@kernel.org> wrote:
>> On Tue, 2 Aug 2016, Jan Beulich wrote:
>>>>>> On 02.08.16 at 16:59, <andrew.cooper3@citrix.com> wrote:
>>>> On 02/08/16 15:54, Jan Beulich wrote:
>>>>>>>> On 02.08.16 at 16:26, <julien.grall@arm.com> wrote:
>>>>>> On 02/08/16 15:17, Jan Beulich wrote:
>>>>>>> Well, I find it quite odd for hypercall argument counts to differ
>>>>>>> between arches. I.e. I'd conclude the ABI was mis-specified.
>>>>>> Is it documented somewhere for the x86 code? Looking at Linux, the
>>>>>> privcmd call is only passing 5 arguments on both ARM and x86.
>>>>> arch-x86/xen-x86_32.h has
>>>>>
>>>>> * Hypercall interface:
>>>>> * Input: %ebx, %ecx, %edx, %esi, %edi, %ebp (arguments 1-6)
>>>>> * Output: %eax
>>>>>
>>>>> while arch-x86/xen-x86_64.h has
>>>>>
>>>>> * Hypercall interface:
>>>>> * Input: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
>>>>> * Output: %rax
>>>>
>>>> The only actual 6 argument hypercall is the v4v hypercall, better known
>>>> as __HYPERVISOR_xc_reserved_op at index 39, but that isn't implemented
>>>> anywhere upstream.
>>>
>>> But it serves as an example what now wouldn't work on ARM.
>>
>> At the time the arm hypercall ABI was published, it matched the x86
>> hypercall ABI, which had only 5 hypercall arguments.
>>
>> The issue is that the x86 hypercall ABI changed, and now is out of sync
>> with ARM. The faulty commit being:
>
> That's one way of viewing it, but I don't think an appropriate one.
> 6-argument hypercalls had always been possible on x86, just that
> they might not have been documented in the public headers (but
> instead only in the actual hypercall implementation).
I would tend to say that anything not documented in the public header is
not part of the ABI regardless how it has been implemented before hand.
Anyway, I looked at the hypercall implementation on ARM and it seems
that we half support the 6th argument. For instance
hypercall_create_continuation is clobbering r5/x5 which is not part of
the ABI.
However do_trap_hypercall is only supporting up to 5 argument.
I don't think it would be an issue to support 6 arguments on ARM.
Stefano, what do you think?
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 6/9] xen/multicall: Rework arch multicall handling
2016-07-18 9:51 ` [PATCH 6/9] xen/multicall: Rework arch multicall handling Andrew Cooper
2016-07-20 12:35 ` Julien Grall
@ 2016-08-03 15:02 ` Jan Beulich
2016-08-03 15:12 ` Andrew Cooper
1 sibling, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-03 15:02 UTC (permalink / raw)
To: Andrew Cooper; +Cc: JulienGrall, Stefano Stabellini, Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hypercall.c
> +++ b/xen/arch/x86/hypercall.c
> @@ -338,6 +338,34 @@ long pv_hypercall(struct cpu_user_regs *regs)
> return ret;
> }
>
> +void arch_do_multicall_call(struct mc_state *state)
> +{
> + if ( !is_pv_32bit_vcpu(current) )
> + {
> + struct multicall_entry *call = &state->call;
> +
> + if ( (call->op < NR_hypercalls) && hypercall_table[call->op] )
> + call->result = hypercall_table[call->op](
> + call->args[0], call->args[1], call->args[2],
> + call->args[3], call->args[4], call->args[5]);
> + else
> + call->result = -ENOSYS;
> + }
> +#ifdef CONFIG_COMPAT
> + else
> + {
> + struct compat_multicall_entry *call = &state->compat_call;
> +
> + if ( (call->op < NR_hypercalls) && compat_hypercall_table[call->op] )
Why two distinct checks here when pv_hypercall() does just one
outside the if/else? With them folded (or if there is a good reason),
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one more remark:
> --- a/xen/common/multicall.c
> +++ b/xen/common/multicall.c
> @@ -63,7 +63,7 @@ do_multicall(
>
> trace_multicall_call(&mcs->call);
>
> - do_multicall_call(&mcs->call);
> + arch_do_multicall_call(mcs);
I think do_multicall_call() as a name was fine, but otoh I also don't
really mind the name change.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 7/9] x86/pv: Merge the pv hypercall tables
2016-07-18 9:51 ` [PATCH 7/9] x86/pv: Merge the pv hypercall tables Andrew Cooper
@ 2016-08-03 15:07 ` Jan Beulich
2016-08-11 12:36 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-03 15:07 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> For the same reason as c/s 33a231e3f "x86/HVM: fold hypercall tables", this
> removes the risk of accidentally updating only one of the tables.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
But having come here I still can't see why this can't be folded with
patch 5 without also folding in patch 6. Anyway - as long as they're
going to get committed without too big of a time gap in between,
the final result is what matters most.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 8/9] x86/hypercall: Merge the hypercall arg tables
2016-07-18 9:51 ` [PATCH 8/9] x86/hypercall: Merge the hypercall arg tables Andrew Cooper
@ 2016-08-03 15:12 ` Jan Beulich
2016-08-03 15:15 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2016-08-03 15:12 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> For the same reason as c/s 33a231e3f "x86/HVM: fold hypercall tables" and
> (TODO - changeset) "x86/pv: Merge the pv hypercall tables", this removes the
> risk of accidentally updating only one of the tables.
Based on this argument perhaps hypercall and args tables should
get folded too, but I guess that's a work item for another day.
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 6/9] xen/multicall: Rework arch multicall handling
2016-08-03 15:02 ` Jan Beulich
@ 2016-08-03 15:12 ` Andrew Cooper
0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2016-08-03 15:12 UTC (permalink / raw)
To: Jan Beulich; +Cc: JulienGrall, Stefano Stabellini, Xen-devel
On 03/08/16 16:02, Jan Beulich wrote:
>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hypercall.c
>> +++ b/xen/arch/x86/hypercall.c
>> @@ -338,6 +338,34 @@ long pv_hypercall(struct cpu_user_regs *regs)
>> return ret;
>> }
>>
>> +void arch_do_multicall_call(struct mc_state *state)
>> +{
>> + if ( !is_pv_32bit_vcpu(current) )
>> + {
>> + struct multicall_entry *call = &state->call;
>> +
>> + if ( (call->op < NR_hypercalls) && hypercall_table[call->op] )
>> + call->result = hypercall_table[call->op](
>> + call->args[0], call->args[1], call->args[2],
>> + call->args[3], call->args[4], call->args[5]);
>> + else
>> + call->result = -ENOSYS;
>> + }
>> +#ifdef CONFIG_COMPAT
>> + else
>> + {
>> + struct compat_multicall_entry *call = &state->compat_call;
>> +
>> + if ( (call->op < NR_hypercalls) && compat_hypercall_table[call->op] )
> Why two distinct checks here when pv_hypercall() does just one
> outside the if/else? With them folded (or if there is a good reason),
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one more remark:
multicall_entry and compat_multicall_entry are different. call->op
lives at the same point in the union, but the field is a different width.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 8/9] x86/hypercall: Merge the hypercall arg tables
2016-08-03 15:12 ` Jan Beulich
@ 2016-08-03 15:15 ` Andrew Cooper
2016-08-03 15:28 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-08-03 15:15 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 03/08/16 16:12, Jan Beulich wrote:
>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> For the same reason as c/s 33a231e3f "x86/HVM: fold hypercall tables" and
>> (TODO - changeset) "x86/pv: Merge the pv hypercall tables", this removes the
>> risk of accidentally updating only one of the tables.
> Based on this argument perhaps hypercall and args tables should
> get folded too, but I guess that's a work item for another day.
That is rather harder to do. I thought about it, but couldn't find a
neat way of doing it.
The call table is an array of pointers, while the args table is an array
of bytes. Merging them would result in excessive padding for alignment
purposes, unless it was packed, at which point we are calling
non-aligned function pointers, and taking that associated performance hit.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 9/9] x86/hypercall: Reduce the size of the hypercall tables
2016-07-18 9:51 ` [PATCH 9/9] x86/hypercall: Reduce the size of the hypercall tables Andrew Cooper
@ 2016-08-03 15:17 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2016-08-03 15:17 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> The highest populated entry in each hypercall table is currently at index 49.
> There is no need to extend both to tables to 64 entries.
>
> Range check eax against the hypercall table array size, and use a
> BUILD_BUG_ON() to ensure that the hypercall tables don't grow larger than the
> args table.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 8/9] x86/hypercall: Merge the hypercall arg tables
2016-08-03 15:15 ` Andrew Cooper
@ 2016-08-03 15:28 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2016-08-03 15:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 03.08.16 at 17:15, <andrew.cooper3@citrix.com> wrote:
> On 03/08/16 16:12, Jan Beulich wrote:
>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> For the same reason as c/s 33a231e3f "x86/HVM: fold hypercall tables" and
>>> (TODO - changeset) "x86/pv: Merge the pv hypercall tables", this removes the
>>> risk of accidentally updating only one of the tables.
>> Based on this argument perhaps hypercall and args tables should
>> get folded too, but I guess that's a work item for another day.
>
> That is rather harder to do. I thought about it, but couldn't find a
> neat way of doing it.
>
> The call table is an array of pointers, while the args table is an array
> of bytes. Merging them would result in excessive padding for alignment
> purposes, unless it was packed, at which point we are calling
> non-aligned function pointers, and taking that associated performance hit.
If we folded everything together (pv, hvm, args), there would be
6 bytes padding per 34 or actual data, so I wouldn't be worried
too much. But I admit that merging hvm and pv tables wouldn't be
entirely trivial.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-03 10:55 ` Julien Grall
@ 2016-08-03 18:20 ` Stefano Stabellini
2016-08-04 11:27 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Stefano Stabellini @ 2016-08-03 18:20 UTC (permalink / raw)
To: Julien Grall; +Cc: Andrew Cooper, Stefano Stabellini, Jan Beulich, Xen-devel
On Wed, 3 Aug 2016, Julien Grall wrote:
> Hi Jan,
>
> On 03/08/16 09:53, Jan Beulich wrote:
> > > > > On 02.08.16 at 20:43, <sstabellini@kernel.org> wrote:
> > > On Tue, 2 Aug 2016, Jan Beulich wrote:
> > > > > > > On 02.08.16 at 16:59, <andrew.cooper3@citrix.com> wrote:
> > > > > On 02/08/16 15:54, Jan Beulich wrote:
> > > > > > > > > On 02.08.16 at 16:26, <julien.grall@arm.com> wrote:
> > > > > > > On 02/08/16 15:17, Jan Beulich wrote:
> > > > > > > > Well, I find it quite odd for hypercall argument counts to
> > > > > > > > differ
> > > > > > > > between arches. I.e. I'd conclude the ABI was mis-specified.
> > > > > > > Is it documented somewhere for the x86 code? Looking at Linux, the
> > > > > > > privcmd call is only passing 5 arguments on both ARM and x86.
> > > > > > arch-x86/xen-x86_32.h has
> > > > > >
> > > > > > * Hypercall interface:
> > > > > > * Input: %ebx, %ecx, %edx, %esi, %edi, %ebp (arguments 1-6)
> > > > > > * Output: %eax
> > > > > >
> > > > > > while arch-x86/xen-x86_64.h has
> > > > > >
> > > > > > * Hypercall interface:
> > > > > > * Input: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
> > > > > > * Output: %rax
> > > > >
> > > > > The only actual 6 argument hypercall is the v4v hypercall, better
> > > > > known
> > > > > as __HYPERVISOR_xc_reserved_op at index 39, but that isn't implemented
> > > > > anywhere upstream.
> > > >
> > > > But it serves as an example what now wouldn't work on ARM.
> > >
> > > At the time the arm hypercall ABI was published, it matched the x86
> > > hypercall ABI, which had only 5 hypercall arguments.
> > >
> > > The issue is that the x86 hypercall ABI changed, and now is out of sync
> > > with ARM. The faulty commit being:
> >
> > That's one way of viewing it, but I don't think an appropriate one.
> > 6-argument hypercalls had always been possible on x86, just that
> > they might not have been documented in the public headers (but
> > instead only in the actual hypercall implementation).
>
> I would tend to say that anything not documented in the public header is not
> part of the ABI regardless how it has been implemented before hand.
I agree. What is documented (or not documented) in the public headers
is the golden standard.
> Anyway, I looked at the hypercall implementation on ARM and it seems that we
> half support the 6th argument. For instance hypercall_create_continuation is
> clobbering r5/x5 which is not part of the ABI.
>
> However do_trap_hypercall is only supporting up to 5 argument.
>
> I don't think it would be an issue to support 6 arguments on ARM. Stefano,
> what do you think?
I am OK with supporting 6 arguments hypercalls and it would be good to
have both architectures match. The 32bit guest-side implementation needs
special caring, but it should be OK (see HYPERCALL5 in Linux).
Fortunately xen/include/public/arch-arm.h states that only arguments
used by an hypercall can be clobbered.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h
2016-08-03 18:20 ` Stefano Stabellini
@ 2016-08-04 11:27 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2016-08-04 11:27 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Andrew Cooper, Jan Beulich, Xen-devel
On 03/08/16 19:20, Stefano Stabellini wrote:
> On Wed, 3 Aug 2016, Julien Grall wrote:
>> Hi Jan,
>>
>> On 03/08/16 09:53, Jan Beulich wrote:
>>>>>> On 02.08.16 at 20:43, <sstabellini@kernel.org> wrote:
>>>> On Tue, 2 Aug 2016, Jan Beulich wrote:
>>>>>>>> On 02.08.16 at 16:59, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 02/08/16 15:54, Jan Beulich wrote:
>>>>>>>>>> On 02.08.16 at 16:26, <julien.grall@arm.com> wrote:
>>>>>>>> On 02/08/16 15:17, Jan Beulich wrote:
>>>>>>>>> Well, I find it quite odd for hypercall argument counts to
>>>>>>>>> differ
>>>>>>>>> between arches. I.e. I'd conclude the ABI was mis-specified.
>>>>>>>> Is it documented somewhere for the x86 code? Looking at Linux, the
>>>>>>>> privcmd call is only passing 5 arguments on both ARM and x86.
>>>>>>> arch-x86/xen-x86_32.h has
>>>>>>>
>>>>>>> * Hypercall interface:
>>>>>>> * Input: %ebx, %ecx, %edx, %esi, %edi, %ebp (arguments 1-6)
>>>>>>> * Output: %eax
>>>>>>>
>>>>>>> while arch-x86/xen-x86_64.h has
>>>>>>>
>>>>>>> * Hypercall interface:
>>>>>>> * Input: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
>>>>>>> * Output: %rax
>>>>>>
>>>>>> The only actual 6 argument hypercall is the v4v hypercall, better
>>>>>> known
>>>>>> as __HYPERVISOR_xc_reserved_op at index 39, but that isn't implemented
>>>>>> anywhere upstream.
>>>>>
>>>>> But it serves as an example what now wouldn't work on ARM.
>>>>
>>>> At the time the arm hypercall ABI was published, it matched the x86
>>>> hypercall ABI, which had only 5 hypercall arguments.
>>>>
>>>> The issue is that the x86 hypercall ABI changed, and now is out of sync
>>>> with ARM. The faulty commit being:
>>>
>>> That's one way of viewing it, but I don't think an appropriate one.
>>> 6-argument hypercalls had always been possible on x86, just that
>>> they might not have been documented in the public headers (but
>>> instead only in the actual hypercall implementation).
>>
>> I would tend to say that anything not documented in the public header is not
>> part of the ABI regardless how it has been implemented before hand.
>
> I agree. What is documented (or not documented) in the public headers
> is the golden standard.
>
>
>> Anyway, I looked at the hypercall implementation on ARM and it seems that we
>> half support the 6th argument. For instance hypercall_create_continuation is
>> clobbering r5/x5 which is not part of the ABI.
>>
>> However do_trap_hypercall is only supporting up to 5 argument.
>>
>> I don't think it would be an issue to support 6 arguments on ARM. Stefano,
>> what do you think?
>
> I am OK with supporting 6 arguments hypercalls and it would be good to
> have both architectures match. The 32bit guest-side implementation needs
> special caring, but it should be OK (see HYPERCALL5 in Linux).
It seems that Linux only handle 5 arguments for both x86 (see
arch/x86/asm/xen/hypercall.h) and ARM.
The funny part is the x86 code has:
"The result certainly isn't pretty, and it really shows up cpp's
weakness as as macro language. Sorry. (But let's just give thanks
there aren't more than 5 arguments...)"
Anyway, I will add this in my todo list, I don't think it is really
important to have this change right now.
> Fortunately xen/include/public/arch-arm.h states that only arguments
> used by an hypercall can be clobbered.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/9] x86/pv: Implement pv_hypercall() in C
2016-08-02 13:12 ` Jan Beulich
2016-08-02 14:06 ` Andrew Cooper
@ 2016-08-11 11:57 ` Andrew Cooper
2016-08-11 12:20 ` Jan Beulich
1 sibling, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2016-08-11 11:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 02/08/16 14:12, Jan Beulich wrote:
>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> +long pv_hypercall(struct cpu_user_regs *regs)
>> +{
>> + struct vcpu *curr = current;
>> +#ifndef NDEBUG
>> + unsigned long old_rip = regs->rip;
>> +#endif
>> + long ret;
>> + uint32_t eax = regs->eax;
>> +
>> + ASSERT(curr->arch.flags & TF_kernel_mode);
> I'm afraid TF_kernel_mode can't be relied on for 32-bit guests, so
> this needs to move into the if() below.
>
>> + if ( (eax >= NR_hypercalls) || !hypercall_table[eax] )
>> + return -ENOSYS;
>> +
>> + if ( !is_pv_32bit_vcpu(curr) )
>> + {
>> + unsigned long rdi = regs->rdi;
>> + unsigned long rsi = regs->rsi;
>> + unsigned long rdx = regs->rdx;
>> + unsigned long r10 = regs->r10;
>> + unsigned long r8 = regs->r8;
>> + unsigned long r9 = regs->r9;
>> +
>> +#ifndef NDEBUG
>> + /* Deliberately corrupt parameter regs not used by this hypercall. */
>> + switch ( hypercall_args_table[eax] )
>> + {
>> + case 0: rdi = 0xdeadbeefdeadf00dUL;
>> + case 1: rsi = 0xdeadbeefdeadf00dUL;
>> + case 2: rdx = 0xdeadbeefdeadf00dUL;
>> + case 3: r10 = 0xdeadbeefdeadf00dUL;
>> + case 4: r8 = 0xdeadbeefdeadf00dUL;
>> + case 5: r9 = 0xdeadbeefdeadf00dUL;
> Without comments, aren't these going to become 5 new Coverity
> issues?
>
>> + }
>> +#endif
>> + if ( unlikely(tb_init_done) )
>> + {
>> + unsigned long args[6] = { rdi, rsi, rdx, r10, r8, r9 };
>> +
>> + __trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
>> + }
>> +
>> + ret = hypercall_table[eax](rdi, rsi, rdx, r10, r8, r9);
>> +
>> +#ifndef NDEBUG
>> + if ( regs->rip == old_rip )
>> + {
>> + /* Deliberately corrupt parameter regs used by this hypercall. */
>> + switch ( hypercall_args_table[eax] )
>> + {
>> + case 6: regs->r9 = 0xdeadbeefdeadf00dUL;
>> + case 5: regs->r8 = 0xdeadbeefdeadf00dUL;
>> + case 4: regs->r10 = 0xdeadbeefdeadf00dUL;
>> + case 3: regs->edx = 0xdeadbeefdeadf00dUL;
>> + case 2: regs->esi = 0xdeadbeefdeadf00dUL;
>> + case 1: regs->edi = 0xdeadbeefdeadf00dUL;
> For consistency with earlier code, lease use rdx, rsi, and rdi here.
>
>> +#ifndef NDEBUG
>> + if ( regs->rip == old_rip )
>> + {
>> + /* Deliberately corrupt parameter regs used by this hypercall. */
>> + switch ( compat_hypercall_args_table[eax] )
>> + {
>> + case 6: regs->ebp = 0xdeadf00d;
>> + case 5: regs->edi = 0xdeadf00d;
>> + case 4: regs->esi = 0xdeadf00d;
>> + case 3: regs->edx = 0xdeadf00d;
>> + case 2: regs->ecx = 0xdeadf00d;
>> + case 1: regs->ebx = 0xdeadf00d;
> Please use 32-bit stores here.
>
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -25,70 +25,10 @@ UNLIKELY_START(ne, msi_check)
>> LOAD_C_CLOBBERED compat=1 ax=0
>> UNLIKELY_END(msi_check)
>>
>> - movl UREGS_rax(%rsp),%eax
>> GET_CURRENT(bx)
>>
>> - cmpl $NR_hypercalls,%eax
>> - jae compat_bad_hypercall
>> -#ifndef NDEBUG
>> - /* Deliberately corrupt parameter regs not used by this hypercall. */
>> - pushq UREGS_rbx(%rsp); pushq %rcx; pushq %rdx; pushq %rsi; pushq %rdi
>> - pushq UREGS_rbp+5*8(%rsp)
>> - leaq compat_hypercall_args_table(%rip),%r10
>> - movl $6,%ecx
>> - subb (%r10,%rax,1),%cl
>> - movq %rsp,%rdi
>> - movl $0xDEADBEEF,%eax
>> - rep stosq
>> - popq %r8 ; popq %r9 ; xchgl %r8d,%r9d /* Args 5&6: zero extend */
>> - popq %rdx; popq %rcx; xchgl %edx,%ecx /* Args 3&4: zero extend */
>> - popq %rdi; popq %rsi; xchgl %edi,%esi /* Args 1&2: zero extend */
>> - movl UREGS_rax(%rsp),%eax
>> - pushq %rax
>> - pushq UREGS_rip+8(%rsp)
>> -#define SHADOW_BYTES 16 /* Shadow EIP + shadow hypercall # */
>> -#else
>> - /* Relocate argument registers and zero-extend to 64 bits. */
>> - xchgl %ecx,%esi /* Arg 2, Arg 4 */
>> - movl %edx,%edx /* Arg 3 */
>> - movl %edi,%r8d /* Arg 5 */
>> - movl %ebp,%r9d /* Arg 6 */
>> - movl UREGS_rbx(%rsp),%edi /* Arg 1 */
>> -#define SHADOW_BYTES 0 /* No on-stack shadow state */
>> -#endif
>> - cmpb $0,tb_init_done(%rip)
>> -UNLIKELY_START(ne, compat_trace)
>> - call __trace_hypercall_entry
>> - /* Restore the registers that __trace_hypercall_entry clobbered. */
>> - movl UREGS_rax+SHADOW_BYTES(%rsp),%eax /* Hypercall # */
>> - movl UREGS_rbx+SHADOW_BYTES(%rsp),%edi /* Arg 1 */
>> - movl UREGS_rcx+SHADOW_BYTES(%rsp),%esi /* Arg 2 */
>> - movl UREGS_rdx+SHADOW_BYTES(%rsp),%edx /* Arg 3 */
>> - movl UREGS_rsi+SHADOW_BYTES(%rsp),%ecx /* Arg 4 */
>> - movl UREGS_rdi+SHADOW_BYTES(%rsp),%r8d /* Arg 5 */
>> - movl UREGS_rbp+SHADOW_BYTES(%rsp),%r9d /* Arg 6 */
>> -#undef SHADOW_BYTES
>> -UNLIKELY_END(compat_trace)
>> - leaq compat_hypercall_table(%rip),%r10
>> - PERFC_INCR(hypercalls, %rax, %rbx)
>> - callq *(%r10,%rax,8)
>> -#ifndef NDEBUG
>> - /* Deliberately corrupt parameter regs used by this hypercall. */
>> - popq %r10 # Shadow RIP
>> - cmpq %r10,UREGS_rip+8(%rsp)
>> - popq %rcx # Shadow hypercall index
>> - jne compat_skip_clobber /* If RIP has changed then don't clobber. */
>> - leaq compat_hypercall_args_table(%rip),%r10
>> - movb (%r10,%rcx,1),%cl
>> - movl $0xDEADBEEF,%r10d
>> - testb %cl,%cl; jz compat_skip_clobber; movl %r10d,UREGS_rbx(%rsp)
>> - cmpb $2, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rcx(%rsp)
>> - cmpb $3, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdx(%rsp)
>> - cmpb $4, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rsi(%rsp)
>> - cmpb $5, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdi(%rsp)
>> - cmpb $6, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rbp(%rsp)
>> -compat_skip_clobber:
>> -#endif
>> + mov %rsp, %rdi
>> + call pv_hypercall
>> movl %eax,UREGS_rax(%rsp) # save the return value
> To follow the HVM model, this should also move into C.
Having tried this, I can't.
Using regs->eax = -ENOSYS; in C results in the upper 32bits of UREGS_rax
set on the stack, and nothing else re-clobbers this.
It highlights a second bug present in the hvm side, and propagated to
the pv side.
Currently, eax gets truncated when reading out of the registers, before
it is bounds-checked against NR_hypercalls. For the HVM side, all this
does is risk aliasing if upper bits are set, but for the PV side, it
will cause a failure for a compat guest issuing a hypercall after
previously receiving an error.
I am proposing the following change to the HVM side to compensate, and
to leave the asm adjustment of UREGS_rax in place.
~Andrew
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e2bb58a..69315d1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4132,11 +4132,11 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
struct domain *currd = curr->domain;
struct segment_register sreg;
int mode = hvm_guest_x86_mode(curr);
- uint32_t eax = regs->eax;
+ unsigned long eax = regs->eax;
switch ( mode )
{
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 5/9] x86/hypercall: Move the hypercall tables into C
2016-08-02 13:40 ` Jan Beulich
@ 2016-08-11 12:00 ` Andrew Cooper
0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2016-08-11 12:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 02/08/16 14:40, Jan Beulich wrote:
>>>> On 02.08.16 at 15:30, <andrew.cooper3@citrix.com> wrote:
>> On 02/08/16 14:23, Jan Beulich wrote:
>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>> +hypercall_fn_t *const hypercall_table[NR_hypercalls] = {
>>>> + HYPERCALL(set_trap_table),
>>>> + HYPERCALL(mmu_update),
>>>> + HYPERCALL(set_gdt),
>>>> + HYPERCALL(stack_switch),
>>>> + HYPERCALL(set_callbacks),
>>>> + HYPERCALL(fpu_taskswitch),
>>>> + HYPERCALL(sched_op_compat),
>>>> + HYPERCALL(platform_op),
>>>> + HYPERCALL(set_debugreg),
>>>> + HYPERCALL(get_debugreg),
>>>> + HYPERCALL(update_descriptor),
>>>> + HYPERCALL(memory_op),
>>>> + HYPERCALL(multicall),
>>>> + HYPERCALL(update_va_mapping),
>>>> + HYPERCALL(set_timer_op),
>>>> + HYPERCALL(event_channel_op_compat),
>>>> + HYPERCALL(xen_version),
>>>> + HYPERCALL(console_io),
>>>> + HYPERCALL(physdev_op_compat),
>>>> + HYPERCALL(grant_table_op),
>>>> + HYPERCALL(vm_assist),
>>>> + HYPERCALL(update_va_mapping_otherdomain),
>>>> + HYPERCALL(iret),
>>>> + HYPERCALL(vcpu_op),
>>>> + HYPERCALL(set_segment_base),
>>>> + HYPERCALL(mmuext_op),
>>>> + HYPERCALL(xsm_op),
>>>> + HYPERCALL(nmi_op),
>>>> + HYPERCALL(sched_op),
>>>> + HYPERCALL(callback_op),
>>>> +#ifdef CONFIG_XENOPROF
>>>> + HYPERCALL(xenoprof_op),
>>>> +#endif
>>>> + HYPERCALL(event_channel_op),
>>>> + HYPERCALL(physdev_op),
>>>> + HYPERCALL(hvm_op),
>>>> + HYPERCALL(sysctl),
>>>> + HYPERCALL(domctl),
>>>> +#ifdef CONFIG_KEXEC
>>>> + HYPERCALL(kexec_op),
>>>> +#endif
>>>> +#ifdef CONFIG_TMEM
>>>> + HYPERCALL(tmem_op),
>>>> +#endif
>>> To be honest I'd prefer the necessary #ifdef-ery to live in hypercall.h,
>>> the more that then ARM could (if they want) benefit from that too.
>> Which #ifdefary?
>>
>> HYPERCALL() can't be used by ARM.
> I mean just the #ifdef-s above, not the HYPERCALL() lines. Clearly
> you can do
>
> #ifndef CONFIG_TMEM
> # define do_tmem_op NULL
> #endif
>
> and alike in the header?
This isn't any neater IMO, and still risks getting a NULL in one half of
a native/compat pair.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/9] x86/pv: Implement pv_hypercall() in C
2016-08-11 11:57 ` Andrew Cooper
@ 2016-08-11 12:20 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2016-08-11 12:20 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 11.08.16 at 13:57, <andrew.cooper3@citrix.com> wrote:
> On 02/08/16 14:12, Jan Beulich wrote:
>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> + mov %rsp, %rdi
>>> + call pv_hypercall
>>> movl %eax,UREGS_rax(%rsp) # save the return value
>> To follow the HVM model, this should also move into C.
>
> Having tried this, I can't.
>
> Using regs->eax = -ENOSYS; in C results in the upper 32bits of UREGS_rax
> set on the stack, and nothing else re-clobbers this.
I don't understand - why would this need "re-clobbering"? Hypercalls
are assumed to return longs, i.e. full 64 bits of data. Even in case
you say this to just the compat variant, my original comment was of
course meant for both, and in the compat case I don't see why the
upper half of RAX would be of any interest, considering the guest
can't look at it anyway.
> It highlights a second bug present in the hvm side, and propagated to
> the pv side.
>
> Currently, eax gets truncated when reading out of the registers, before
> it is bounds-checked against NR_hypercalls. For the HVM side, all this
> does is risk aliasing if upper bits are set, but for the PV side, it
> will cause a failure for a compat guest issuing a hypercall after
> previously receiving an error.
And again I don't understand - when we look at only the low 32 bits,
how would a prior error matter?
> I am proposing the following change to the HVM side to compensate, and
> to leave the asm adjustment of UREGS_rax in place.
>
> ~Andrew
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e2bb58a..69315d1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4132,11 +4132,11 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
> struct domain *currd = curr->domain;
> struct segment_register sreg;
> int mode = hvm_guest_x86_mode(curr);
> - uint32_t eax = regs->eax;
> + unsigned long eax = regs->eax;
That would have the potential of breaking 32-bit callers, as we
mustn't rely on the upper halves of registers when coming out of
compat mode. IOW this would need to be made mode dependent,
and even then I'm afraid this has a (however small) potential of
breaking existing callers (but I agree that from a pure bug fix pov
this change should be made for the 64-bit path, as the PV code
looks at the full 64 bits too).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 7/9] x86/pv: Merge the pv hypercall tables
2016-08-03 15:07 ` Jan Beulich
@ 2016-08-11 12:36 ` Andrew Cooper
0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2016-08-11 12:36 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 03/08/16 16:07, Jan Beulich wrote:
>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> For the same reason as c/s 33a231e3f "x86/HVM: fold hypercall tables", this
>> removes the risk of accidentally updating only one of the tables.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> But having come here I still can't see why this can't be folded with
> patch 5 without also folding in patch 6. Anyway - as long as they're
> going to get committed without too big of a time gap in between,
> the final result is what matters most.
References to hypercall_table and compat_hypercall_table are buried in
the multicall inline assembler.
Folding the tables first involves complicated changes, just to be taken
out one patch later.
The risk of accidentally breaking bisectability is greater than the
downside of splitting the patches up a bit. (Either way, they will be
committed together.)
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2016-08-11 12:36 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-18 9:51 [PATCH 0/9] x86: Move the pv hypercall into C Andrew Cooper
2016-07-18 9:51 ` [PATCH 1/9] x86/hypercall: Move some of the hvm hypercall infrastructure into hypercall.h Andrew Cooper
2016-08-02 12:50 ` Jan Beulich
2016-08-02 13:14 ` Andrew Cooper
2016-08-02 13:28 ` Jan Beulich
2016-08-02 14:04 ` Julien Grall
2016-08-02 14:17 ` Jan Beulich
2016-08-02 14:26 ` Julien Grall
2016-08-02 14:54 ` Jan Beulich
2016-08-02 14:59 ` Andrew Cooper
2016-08-02 15:05 ` Jan Beulich
2016-08-02 18:43 ` Stefano Stabellini
2016-08-03 8:53 ` Jan Beulich
2016-08-03 10:55 ` Julien Grall
2016-08-03 18:20 ` Stefano Stabellini
2016-08-04 11:27 ` Julien Grall
2016-07-18 9:51 ` [PATCH 2/9] x86/pv: Support do_set_segment_base() for compat guests Andrew Cooper
2016-08-02 12:52 ` Jan Beulich
2016-08-02 13:25 ` Andrew Cooper
2016-08-02 13:31 ` Jan Beulich
2016-08-02 13:39 ` Andrew Cooper
2016-08-02 13:47 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 3/9] x86/hypercall: Move the hypercall arg tables into C Andrew Cooper
2016-08-02 12:59 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 4/9] x86/pv: Implement pv_hypercall() in C Andrew Cooper
2016-08-02 13:12 ` Jan Beulich
2016-08-02 14:06 ` Andrew Cooper
2016-08-02 14:19 ` Jan Beulich
2016-08-11 11:57 ` Andrew Cooper
2016-08-11 12:20 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 5/9] x86/hypercall: Move the hypercall tables into C Andrew Cooper
2016-08-02 13:23 ` Jan Beulich
2016-08-02 13:30 ` Andrew Cooper
2016-08-02 13:40 ` Jan Beulich
2016-08-11 12:00 ` Andrew Cooper
2016-07-18 9:51 ` [PATCH 6/9] xen/multicall: Rework arch multicall handling Andrew Cooper
2016-07-20 12:35 ` Julien Grall
2016-08-03 15:02 ` Jan Beulich
2016-08-03 15:12 ` Andrew Cooper
2016-07-18 9:51 ` [PATCH 7/9] x86/pv: Merge the pv hypercall tables Andrew Cooper
2016-08-03 15:07 ` Jan Beulich
2016-08-11 12:36 ` Andrew Cooper
2016-07-18 9:51 ` [PATCH 8/9] x86/hypercall: Merge the hypercall arg tables Andrew Cooper
2016-08-03 15:12 ` Jan Beulich
2016-08-03 15:15 ` Andrew Cooper
2016-08-03 15:28 ` Jan Beulich
2016-07-18 9:51 ` [PATCH 9/9] x86/hypercall: Reduce the size of the hypercall tables Andrew Cooper
2016-08-03 15:17 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.