* [PATCH 00/12] xen: drop hypercall function tables
@ 2021-10-15 12:51 Juergen Gross
2021-10-15 12:51 ` [PATCH 01/12] xen: limit number of hypercall parameters to 5 Juergen Gross
` (11 more replies)
0 siblings, 12 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Ian Jackson, Christopher Clark
In order to avoid indirect function calls on the hypercall path as
much as possible this series is removing the hypercall function tables
and is replacing the hypercall handler calls via the function array
by automatically generated switch statements.
Another by-product of generating the switch statements is the
automatic generating of the hypercall handler prototypes from the
same data base which is used to generate the switch.
This has the additional advantage of using type safe calls of the
handlers and to ensure related handler (e.g. PV and HVM ones) share
the same prototypes.
A very brief performance test (parallel build of the Xen hypervisor
in a 6 vcpu guest) showed a very slim improvement (less than 1%) of
the performance with the patches applied. The test was performed using
a PV and a PVH guest.
Juergen Gross (12):
xen: limit number of hypercall parameters to 5
xen: move do_vcpu_op() to arch specific code
xen: harmonize return types of hypercall handlers
xen/x86: modify hvm_memory_op() prototype
xen: don't include asm/hypercall.h from C sources
xen: generate hypercall interface related code
xen: use generated prototypes for hypercall handlers
x86/pv-shim: don't modify hypercall table
xen/x86: don't use hypercall table for calling compat hypercalls
xen/x86: call hypercall handlers via switch statement
xen/arm: call hypercall handlers via switch statement
xen/x86: add hypercall performance counters for hvm, correct pv
.gitignore | 2 +
xen/Makefile | 10 +
xen/arch/arm/domain.c | 15 +-
xen/arch/arm/hvm.c | 3 +-
xen/arch/arm/physdev.c | 2 +-
xen/arch/arm/platform_hypercall.c | 6 +-
xen/arch/arm/traps.c | 124 +++--------
xen/arch/x86/compat.c | 14 +-
xen/arch/x86/cpu/vpmu.c | 1 +
xen/arch/x86/domain.c | 11 +-
xen/arch/x86/domctl.c | 4 +-
xen/arch/x86/hvm/hypercall.c | 172 ++------------
xen/arch/x86/hypercall.c | 59 -----
xen/arch/x86/mm.c | 1 -
xen/arch/x86/mm/paging.c | 3 +-
xen/arch/x86/platform_hypercall.c | 6 +-
xen/arch/x86/pv/callback.c | 20 +-
xen/arch/x86/pv/emul-priv-op.c | 2 +-
xen/arch/x86/pv/hypercall.c | 188 ++--------------
xen/arch/x86/pv/iret.c | 5 +-
xen/arch/x86/pv/misc-hypercalls.c | 14 +-
xen/arch/x86/pv/shim.c | 48 ++--
xen/arch/x86/traps.c | 2 +-
xen/arch/x86/x86_64/compat.c | 1 -
xen/arch/x86/x86_64/compat/mm.c | 1 +
xen/arch/x86/x86_64/domain.c | 16 +-
xen/arch/x86/x86_64/mm.c | 1 -
xen/arch/x86/x86_64/platform_hypercall.c | 9 +-
xen/common/argo.c | 12 +-
xen/common/compat/domain.c | 14 +-
xen/common/compat/grant_table.c | 1 +
xen/common/compat/multicall.c | 2 +-
xen/common/domain.c | 11 +-
xen/common/event_channel.c | 10 +
xen/common/grant_table.c | 10 +
xen/common/kexec.c | 6 +-
xen/common/multicall.c | 1 +
xen/common/trace.c | 2 +-
xen/include/asm-arm/hypercall.h | 7 +-
xen/include/asm-x86/hypercall.h | 195 +++-------------
xen/include/asm-x86/paging.h | 3 -
xen/include/asm-x86/pv/shim.h | 3 +
xen/include/hypercall-defs.c | 272 +++++++++++++++++++++++
xen/include/xen/hypercall.h | 181 +--------------
xen/scripts/gen_hypercall.awk | 228 +++++++++++++++++++
45 files changed, 769 insertions(+), 929 deletions(-)
create mode 100644 xen/include/hypercall-defs.c
create mode 100644 xen/scripts/gen_hypercall.awk
--
2.26.2
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 01/12] xen: limit number of hypercall parameters to 5
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 02/12] xen: move do_vcpu_op() to arch specific code Juergen Gross
` (10 subsequent siblings)
11 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap
Today there is no hypercall with more than 5 parameters, while the ABI
allows up to 6 parameters. Especially for the X86 32-bit case using
6 parameters would require to run without frame pointer, which isn't
very fortunate. Note that for Arm the limit is 5 parameters already.
So limit the maximum number of parameters to 5 for x86, too.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/hvm/hypercall.c | 28 ++++++++++------------------
xen/arch/x86/pv/hypercall.c | 22 ++++++++--------------
xen/common/trace.c | 2 +-
xen/include/asm-x86/hypercall.h | 2 +-
4 files changed, 20 insertions(+), 34 deletions(-)
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 122abf80de..f0321c6cb4 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -239,10 +239,9 @@ int hvm_hypercall(struct cpu_user_regs *regs)
unsigned long rdx = regs->rdx;
unsigned long r10 = regs->r10;
unsigned long r8 = regs->r8;
- unsigned long r9 = regs->r9;
- HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx, %lx)",
- eax, rdi, rsi, rdx, r10, r8, r9);
+ HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx)",
+ eax, rdi, rsi, rdx, r10, r8);
#ifndef NDEBUG
/* Deliberately corrupt parameter regs not used by this hypercall. */
@@ -252,13 +251,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
case 1: rsi = 0xdeadbeefdeadf00dUL; fallthrough;
case 2: rdx = 0xdeadbeefdeadf00dUL; fallthrough;
case 3: r10 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 4: r8 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 5: r9 = 0xdeadbeefdeadf00dUL;
+ case 4: r8 = 0xdeadbeefdeadf00dUL;
}
#endif
- regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8,
- r9);
+ regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8);
#ifndef NDEBUG
if ( !curr->hcall_preempted )
@@ -266,7 +263,6 @@ int hvm_hypercall(struct cpu_user_regs *regs)
/* Deliberately corrupt parameter regs used by this hypercall. */
switch ( hypercall_args_table[eax].native )
{
- case 6: regs->r9 = 0xdeadbeefdeadf00dUL; fallthrough;
case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
@@ -283,10 +279,9 @@ int hvm_hypercall(struct cpu_user_regs *regs)
unsigned int edx = regs->edx;
unsigned int esi = regs->esi;
unsigned int edi = regs->edi;
- unsigned int ebp = regs->ebp;
- HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%x, %x, %x, %x, %x, %x)", eax,
- ebx, ecx, edx, esi, edi, ebp);
+ HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%x, %x, %x, %x, %x)", eax,
+ ebx, ecx, edx, esi, edi);
#ifndef NDEBUG
/* Deliberately corrupt parameter regs not used by this hypercall. */
@@ -296,14 +291,12 @@ int hvm_hypercall(struct cpu_user_regs *regs)
case 1: ecx = 0xdeadf00d; fallthrough;
case 2: edx = 0xdeadf00d; fallthrough;
case 3: esi = 0xdeadf00d; fallthrough;
- case 4: edi = 0xdeadf00d; fallthrough;
- case 5: ebp = 0xdeadf00d;
+ case 4: edi = 0xdeadf00d;
}
#endif
curr->hcall_compat = true;
- regs->rax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi,
- ebp);
+ regs->rax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi);
curr->hcall_compat = false;
#ifndef NDEBUG
@@ -312,7 +305,6 @@ int hvm_hypercall(struct cpu_user_regs *regs)
/* Deliberately corrupt parameter regs used by this hypercall. */
switch ( hypercall_args_table[eax].compat )
{
- case 6: regs->rbp = 0xdeadf00d; fallthrough;
case 5: regs->rdi = 0xdeadf00d; fallthrough;
case 4: regs->rsi = 0xdeadf00d; fallthrough;
case 3: regs->rdx = 0xdeadf00d; fallthrough;
@@ -349,7 +341,7 @@ enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
func = array_access_nospec(hvm_hypercall_table, call->op).native;
if ( func )
call->result = func(call->args[0], call->args[1], call->args[2],
- call->args[3], call->args[4], call->args[5]);
+ call->args[3], call->args[4]);
else
call->result = -ENOSYS;
}
@@ -361,7 +353,7 @@ enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
func = array_access_nospec(hvm_hypercall_table, call->op).compat;
if ( func )
call->result = func(call->args[0], call->args[1], call->args[2],
- call->args[3], call->args[4], call->args[5]);
+ call->args[3], call->args[4]);
else
call->result = -ENOSYS;
}
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 3579ba905c..16a77e3a35 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -145,7 +145,6 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
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. */
@@ -155,18 +154,17 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
case 1: rsi = 0xdeadbeefdeadf00dUL; fallthrough;
case 2: rdx = 0xdeadbeefdeadf00dUL; fallthrough;
case 3: r10 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 4: r8 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 5: r9 = 0xdeadbeefdeadf00dUL;
+ case 4: r8 = 0xdeadbeefdeadf00dUL;
}
#endif
if ( unlikely(tb_init_done) )
{
- unsigned long args[6] = { rdi, rsi, rdx, r10, r8, r9 };
+ unsigned long args[5] = { rdi, rsi, rdx, r10, r8 };
__trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
}
- regs->rax = pv_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8, r9);
+ regs->rax = pv_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8);
#ifndef NDEBUG
if ( !curr->hcall_preempted )
@@ -174,7 +172,6 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
/* Deliberately corrupt parameter regs used by this hypercall. */
switch ( hypercall_args_table[eax].native )
{
- case 6: regs->r9 = 0xdeadbeefdeadf00dUL; fallthrough;
case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
@@ -192,7 +189,6 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
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. */
@@ -202,20 +198,19 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
case 1: ecx = 0xdeadf00d; fallthrough;
case 2: edx = 0xdeadf00d; fallthrough;
case 3: esi = 0xdeadf00d; fallthrough;
- case 4: edi = 0xdeadf00d; fallthrough;
- case 5: ebp = 0xdeadf00d;
+ case 4: edi = 0xdeadf00d;
}
#endif
if ( unlikely(tb_init_done) )
{
- unsigned long args[6] = { ebx, ecx, edx, esi, edi, ebp };
+ unsigned long args[5] = { ebx, ecx, edx, esi, edi };
__trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
}
curr->hcall_compat = true;
- regs->eax = pv_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi, ebp);
+ regs->eax = pv_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi);
curr->hcall_compat = false;
#ifndef NDEBUG
@@ -224,7 +219,6 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
/* Deliberately corrupt parameter regs used by this hypercall. */
switch ( hypercall_args_table[eax].compat )
{
- case 6: regs->ebp = 0xdeadf00d; fallthrough;
case 5: regs->edi = 0xdeadf00d; fallthrough;
case 4: regs->esi = 0xdeadf00d; fallthrough;
case 3: regs->edx = 0xdeadf00d; fallthrough;
@@ -262,7 +256,7 @@ enum mc_disposition pv_do_multicall_call(struct mc_state *state)
pv_hypercall_table[op].compat )
call->result = pv_hypercall_table[op].compat(
call->args[0], call->args[1], call->args[2],
- call->args[3], call->args[4], call->args[5]);
+ call->args[3], call->args[4]);
else
call->result = -ENOSYS;
}
@@ -276,7 +270,7 @@ enum mc_disposition pv_do_multicall_call(struct mc_state *state)
pv_hypercall_table[op].native )
call->result = pv_hypercall_table[op].native(
call->args[0], call->args[1], call->args[2],
- call->args[3], call->args[4], call->args[5]);
+ call->args[3], call->args[4]);
else
call->result = -ENOSYS;
}
diff --git a/xen/common/trace.c b/xen/common/trace.c
index a2a389a1c7..61fecc2b2b 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -822,7 +822,7 @@ void __trace_hypercall(uint32_t event, unsigned long op,
{
struct {
uint32_t op;
- uint32_t args[6];
+ uint32_t args[5];
} d;
uint32_t *a = d.args;
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index 0ae3b8b043..5d394d4923 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -13,7 +13,7 @@
typedef unsigned long hypercall_fn_t(
unsigned long, unsigned long, unsigned long,
- unsigned long, unsigned long, unsigned long);
+ unsigned long, unsigned long);
typedef struct {
hypercall_fn_t *native;
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 02/12] xen: move do_vcpu_op() to arch specific code
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
2021-10-15 12:51 ` [PATCH 01/12] xen: limit number of hypercall parameters to 5 Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 03/12] xen: harmonize return types of hypercall handlers Juergen Gross
` (9 subsequent siblings)
11 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu, Roger Pau Monné
Today Arm is using another entry point for the vcpu_op hypercall as
x86, as some of the common sub-ops are not supported on Arm. The Arm
specific handler filetrs out the not supported sub-ops and then calls
the common handler. This leads to the weird call hierarchy:
do_arm_vcpu_op()
do_vcpu_op()
arch_do_vcpu_op()
Clean this up by renaming do_vcpu_op() to common_vcpu_op() and
arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one
of above calls can be avoided without restricting any potential
future use of common sub-ops for Arm.
Additionally the single user of HYPERCALL_ARM() can be modified and
HYPERCALL_ARM() can be removed.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/arm/domain.c | 15 ++++++++-------
xen/arch/arm/traps.c | 7 +------
xen/arch/x86/domain.c | 11 +++++++----
xen/arch/x86/x86_64/domain.c | 16 ++++++++++++----
xen/common/compat/domain.c | 14 ++++++--------
xen/common/domain.c | 11 ++++-------
xen/include/asm-arm/hypercall.h | 2 --
xen/include/asm-x86/hypercall.h | 2 +-
xen/include/xen/hypercall.h | 2 +-
9 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index eef0661beb..d20a972b51 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1070,23 +1070,24 @@ void arch_dump_domain_info(struct domain *d)
}
-long do_arm_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
+long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
{
+ struct domain *d = current->domain;
+ struct vcpu *v;
+
+ if ( (v = domain_vcpu(d, vcpuid)) == NULL )
+ return -ENOENT;
+
switch ( cmd )
{
case VCPUOP_register_vcpu_info:
case VCPUOP_register_runstate_memory_area:
- return do_vcpu_op(cmd, vcpuid, arg);
+ return common_vcpu_op(cmd, v, arg);
default:
return -EINVAL;
}
}
-long arch_do_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
- return -ENOSYS;
-}
-
void arch_dump_vcpu_info(struct vcpu *v)
{
gic_dump_info(v);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 219ab3c3fb..7abc28848e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1351,11 +1351,6 @@ typedef struct {
.nr_args = _nr_args, \
}
-#define HYPERCALL_ARM(_name, _nr_args) \
- [ __HYPERVISOR_ ## _name ] = { \
- .fn = (arm_hypercall_fn_t) &do_arm_ ## _name, \
- .nr_args = _nr_args, \
- }
/*
* Only use this for hypercalls which were deprecated (i.e. replaced
* by something else) before Xen on ARM was created, i.e. *not* for
@@ -1386,7 +1381,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
#endif
HYPERCALL(multicall, 2),
HYPERCALL(platform_op, 1),
- HYPERCALL_ARM(vcpu_op, 3),
+ HYPERCALL(vcpu_op, 3),
HYPERCALL(vm_assist, 2),
#ifdef CONFIG_ARGO
HYPERCALL(argo_op, 5),
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc14..e1440ec2f5 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1488,11 +1488,14 @@ int arch_vcpu_reset(struct vcpu *v)
return 0;
}
-long
-arch_do_vcpu_op(
- int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
+long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
{
long rc = 0;
+ struct domain *d = current->domain;
+ struct vcpu *v;
+
+ if ( (v = domain_vcpu(d, vcpuid)) == NULL )
+ return -ENOENT;
switch ( cmd )
{
@@ -1544,7 +1547,7 @@ arch_do_vcpu_op(
}
default:
- rc = -ENOSYS;
+ rc = common_vcpu_op(cmd, v, arg);
break;
}
diff --git a/xen/arch/x86/x86_64/domain.c b/xen/arch/x86/x86_64/domain.c
index c46dccc25a..62fe51ee74 100644
--- a/xen/arch/x86/x86_64/domain.c
+++ b/xen/arch/x86/x86_64/domain.c
@@ -13,10 +13,14 @@ CHECK_vcpu_get_physid;
#undef xen_vcpu_get_physid
int
-arch_compat_vcpu_op(
- int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
+compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
{
- int rc = -ENOSYS;
+ int rc;
+ struct domain *d = current->domain;
+ struct vcpu *v;
+
+ if ( (v = domain_vcpu(d, vcpuid)) == NULL )
+ return -ENOENT;
switch ( cmd )
{
@@ -55,7 +59,11 @@ arch_compat_vcpu_op(
}
case VCPUOP_get_physid:
- rc = arch_do_vcpu_op(cmd, v, arg);
+ rc = do_vcpu_op(cmd, vcpuid, arg);
+ break;
+
+ default:
+ rc = compat_common_vcpu_op(cmd, v, arg);
break;
}
diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c
index 98b8c15cea..1119534679 100644
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -38,14 +38,12 @@ CHECK_vcpu_hvm_context;
#endif
-int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
+int compat_common_vcpu_op(int cmd, struct vcpu *v,
+ XEN_GUEST_HANDLE_PARAM(void) arg)
{
- struct domain *d = current->domain;
- struct vcpu *v;
int rc = 0;
-
- if ( (v = domain_vcpu(d, vcpuid)) == NULL )
- return -ENOENT;
+ struct domain *d = current->domain;
+ unsigned int vcpuid = v->vcpu_id;
switch ( cmd )
{
@@ -102,7 +100,7 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) ar
case VCPUOP_stop_singleshot_timer:
case VCPUOP_register_vcpu_info:
case VCPUOP_send_nmi:
- rc = do_vcpu_op(cmd, vcpuid, arg);
+ rc = common_vcpu_op(cmd, v, arg);
break;
case VCPUOP_get_runstate_info:
@@ -133,7 +131,7 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) ar
}
default:
- rc = arch_compat_vcpu_op(cmd, v, arg);
+ rc = -ENOSYS;
break;
}
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8b53c49d1e..8eb36fe714 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1559,14 +1559,11 @@ int default_initialise_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
return rc;
}
-long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
+long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
{
- struct domain *d = current->domain;
- struct vcpu *v;
long rc = 0;
-
- if ( (v = domain_vcpu(d, vcpuid)) == NULL )
- return -ENOENT;
+ struct domain *d = current->domain;
+ unsigned int vcpuid = v->vcpu_id;
switch ( cmd )
{
@@ -1736,7 +1733,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
}
default:
- rc = arch_do_vcpu_op(cmd, v, arg);
+ rc = -ENOSYS;
break;
}
diff --git a/xen/include/asm-arm/hypercall.h b/xen/include/asm-arm/hypercall.h
index a0c5a31a2f..9fd13c6b2c 100644
--- a/xen/include/asm-arm/hypercall.h
+++ b/xen/include/asm-arm/hypercall.h
@@ -4,8 +4,6 @@
#include <public/domctl.h> /* for arch_do_domctl */
int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-long do_arm_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg);
-
long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index 5d394d4923..e614f7c78c 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -152,7 +152,7 @@ compat_physdev_op(
XEN_GUEST_HANDLE_PARAM(void) arg);
extern int
-arch_compat_vcpu_op(
+compat_common_vcpu_op(
int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
extern int compat_mmuext_op(
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index 07b10ec230..30558d3c61 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -110,7 +110,7 @@ do_vcpu_op(
struct vcpu;
extern long
-arch_do_vcpu_op(int cmd,
+common_vcpu_op(int cmd,
struct vcpu *v,
XEN_GUEST_HANDLE_PARAM(void) arg);
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 03/12] xen: harmonize return types of hypercall handlers
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
2021-10-15 12:51 ` [PATCH 01/12] xen: limit number of hypercall parameters to 5 Juergen Gross
2021-10-15 12:51 ` [PATCH 02/12] xen: move do_vcpu_op() to arch specific code Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-18 11:55 ` Jan Beulich
2021-10-15 12:51 ` [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype Juergen Gross
` (8 subsequent siblings)
11 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu, Roger Pau Monné,
Christopher Clark
Today most hypercall handlers have a return type of long, while the
compat ones return an int. There are a few exceptions from that rule,
however.
Get rid of the exceptions by letting compat handlers always return int
and others always return long.
For the compat hvm case use eax instead of rax for the stored result as
it should have been from the beginning.
Additionally move some prototypes to include/asm-x86/hypercall.h
as they are x86 specific. Move the do_physdev_op() prototype from both
architecture dependant headers to the common one. Move the
compat_platform_op() prototype to the common header.
Switch some non style compliant types (u32, s32, s64) to style compliant
ones.
Rename paging_domctl_continuation() to do_paging_domctl_cont() and add
a matching define for the associated hypercall.
Make do_callback_op() and compat_callback_op() more similar by adding
the const attribute to compat_callback_op()'s 2nd parameter.
The do_platform_op() prototype needs to be modified in order to better
match its compat variant.
Change the type of the cmd parameter for [do|compat]_kexec_op() to
unsigned int, as this is more appropriate for the compat case.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/arm/physdev.c | 2 +-
xen/arch/arm/platform_hypercall.c | 5 ++-
xen/arch/x86/domctl.c | 4 +--
xen/arch/x86/hvm/hypercall.c | 8 ++---
xen/arch/x86/hypercall.c | 2 +-
xen/arch/x86/mm/paging.c | 3 +-
xen/arch/x86/platform_hypercall.c | 5 ++-
xen/arch/x86/pv/callback.c | 20 +++++------
xen/arch/x86/pv/emul-priv-op.c | 2 +-
xen/arch/x86/pv/hypercall.c | 5 +--
xen/arch/x86/pv/iret.c | 4 +--
xen/arch/x86/pv/misc-hypercalls.c | 14 +++++---
xen/arch/x86/x86_64/platform_hypercall.c | 5 ++-
xen/common/argo.c | 12 +++----
xen/common/kexec.c | 6 ++--
xen/include/asm-arm/hypercall.h | 1 -
xen/include/asm-x86/hypercall.h | 43 +++++++++++-------------
xen/include/asm-x86/paging.h | 3 --
xen/include/xen/hypercall.h | 26 +++++++-------
19 files changed, 84 insertions(+), 86 deletions(-)
diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
index f9aa274dda..5a7593fa8f 100644
--- a/xen/arch/arm/physdev.c
+++ b/xen/arch/arm/physdev.c
@@ -11,7 +11,7 @@
#include <xen/hypercall.h>
-int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+long do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
#ifdef CONFIG_HAS_PCI
return pci_physdev_op(cmd, arg);
diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
index 8efac7ee60..0013b99202 100644
--- a/xen/arch/arm/platform_hypercall.c
+++ b/xen/arch/arm/platform_hypercall.c
@@ -17,12 +17,15 @@
DEFINE_SPINLOCK(xenpf_lock);
-long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
+long do_platform_op(XEN_GUEST_HANDLE_PARAM(void) arg)
{
long ret;
+ XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op;
struct xen_platform_op curop, *op = &curop;
struct domain *d;
+ u_xenpf_op = guest_handle_cast(arg, xen_platform_op_t);
+
if ( copy_from_guest(op, u_xenpf_op, 1) )
return -EFAULT;
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 7d102e0647..b01ea81373 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -221,8 +221,8 @@ long arch_do_domctl(
case XEN_DOMCTL_shadow_op:
ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
if ( ret == -ERESTART )
- return hypercall_create_continuation(__HYPERVISOR_arch_1,
- "h", u_domctl);
+ return hypercall_create_continuation(
+ __HYPERVISOR_paging_domctl_cont, "h", u_domctl);
copyback = true;
break;
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index f0321c6cb4..5be1050453 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -120,8 +120,6 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
[ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
(hypercall_fn_t *) compat_ ## x }
-#define do_arch_1 paging_domctl_continuation
-
static const struct {
hypercall_fn_t *native, *compat;
} hvm_hypercall_table[] = {
@@ -154,11 +152,9 @@ static const struct {
#ifdef CONFIG_HYPFS
HYPERCALL(hypfs_op),
#endif
- HYPERCALL(arch_1)
+ HYPERCALL(paging_domctl_cont)
};
-#undef do_arch_1
-
#undef HYPERCALL
#undef HVM_CALL
#undef COMPAT_CALL
@@ -296,7 +292,7 @@ int hvm_hypercall(struct cpu_user_regs *regs)
#endif
curr->hcall_compat = true;
- regs->rax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi);
+ regs->eax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi);
curr->hcall_compat = false;
#ifndef NDEBUG
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 2370d31d3f..07e1a45ef5 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -75,7 +75,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
ARGS(dm_op, 3),
ARGS(hypfs_op, 5),
ARGS(mca, 1),
- ARGS(arch_1, 1),
+ ARGS(paging_domctl_cont, 1),
};
#undef COMP
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index dd6b2bdf6f..6cc2636bf4 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -21,6 +21,7 @@
#include <xen/init.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <asm/paging.h>
#include <asm/shadow.h>
#include <asm/p2m.h>
@@ -756,7 +757,7 @@ int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
return shadow_domctl(d, sc, u_domctl);
}
-long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+long do_paging_domctl_cont(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
struct xen_domctl op;
struct domain *d;
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 284c2dfb9e..681e0eb6c6 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -210,11 +210,14 @@ void resource_access(void *info)
}
#endif
-ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
+ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(void) arg)
{
ret_t ret;
+ XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op;
struct xen_platform_op curop, *op = &curop;
+ u_xenpf_op = guest_handle_cast(arg, xen_platform_op_t);
+
if ( copy_from_guest(op, u_xenpf_op, 1) )
return -EFAULT;
diff --git a/xen/arch/x86/pv/callback.c b/xen/arch/x86/pv/callback.c
index 42a6aa0831..6d60263dbc 100644
--- a/xen/arch/x86/pv/callback.c
+++ b/xen/arch/x86/pv/callback.c
@@ -207,9 +207,9 @@ long do_set_callbacks(unsigned long event_address,
#include <compat/callback.h>
#include <compat/nmi.h>
-static long compat_register_guest_callback(struct compat_callback_register *reg)
+static int compat_register_guest_callback(struct compat_callback_register *reg)
{
- long ret = 0;
+ int ret = 0;
struct vcpu *curr = current;
fixup_guest_code_selector(curr->domain, reg->address.cs);
@@ -256,10 +256,10 @@ static long compat_register_guest_callback(struct compat_callback_register *reg)
return ret;
}
-static long compat_unregister_guest_callback(
+static int compat_unregister_guest_callback(
struct compat_callback_unregister *unreg)
{
- long ret;
+ int ret;
switch ( unreg->type )
{
@@ -283,9 +283,9 @@ static long compat_unregister_guest_callback(
return ret;
}
-long compat_callback_op(int cmd, XEN_GUEST_HANDLE(void) arg)
+int compat_callback_op(int cmd, XEN_GUEST_HANDLE(const_void) arg)
{
- long ret;
+ int ret;
switch ( cmd )
{
@@ -321,10 +321,10 @@ long compat_callback_op(int cmd, XEN_GUEST_HANDLE(void) arg)
return ret;
}
-long compat_set_callbacks(unsigned long event_selector,
- unsigned long event_address,
- unsigned long failsafe_selector,
- unsigned long failsafe_address)
+int compat_set_callbacks(unsigned long event_selector,
+ unsigned long event_address,
+ unsigned long failsafe_selector,
+ unsigned long failsafe_address)
{
struct compat_callback_register event = {
.type = CALLBACKTYPE_event,
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index d0df5bc616..41f72abbd2 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -22,12 +22,12 @@
#include <xen/domain_page.h>
#include <xen/event.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/iocap.h>
#include <asm/amd.h>
#include <asm/debugreg.h>
#include <asm/hpet.h>
-#include <asm/hypercall.h>
#include <asm/mc146818rtc.h>
#include <asm/pv/domain.h>
#include <asm/pv/trace.h>
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 16a77e3a35..7e99dbda34 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -40,8 +40,6 @@
#define COMPAT_CALL(x) HYPERCALL(x)
#endif
-#define do_arch_1 paging_domctl_continuation
-
const pv_hypercall_table_t pv_hypercall_table[] = {
COMPAT_CALL(set_trap_table),
HYPERCALL(mmu_update),
@@ -102,11 +100,10 @@ const pv_hypercall_table_t pv_hypercall_table[] = {
#endif
HYPERCALL(mca),
#ifndef CONFIG_PV_SHIM_EXCLUSIVE
- HYPERCALL(arch_1),
+ HYPERCALL(paging_domctl_cont),
#endif
};
-#undef do_arch_1
#undef COMPAT_CALL
#undef HYPERCALL
diff --git a/xen/arch/x86/pv/iret.c b/xen/arch/x86/pv/iret.c
index 29a2f7cc45..90946c4629 100644
--- a/xen/arch/x86/pv/iret.c
+++ b/xen/arch/x86/pv/iret.c
@@ -48,7 +48,7 @@ static void async_exception_cleanup(struct vcpu *curr)
curr->arch.async_exception_state(trap).old_mask;
}
-unsigned long do_iret(void)
+long do_iret(void)
{
struct cpu_user_regs *regs = guest_cpu_user_regs();
struct iret_context iret_saved;
@@ -105,7 +105,7 @@ unsigned long do_iret(void)
}
#ifdef CONFIG_PV32
-unsigned int compat_iret(void)
+int compat_iret(void)
{
struct cpu_user_regs *regs = guest_cpu_user_regs();
struct vcpu *v = current;
diff --git a/xen/arch/x86/pv/misc-hypercalls.c b/xen/arch/x86/pv/misc-hypercalls.c
index 5dade24726..8b9b1f8dfa 100644
--- a/xen/arch/x86/pv/misc-hypercalls.c
+++ b/xen/arch/x86/pv/misc-hypercalls.c
@@ -28,12 +28,16 @@ long do_set_debugreg(int reg, unsigned long value)
return set_debugreg(current, reg, value);
}
-unsigned long do_get_debugreg(int reg)
+long do_get_debugreg(int reg)
{
- unsigned long val;
- int res = x86emul_read_dr(reg, &val, NULL);
-
- return res == X86EMUL_OKAY ? val : -ENODEV;
+ /* Avoid undefined behavior due to casting an unsigned long to long. */
+ union {
+ unsigned long val;
+ long ret;
+ } u;
+ int res = x86emul_read_dr(reg, &u.val, NULL);
+
+ return res == X86EMUL_OKAY ? u.ret : -ENODEV;
}
long do_fpu_taskswitch(int set)
diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c
index fbba893a47..4576c014a4 100644
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -4,13 +4,16 @@
EMIT_FILE;
+#include <xen/types.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/lib.h>
#include <compat/platform.h>
DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t);
#define xen_platform_op compat_platform_op
#define xen_platform_op_t compat_platform_op_t
-#define do_platform_op(x) compat_platform_op(_##x)
+#define do_platform_op(x) compat_platform_op(x)
#define efi_get_info efi_compat_get_info
#define efi_runtime_call(x) efi_compat_runtime_call(x)
diff --git a/xen/common/argo.c b/xen/common/argo.c
index eaea7ba888..bf6aac7655 100644
--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -2207,13 +2207,13 @@ do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
}
#ifdef CONFIG_COMPAT
-long
-compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
- XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
- unsigned long arg4)
+int compat_argo_op(unsigned int cmd,
+ XEN_GUEST_HANDLE_PARAM(void) arg1,
+ XEN_GUEST_HANDLE_PARAM(void) arg2,
+ unsigned long arg3, unsigned long arg4)
{
struct domain *currd = current->domain;
- long rc;
+ int rc;
xen_argo_send_addr_t send_addr;
xen_argo_iov_t iovs[XEN_ARGO_MAXIOV];
compat_argo_iov_t compat_iovs[XEN_ARGO_MAXIOV];
@@ -2267,7 +2267,7 @@ compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
rc = sendv(currd, &send_addr.src, &send_addr.dst, iovs, niov, arg4);
out:
- argo_dprintk("<-compat_argo_op(%u)=%ld\n", cmd, rc);
+ argo_dprintk("<-compat_argo_op(%u)=%d\n", cmd, rc);
return rc;
}
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index c63db618a7..d7373233e1 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -1213,7 +1213,7 @@ static int kexec_status(XEN_GUEST_HANDLE_PARAM(void) uarg)
return !!test_bit(bit, &kexec_flags);
}
-static int do_kexec_op_internal(unsigned long op,
+static int do_kexec_op_internal(unsigned int op,
XEN_GUEST_HANDLE_PARAM(void) uarg,
bool_t compat)
{
@@ -1265,13 +1265,13 @@ static int do_kexec_op_internal(unsigned long op,
return ret;
}
-long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg)
+long do_kexec_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
{
return do_kexec_op_internal(op, uarg, 0);
}
#ifdef CONFIG_COMPAT
-int compat_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg)
+int compat_kexec_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
{
return do_kexec_op_internal(op, uarg, 1);
}
diff --git a/xen/include/asm-arm/hypercall.h b/xen/include/asm-arm/hypercall.h
index 9fd13c6b2c..cadafd76c7 100644
--- a/xen/include/asm-arm/hypercall.h
+++ b/xen/include/asm-arm/hypercall.h
@@ -2,7 +2,6 @@
#define __ASM_ARM_HYPERCALL_H__
#include <public/domctl.h> /* for arch_do_domctl */
-int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index e614f7c78c..9c0981defd 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -11,6 +11,8 @@
#include <public/arch-x86/xen-mca.h> /* for do_mca */
#include <asm/paging.h>
+#define __HYPERVISOR_paging_domctl_cont __HYPERVISOR_arch_1
+
typedef unsigned long hypercall_fn_t(
unsigned long, unsigned long, unsigned long,
unsigned long, unsigned long);
@@ -88,7 +90,7 @@ do_set_debugreg(
int reg,
unsigned long value);
-extern unsigned long
+extern long
do_get_debugreg(
int reg);
@@ -102,17 +104,13 @@ do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc);
extern long
do_update_va_mapping(
unsigned long va,
- u64 val64,
+ uint64_t val64,
unsigned long flags);
-extern long
-do_physdev_op(
- int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-
extern long
do_update_va_mapping_otherdomain(
unsigned long va,
- u64 val64,
+ uint64_t val64,
unsigned long flags,
domid_t domid);
@@ -126,7 +124,7 @@ do_mmuext_op(
extern long do_callback_op(
int cmd, XEN_GUEST_HANDLE_PARAM(const_void) arg);
-extern unsigned long
+extern long
do_iret(
void);
@@ -141,16 +139,18 @@ do_set_segment_base(
unsigned int which,
unsigned long base);
+long do_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
+
+long do_xenpmu_op(unsigned int op,
+ XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg);
+
+long do_paging_domctl_cont(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
+
#ifdef CONFIG_COMPAT
#include <compat/arch-x86/xen.h>
#include <compat/physdev.h>
-extern int
-compat_physdev_op(
- int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
extern int
compat_common_vcpu_op(
int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
@@ -161,17 +161,14 @@ extern int compat_mmuext_op(
XEN_GUEST_HANDLE_PARAM(uint) pdone,
unsigned int foreigndom);
-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_callback_op(
+ int cmd, XEN_GUEST_HANDLE(const_void) arg);
extern int compat_update_va_mapping(
- unsigned int va, u32 lo, u32 hi, unsigned int flags);
+ unsigned int va, uint32_t lo, uint32_t hi, unsigned int flags);
extern int compat_update_va_mapping_otherdomain(
- unsigned int va, u32 lo, u32 hi, unsigned int flags, domid_t domid);
+ unsigned int va, uint32_t lo, uint32_t hi, unsigned int 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);
@@ -180,13 +177,13 @@ 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);
+ uint32_t pa_lo, uint32_t pa_hi, uint32_t desc_lo, uint32_t desc_hi);
-extern unsigned int compat_iret(void);
+extern int compat_iret(void);
extern int compat_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-extern long compat_set_callbacks(
+extern int compat_set_callbacks(
unsigned long event_selector, unsigned long event_address,
unsigned long failsafe_selector, unsigned long failsafe_address);
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index fa22558e2e..f6e8808b0a 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -236,9 +236,6 @@ int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl,
bool_t resuming);
-/* Helper hypercall for dealing with continuations. */
-long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
-
/* Call when destroying a vcpu/domain */
void paging_vcpu_teardown(struct vcpu *v);
int paging_teardown(struct domain *d);
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index 30558d3c61..8247a7ffc8 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -43,7 +43,7 @@ arch_do_sysctl(
extern long
do_platform_op(
- XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op);
+ XEN_GUEST_HANDLE_PARAM(void) u_xenpf_op);
extern long
pci_physdev_op(
@@ -114,11 +114,6 @@ common_vcpu_op(int cmd,
struct vcpu *v,
XEN_GUEST_HANDLE_PARAM(void) arg);
-extern long
-do_nmi_op(
- unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
extern long
do_hvm_op(
unsigned long op,
@@ -126,13 +121,15 @@ do_hvm_op(
extern long
do_kexec_op(
- unsigned long op,
+ unsigned int op,
XEN_GUEST_HANDLE_PARAM(void) uarg);
extern long
do_xsm_op(
XEN_GUEST_HANDLE_PARAM(void) u_xsm_op);
+long do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
+
#ifdef CONFIG_ARGO
extern long do_argo_op(
unsigned int cmd,
@@ -145,9 +142,6 @@ extern long do_argo_op(
extern long
do_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
-extern long
-do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg);
-
extern long
do_dm_op(
domid_t domid,
@@ -198,21 +192,25 @@ compat_sched_op(
extern int
compat_set_timer_op(
- u32 lo,
- s32 hi);
+ uint32_t lo,
+ int32_t hi);
extern int compat_xsm_op(
XEN_GUEST_HANDLE_PARAM(void) op);
-extern int compat_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg);
+extern int compat_kexec_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg);
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);
+int compat_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
+
+int compat_platform_op(XEN_GUEST_HANDLE_PARAM(void) u_xenpf_op);
+
#ifdef CONFIG_ARGO
-extern long compat_argo_op(
+extern int compat_argo_op(
unsigned int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg1,
XEN_GUEST_HANDLE_PARAM(void) arg2,
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (2 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 03/12] xen: harmonize return types of hypercall handlers Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-18 12:31 ` Jan Beulich
2021-10-15 12:51 ` [PATCH 05/12] xen: don't include asm/hypercall.h from C sources Juergen Gross
` (7 subsequent siblings)
11 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu
hvm_memory_op() should take an unsigned long as cmd, like
do_memory_op().
As hvm_memory_op() is basically just calling do_memory_op() (or
compat_memory_op()) passing through the parameters the cmd parameter
should have no smaller size than that of the called functions.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/hvm/hypercall.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 5be1050453..9d3b193bad 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -31,7 +31,7 @@
#include <public/hvm/hvm_op.h>
#include <public/hvm/params.h>
-static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
long rc;
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 05/12] xen: don't include asm/hypercall.h from C sources
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (3 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-18 12:39 ` Jan Beulich
2021-10-15 12:51 ` [PATCH 06/12] xen: generate hypercall interface related code Juergen Gross
` (6 subsequent siblings)
11 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu, Roger Pau Monné
Instead of including asm/hypercall.h always use xen/hypercall.h.
Additionally include xen/hypercall.h from all sources containing a
hypercall handler.
This prepares for generating the handlers' prototypes at build time.
Add a guard in asm/hypercall.h to catch direct inclusion.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/arm/hvm.c | 3 +--
xen/arch/arm/platform_hypercall.c | 1 +
xen/arch/x86/cpu/vpmu.c | 1 +
xen/arch/x86/mm.c | 1 -
xen/arch/x86/platform_hypercall.c | 1 +
xen/arch/x86/pv/iret.c | 1 +
xen/arch/x86/traps.c | 2 +-
xen/arch/x86/x86_64/compat/mm.c | 1 +
xen/arch/x86/x86_64/mm.c | 1 -
xen/arch/x86/x86_64/platform_hypercall.c | 4 ++--
xen/common/compat/grant_table.c | 1 +
xen/common/compat/multicall.c | 2 +-
xen/common/event_channel.c | 1 +
xen/common/grant_table.c | 1 +
xen/common/multicall.c | 1 +
xen/include/asm-arm/hypercall.h | 4 ++++
xen/include/asm-x86/hypercall.h | 4 ++++
17 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 8951b34086..fc1a52767d 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -20,6 +20,7 @@
#include <xen/lib.h>
#include <xen/errno.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/sched.h>
#include <xen/monitor.h>
@@ -29,8 +30,6 @@
#include <public/hvm/params.h>
#include <public/hvm/hvm_op.h>
-#include <asm/hypercall.h>
-
static int hvm_allow_set_param(const struct domain *d, unsigned int param)
{
switch ( param )
diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
index 0013b99202..6ede3b508c 100644
--- a/xen/arch/arm/platform_hypercall.c
+++ b/xen/arch/arm/platform_hypercall.c
@@ -9,6 +9,7 @@
#include <xen/types.h>
#include <xen/sched.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/spinlock.h>
#include <public/platform.h>
#include <xsm/xsm.h>
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 16e91a3694..6784f1bc2b 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -21,6 +21,7 @@
#include <xen/xenoprof.h>
#include <xen/event.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/cpu.h>
#include <xen/param.h>
#include <asm/regs.h>
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 4d799032dc..8df4539b7e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -129,7 +129,6 @@
#include <asm/ldt.h>
#include <asm/x86_emulate.h>
#include <asm/e820.h>
-#include <asm/hypercall.h>
#include <asm/shared.h>
#include <asm/mem_sharing.h>
#include <public/memory.h>
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 681e0eb6c6..639fd8f959 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -17,6 +17,7 @@
#include <xen/console.h>
#include <xen/iocap.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/acpi.h>
#include <xen/efi.h>
#include <xen/cpu.h>
diff --git a/xen/arch/x86/pv/iret.c b/xen/arch/x86/pv/iret.c
index 90946c4629..316a23e77e 100644
--- a/xen/arch/x86/pv/iret.c
+++ b/xen/arch/x86/pv/iret.c
@@ -18,6 +18,7 @@
*/
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/lib.h>
#include <xen/sched.h>
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 64f3396f20..05acbb8479 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -29,6 +29,7 @@
#include <xen/lib.h>
#include <xen/err.h>
#include <xen/errno.h>
+#include <xen/hypercall.h>
#include <xen/mm.h>
#include <xen/param.h>
#include <xen/console.h>
@@ -70,7 +71,6 @@
#include <asm/x86_emulate.h>
#include <asm/traps.h>
#include <asm/hvm/vpt.h>
-#include <asm/hypercall.h>
#include <asm/mce.h>
#include <asm/apic.h>
#include <asm/mc146818rtc.h>
diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 215e96aba0..13dfa94fee 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -1,4 +1,5 @@
#include <xen/event.h>
+#include <xen/hypercall.h>
#include <xen/mem_access.h>
#include <xen/multicall.h>
#include <compat/memory.h>
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 4f225da81e..fdcb75c78f 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -32,7 +32,6 @@ EMIT_FILE;
#include <asm/page.h>
#include <asm/flushtlb.h>
#include <asm/fixmap.h>
-#include <asm/hypercall.h>
#include <asm/msr.h>
#include <asm/pv/domain.h>
#include <asm/setup.h>
diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c
index 4576c014a4..1368be455f 100644
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -41,8 +41,8 @@ CHECK_pf_resource_entry;
#undef xen_pf_resource_entry
#define COMPAT
-#define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
-#define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
+#undef guest_handle_okay
+#define guest_handle_okay compat_handle_okay
typedef int ret_t;
#include "../platform_hypercall.c"
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index ff1d678f01..8077771508 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -4,6 +4,7 @@
*/
#include <compat/grant_table.h>
+#include <xen/hypercall.h>
#define xen_grant_entry_v1 grant_entry_v1
CHECK_grant_entry_v1;
diff --git a/xen/common/compat/multicall.c b/xen/common/compat/multicall.c
index a0e9918f48..c5982baf76 100644
--- a/xen/common/compat/multicall.c
+++ b/xen/common/compat/multicall.c
@@ -4,6 +4,7 @@
EMIT_FILE;
+#include <xen/hypercall.h>
#include <xen/types.h>
#include <xen/multicall.h>
#include <xen/trace.h>
@@ -19,7 +20,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs)
mcs->compat_call.args[i] = mcs->call.args[i];
}
-DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t);
#define multicall_entry compat_multicall_entry
#define multicall_entry_t multicall_entry_compat_t
#define do_multicall_call compat_multicall_call
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index da88ad141a..12006f592e 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -24,6 +24,7 @@
#include <xen/iocap.h>
#include <xen/compat.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/keyhandler.h>
#include <asm/current.h>
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index e510395d08..49e12621ac 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -33,6 +33,7 @@
#include <xen/trace.h>
#include <xen/grant_table.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/domain_page.h>
#include <xen/iommu.h>
#include <xen/paging.h>
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index 5a199ebf8f..7b20717c88 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -9,6 +9,7 @@
#include <xen/event.h>
#include <xen/multicall.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/perfc.h>
#include <xen/trace.h>
#include <asm/current.h>
diff --git a/xen/include/asm-arm/hypercall.h b/xen/include/asm-arm/hypercall.h
index cadafd76c7..ccd26c5184 100644
--- a/xen/include/asm-arm/hypercall.h
+++ b/xen/include/asm-arm/hypercall.h
@@ -1,3 +1,7 @@
+#ifndef __XEN_HYPERCALL_H__
+#error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
+#endif
+
#ifndef __ASM_ARM_HYPERCALL_H__
#define __ASM_ARM_HYPERCALL_H__
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index 9c0981defd..efe5963ee5 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -2,6 +2,10 @@
* asm-x86/hypercall.h
*/
+#ifndef __XEN_HYPERCALL_H__
+#error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
+#endif
+
#ifndef __ASM_X86_HYPERCALL_H__
#define __ASM_X86_HYPERCALL_H__
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 06/12] xen: generate hypercall interface related code
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (4 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 05/12] xen: don't include asm/hypercall.h from C sources Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-18 12:58 ` Jan Beulich
2021-10-15 12:51 ` [PATCH 07/12] xen: use generated prototypes for hypercall handlers Juergen Gross
` (5 subsequent siblings)
11 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Instead of repeating similar data multiple times use a single source
file and a generator script for producing prototypes and call sequences
of the hypercalls.
As the script already knows the number of parameters used add generating
a macro for populating an array with the number of parameters per
hypercall.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
.gitignore | 2 +
xen/Makefile | 10 ++
xen/include/hypercall-defs.c | 272 ++++++++++++++++++++++++++++++++++
xen/scripts/gen_hypercall.awk | 228 ++++++++++++++++++++++++++++
4 files changed, 512 insertions(+)
create mode 100644 xen/include/hypercall-defs.c
create mode 100644 xen/scripts/gen_hypercall.awk
diff --git a/.gitignore b/.gitignore
index 8ebb51b6c5..0edc014eb7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -332,10 +332,12 @@ xen/include/asm-x86/asm-macros.h
xen/include/compat/*
xen/include/config/
xen/include/generated/
+xen/include//hypercall-defs.i
xen/include/public/public
xen/include/xen/*.new
xen/include/xen/acm_policy.h
xen/include/xen/compile.h
+xen/include/xen/hypercall-defs.h
xen/include/xen/lib/x86/cpuid-autogen.h
xen/test/livepatch/config.h
xen/test/livepatch/expect_config.h
diff --git a/xen/Makefile b/xen/Makefile
index a3189eb47c..dfdae47e74 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -383,6 +383,7 @@ _clean: delete-unfresh-files
-o -name "*.gcno" -o -name ".*.cmd" -o -name "lib.a" \) -exec rm -f {} \;
rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
rm -f asm-offsets.s include/asm-*/asm-offsets.h
+ rm -f include/xen/hypercall-defs.h include/hypercall-defs.i
rm -f .banner .allconfig.tmp
.PHONY: _distclean
@@ -405,6 +406,7 @@ $(TARGET): delete-unfresh-files
$(MAKE) -f $(BASEDIR)/Rules.mk -C include
$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) include
$(MAKE) -f $(BASEDIR)/Rules.mk include/asm-$(TARGET_ARCH)/asm-offsets.h
+ $(MAKE) -f $(BASEDIR)/Rules.mk include/xen/hypercall-defs.h
$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) $@
# drivers/char/console.o contains static banner/compile info. Blow it away.
@@ -466,6 +468,14 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: asm-offsets.s
echo ""; \
echo "#endif") <$< >$@
+quiet_cmd_genhyp = GEN $@
+define cmd_genhyp
+ awk -f scripts/gen_hypercall.awk <$< >$@
+endef
+
+include/xen/hypercall-defs.h: include/hypercall-defs.i scripts/gen_hypercall.awk FORCE
+ $(call if_changed,genhyp)
+
SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
define all_sources
( find include/asm-$(TARGET_ARCH) -name '*.h' -print; \
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
new file mode 100644
index 0000000000..2ddd5ec962
--- /dev/null
+++ b/xen/include/hypercall-defs.c
@@ -0,0 +1,272 @@
+/*
+ * Hypercall interface description:
+ * Used by scripts/gen_hypercall.awk to generate hypercall prototypes and call
+ * sequences.
+ *
+ * Syntax is like a prototype, but without return type and without the ";" at
+ * the end. Pointer types will be automatically converted to use the
+ * XEN_GUEST_HANDLE_PARAM() macro. Handlers with no parameters just use a
+ * definition like "fn()".
+ * Hypercall/function names are without the leading "__HYPERVISOR_"/"do_"
+ * strings.
+ *
+ * The return type of a class of prototypes using the same prefix is set via:
+ * rettype: <prefix> <type>
+ * Default return type is "long". A return type for a prefix can be set only
+ * once and it needs to be set before that prefix is being used via the
+ * "prefix:" directive.
+ *
+ * The prefix of the prototypes is set via a line:
+ * prefix: <prefix> ...
+ * Multiple prefixes are possible (restriction see below). Prefixes are without
+ * a trailing "_". The current prefix settings are active until a new "prefix:"
+ * line.
+ *
+ * Caller macros are suffixed with a selectable name via lines like:
+ * caller: <suffix>
+ * When a caller suffix is active, there is only one active prefix allowed.
+ *
+ * With a "defhandle:" line it is possible to add a DEFINE_XEN_GUEST_HANDLE()
+ * to the generated header:
+ * defhandle: <handle-type> [<type>]
+ * Without specifying <type> only a DEFINE_XEN_GUEST_HANDLE(<handle-type>)
+ * will be generated, otherwise it will be a
+ * __DEFINE_XEN_GUEST_HANDLE(<handle-type>, <type>) being generated. Note that
+ * the latter will include the related "const" handle "const_<handle-type>".
+ *
+ * In order to support using coding style compliant pointers in the
+ * prototypes it is possible to add translation entries to generate the correct
+ * handle types:
+ * handle: <handle-type> <type>
+ * This will result in the prototype translation from "<type> *" to
+ * "XEN_GUEST_HANDLE_PARAM(<handle-type>)".
+ *
+ * The switch() statement bodies will be generated from a final table in the
+ * source file, which is started via the line:
+ * table: <caller> <caller> ...
+ * with the <caller>s specifying the designated caller macro of each column of
+ * the table. Any column of a <caller> not having been set via a "caller:"
+ * line will be ignored.
+ * The first column of the table contains the hypercall/prototype, each
+ * <caller> column contains the prefix for the function to use for that caller.
+ * A column not being supported by a <caller> is marked with "-". Lines with all
+ * entries being "-" after removal of inactive <caller> columns are ignored.
+ *
+ * This file is being preprocessed using $(CPP), so #ifdef CONFIG_* conditionals
+ * are possible.
+ */
+
+#ifdef CONFIG_HVM
+#define PREFIX_hvm hvm
+#else
+#define PREFIX_hvm
+#endif
+
+#ifdef CONFIG_COMPAT
+#define PREFIX_compat compat
+rettype: compat int
+#else
+#define PREFIX_compat
+#endif
+
+#ifdef CONFIG_ARM
+#define PREFIX_dep dep
+#else
+#define PREFIX_dep
+#endif
+
+handle: uint unsigned int
+handle: const_void const void
+handle: const_char const char
+
+#ifdef CONFIG_COMPAT
+defhandle: multicall_entry_compat_t
+#endif
+#ifdef CONFIG_PV32
+defhandle: trap_info_compat_t
+defhandle: physdev_op_compat_t
+#endif
+
+prefix: do PREFIX_hvm PREFIX_compat
+physdev_op(int cmd, void *arg)
+#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
+grant_table_op(unsigned int cmd, void *uop, unsigned int count)
+#endif
+
+prefix: do PREFIX_hvm
+memory_op(unsigned long cmd, void *arg)
+
+prefix: do PREFIX_compat
+xen_version(int cmd, void *arg)
+vcpu_op(int cmd, unsigned int vcpuid, void *arg)
+sched_op(int cmd, void *arg)
+xsm_op(void *op)
+callback_op(int cmd, const void *arg)
+#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+platform_op(void *u_xenpf_op)
+#endif
+#ifdef CONFIG_ARGO
+argo_op(unsigned int cmd, void *arg1, void *arg2, unsigned long arg3, unsigned long arg4)
+#endif
+#ifdef CONFIG_KEXEC
+kexec_op(unsigned int op, void *uarg)
+#endif
+#ifdef CONFIG_PV
+iret()
+nmi_op(unsigned int cmd, void *arg)
+#ifdef CONFIG_XENOPROF
+xenoprof_op(int op, void *arg)
+#endif
+#endif /* CONFIG_PV */
+
+#ifdef CONFIG_COMPAT
+prefix: compat
+set_timer_op(uint32_t lo, int32_t hi)
+multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls)
+memory_op(unsigned int cmd, void *arg)
+#ifdef CONFIG_IOREQ_SERVER
+dm_op(domid_t domid, unsigned int nr_bufs, void *bufs)
+#endif
+mmuext_op(void *arg, unsigned int count, uint *pdone, unsigned int foreigndom)
+#ifdef CONFIG_PV32
+set_trap_table(trap_info_compat_t *traps)
+set_gdt(unsigned int *frame_list, unsigned int entries)
+set_callbacks(unsigned long event_selector, unsigned long event_address, unsigned long failsafe_selector, unsigned long failsafe_address)
+update_descriptor(uint32_t pa_lo, uint32_t pa_hi, uint32_t desc_lo, uint32_t desc_hi)
+update_va_mapping(unsigned int va, uint32_t lo, uint32_t hi, unsigned int flags)
+physdev_op_compat(physdev_op_compat_t *uop)
+update_va_mapping_otherdomain(unsigned int va, uint32_t lo, uint32_t hi, unsigned int flags, domid_t domid)
+#endif
+#endif /* CONFIG_COMPAT */
+
+#if defined(CONFIG_PV) || defined(CONFIG_ARM)
+prefix: do PREFIX_dep
+event_channel_op_compat(evtchn_op_t *uop)
+physdev_op_compat(physdev_op_t *uop)
+/* Legacy hypercall (as of 0x00030101). */
+sched_op_compat(int cmd, unsigned long arg)
+#endif
+
+prefix: do
+set_timer_op(s_time_t timeout)
+console_io(unsigned int cmd, unsigned int count, char *buffer)
+vm_assist(unsigned int cmd, unsigned int type)
+event_channel_op(int cmd, void *arg)
+mmuext_op(mmuext_op_t *uops, unsigned int count, unsigned int *pdone, unsigned int foreigndom)
+multicall(multicall_entry_t *call_list, unsigned int nr_calls)
+#ifdef CONFIG_PV
+mmu_update(mmu_update_t *ureqs, unsigned int count, unsigned int *pdone, unsigned int foreigndom)
+stack_switch(unsigned long ss, unsigned long esp)
+fpu_taskswitch(int set)
+set_debugreg(int reg, unsigned long value)
+get_debugreg(int reg)
+set_segment_base(unsigned int which, unsigned long base)
+mca(xen_mc_t *u_xen_mc)
+set_trap_table(const_trap_info_t *traps)
+set_gdt(xen_ulong_t *frame_list, unsigned int entries)
+set_callbacks(unsigned long event_address, unsigned long failsafe_address, unsigned long syscall_address)
+update_descriptor(uint64_t gaddr, seg_desc_t desc)
+update_va_mapping(unsigned long va, uint64_t val64, unsigned long flags)
+update_va_mapping_otherdomain(unsigned long va, uint64_t val64, unsigned long flags, domid_t domid)
+#endif
+#ifdef CONFIG_IOREQ_SERVER
+dm_op(domid_t domid, unsigned int nr_bufs, xen_dm_op_buf_t *bufs)
+#endif
+#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+sysctl(xen_sysctl_t *u_sysctl)
+domctl(xen_domctl_t *u_domctl)
+paging_domctl_cont(xen_domctl_t *u_domctl)
+#endif
+#ifdef CONFIG_HVM
+hvm_op(unsigned long op, void *arg)
+#endif
+#ifdef CONFIG_HYPFS
+hypfs_op(unsigned int cmd, const char *arg1, unsigned long arg2, void *arg3, unsigned long arg4)
+#endif
+#ifdef CONFIG_X86
+xenpmu_op(unsigned int op, xen_pmu_params_t *arg)
+#endif
+
+#ifdef CONFIG_PV
+caller: pv64
+#ifdef CONFIG_PV32
+caller: pv32
+#endif
+#endif
+#ifdef CONFIG_HVM
+caller: hvm64
+#ifdef CONFIG_COMPAT
+caller: hvm32
+#endif
+#endif
+#ifdef CONFIG_ARM
+caller: arm
+#endif
+
+table: pv32 pv64 hvm32 hvm64 arm
+set_trap_table compat do - - -
+mmu_update do do - - -
+set_gdt compat do - - -
+stack_switch do do - - -
+set_callbacks compat do - - -
+fpu_taskswitch do do - - -
+sched_op_compat do do - - dep
+#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+platform_op compat do compat do do
+#endif
+set_debugreg do do - - -
+get_debugreg do do - - -
+update_descriptor compat do - - -
+memory_op compat do hvm hvm do
+multicall compat do compat do do
+update_va_mapping compat do - - -
+set_timer_op compat do compat do -
+event_channel_op_compat do do - - dep
+xen_version compat do compat do do
+console_io do do do do do
+physdev_op_compat compat do - - dep
+#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
+grant_table_op compat do hvm hvm do
+#endif
+vm_assist do do do do do
+update_va_mapping_otherdomain compat do - - -
+iret compat do - - -
+vcpu_op compat do compat do do
+set_segment_base do do - - -
+#ifdef CONFIG_PV
+mmuext_op compat do compat do -
+#endif
+xsm_op compat do compat do do
+nmi_op compat do - - -
+sched_op compat do compat do do
+callback_op compat do - - -
+#ifdef CONFIG_XENOPROF
+xenoprof_op compat do - - -
+#endif
+event_channel_op do do do do do
+physdev_op compat do hvm hvm do
+#ifdef CONFIG_HVM
+hvm_op do do do do do
+#endif
+#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+sysctl do do do do do
+domctl do do do do do
+#endif
+#ifdef CONFIG_KEXEC
+kexec_op compat do - - -
+#endif
+tmem_op - - - - -
+#ifdef CONFIG_ARGO
+argo_op compat do compat do do
+#endif
+xenpmu_op do do do do -
+#ifdef CONFIG_IOREQ_SERVER
+dm_op compat do compat do do
+#endif
+#ifdef CONFIG_HYPFS
+hypfs_op do do do do do
+#endif
+mca do do - - -
+#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+paging_domctl_cont do do do do -
+#endif
diff --git a/xen/scripts/gen_hypercall.awk b/xen/scripts/gen_hypercall.awk
new file mode 100644
index 0000000000..50d4cdbfcd
--- /dev/null
+++ b/xen/scripts/gen_hypercall.awk
@@ -0,0 +1,228 @@
+# awk script to generate hypercall handler prototypes and a macro for doing
+# the calls of the handlers inside a switch() statement.
+
+BEGIN {
+ printf("/* Generated file, do not edit! */\n\n");
+ e = 0;
+ n = 0;
+ p = 0;
+ nc = 0;
+}
+
+# Issue error to stderr
+function do_err(msg) {
+ print "Error: "msg": "$0 >"/dev/stderr";
+ exit 1;
+}
+
+# Generate case statement for call
+function do_call(f, p, i) {
+ printf(" case __HYPERVISOR_%s: \\\n", fn[f]);
+ printf(" ret = %s_%s(", pre[f, p], fn[f]);
+ for (i = 1; i <= n_args[f]; i++) {
+ if (i > 1)
+ printf(", ");
+ if (ptr[f, i])
+ printf("(XEN_GUEST_HANDLE_PARAM(%s)){ _p(a%d) }", typ[f, i], i);
+ else
+ printf("(%s)(a%d)", typ[f, i], i);
+ }
+ printf("); \\\n");
+ printf(" break; \\\n");
+}
+
+function rest_of_line(par, i, val) {
+ val = $(par);
+ for (i = par + 1; i <= NF; i++)
+ val = val " " $(i);
+ return val;
+}
+
+# Handle comments (multi- and single line)
+$1 == "/*" {
+ comment = 1;
+}
+comment == 1 {
+ if ($(NF) == "*/") comment = 0;
+ next;
+}
+
+# Skip preprocessing artefacts
+$1 == "extern" {
+ next;
+}
+/^#/ {
+ next;
+}
+
+# Drop empty lines
+NF == 0 {
+ next;
+}
+
+# Handle "handle:" line
+$1 == "handle:" {
+ if (NF < 3)
+ do_err("\"handle:\" requires at least two parameters");
+ val = rest_of_line(3);
+ xlate[val] = $2;
+ next;
+}
+
+# Handle "defhandle:" line
+$1 == "defhandle:" {
+ if (NF < 2)
+ do_err("\"defhandle:\" requires at least one parameter");
+ e++;
+ if (NF == 2) {
+ emit[e] = sprintf("DEFINE_XEN_GUEST_HANDLE(%s);", $2);
+ } else {
+ val = rest_of_line(3);
+ emit[e] = sprintf("__DEFINE_XEN_GUEST_HANDLE(%s, %s);", $2, val);
+ xlate[val] = $2;
+ }
+ next;
+}
+
+# Handle "rettype:" line
+$1 == "rettype:" {
+ if (NF < 3)
+ do_err("\"rettype:\" requires at least two parameters");
+ if ($2 in rettype)
+ do_err("rettype can be set only once for each prefix");
+ rettype[$2] = rest_of_line(3);
+ next;
+}
+
+# Handle "caller:" line
+$1 == "caller:" {
+ caller[$2] = 1;
+ next;
+}
+
+# Handle "prefix:" line
+$1 == "prefix:" {
+ p = NF - 1;
+ for (i = 2; i <= NF; i++) {
+ prefix[i - 1] = $(i);
+ if (!(prefix[i - 1] in rettype))
+ rettype[prefix[i - 1]] = "long";
+ }
+ next;
+}
+
+# Handle "table:" line
+$1 == "table:" {
+ table = 1;
+ for (i = 2; i <= NF; i++)
+ col[i - 1] = $(i);
+ n_cols = NF - 1;
+ next;
+}
+
+# Handle table definition line
+table == 1 {
+ if (NF != n_cols + 1)
+ do_err("Table definition line has wrong number of fields");
+ for (c = 1; c <= n_cols; c++) {
+ if (caller[col[c]] != 1)
+ continue;
+ if ($(c + 1) == "-")
+ continue;
+ for (i = 1; i <= n; i++) {
+ if (fn[i] != $1)
+ continue;
+ for (j = 1; j <= n_pre[i]; j++) {
+ if (pre[i, j] == $(c + 1)) {
+ nc++;
+ call[nc] = col[c];
+ call_fn[nc] = i;
+ call_p[nc] = j;
+ }
+ }
+ }
+ }
+ next;
+}
+
+# Prototype line
+{
+ bro = index($0, "(");
+ brc = index($0, ")");
+ if (bro < 2 || brc < bro)
+ do_err("No valid prototype line");
+ n++;
+ fn[n] = substr($0, 1, bro - 1);
+ n_pre[n] = p;
+ for (i = 1; i <= p; i++)
+ pre[n, i] = prefix[i];
+ args = substr($0, bro + 1, brc - bro - 1);
+ n_args[n] = split(args, a, ",");
+ if (n_args[n] > 5)
+ do_err("Too many parameters");
+ for (i = 1; i <= n_args[n]; i++) {
+ sub("^ *", "", a[i]); # Remove leading white space
+ sub(" +", " ", a[i]); # Replace multiple spaces with single ones
+ sub(" *$", "", a[i]); # Remove trailing white space
+ ptr[n, i] = index(a[i], "*"); # Is it a pointer type?
+ sub("[*]", "", a[i]); # Remove "*"
+ if (index(a[i], " ") == 0)
+ do_err("Parameter with no type or no name");
+ typ[n, i] = a[i];
+ sub(" [^ ]+$", "", typ[n, i]); # Remove parameter name
+ if (ptr[n, i] && (typ[n, i] in xlate))
+ typ[n, i] = xlate[typ[n, i]];
+ arg[n, i] = a[i];
+ sub("^([^ ]+ )+", "", arg[n, i]); # Remove parameter type
+ }
+}
+
+# Generate the output
+END {
+ # Verbatim generated lines
+ for (i = 1; i <= e; i++)
+ printf("%s\n", emit[i]);
+ printf("\n");
+ # Generate prototypes
+ for (i = 1; i <= n; i++) {
+ for (p = 1; p <= n_pre[i]; p++) {
+ printf("%s %s_%s(", rettype[pre[i, p]], pre[i, p], fn[i]);
+ if (n_args[i] == 0)
+ printf("void");
+ else
+ for (j = 1; j <= n_args[i]; j++) {
+ if (j > 1)
+ printf(", ");
+ if (ptr[i, j])
+ printf("XEN_GUEST_HANDLE_PARAM(%s)", typ[i, j]);
+ else
+ printf("%s", typ[i, j]);
+ printf(" %s", arg[i, j]);
+ }
+ printf(");\n");
+ }
+ }
+ # Generate call sequences and args array contents
+ for (ca in caller) {
+ if (caller[ca] != 1)
+ continue;
+ printf("\n");
+ printf("#define call_handlers_%s(num, ret, a1, a2, a3, a4, a5) \\\n", ca);
+ printf(" switch ( num ) \\\n");
+ printf(" { \\\n");
+ for (i = 1; i <= nc; i++)
+ if (call[i] == ca)
+ do_call(call_fn[i], call_p[i]);
+ printf(" default: \\\n");
+ printf(" ret = -ENOSYS; \\\n");
+ printf(" break; \\\n");
+ printf(" }\n");
+ printf("\n");
+ printf("#define hypercall_args_%s \\\n", ca);
+ printf("{ \\\n");
+ for (i = 1; i <= nc; i++)
+ if (call[i] == ca)
+ printf("[__HYPERVISOR_%s] = %d, \\\n", fn[call_fn[i]], n_args[call_fn[i]]);
+ printf("}\n");
+ }
+}
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 07/12] xen: use generated prototypes for hypercall handlers
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (5 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 06/12] xen: generate hypercall interface related code Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-18 13:01 ` Jan Beulich
2021-10-15 12:51 ` [PATCH 08/12] x86/pv-shim: don't modify hypercall table Juergen Gross
` (4 subsequent siblings)
11 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini
Remove the hypercall handler's prototypes in the related header files
and use the generated ones instead.
Some handlers having been static before need to be made globally
visible.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/hvm/hypercall.c | 6 +-
xen/include/asm-x86/hypercall.h | 133 ------------------------
xen/include/xen/hypercall.h | 177 +-------------------------------
3 files changed, 5 insertions(+), 311 deletions(-)
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 9d3b193bad..85b7a33523 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -31,7 +31,7 @@
#include <public/hvm/hvm_op.h>
#include <public/hvm/params.h>
-static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
long rc;
@@ -51,7 +51,7 @@ static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
}
#ifdef CONFIG_GRANT_TABLE
-static long hvm_grant_table_op(
+long hvm_grant_table_op(
unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
{
switch ( cmd )
@@ -77,7 +77,7 @@ static long hvm_grant_table_op(
}
#endif
-static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
const struct vcpu *curr = current;
const struct domain *currd = curr->domain;
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index efe5963ee5..2547572ccd 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -52,104 +52,6 @@ void pv_ring3_init_hypercall_page(void *ptr);
*/
#define MMU_UPDATE_PREEMPTED (~(~0U>>1))
-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);
-
-extern long
-do_mmu_update(
- XEN_GUEST_HANDLE_PARAM(mmu_update_t) ureqs,
- unsigned int count,
- XEN_GUEST_HANDLE_PARAM(uint) pdone,
- unsigned int foreigndom);
-
-extern long
-do_set_gdt(
- XEN_GUEST_HANDLE_PARAM(xen_ulong_t) frame_list,
- unsigned int entries);
-
-extern long
-do_stack_switch(
- unsigned long ss,
- unsigned long esp);
-
-extern long
-do_fpu_taskswitch(
- int set);
-
-extern long
-do_set_debugreg(
- int reg,
- unsigned long value);
-
-extern long
-do_get_debugreg(
- int reg);
-
-extern long
-do_update_descriptor(
- uint64_t gaddr, seg_desc_t desc);
-
-extern long
-do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc);
-
-extern long
-do_update_va_mapping(
- unsigned long va,
- uint64_t val64,
- unsigned long flags);
-
-extern long
-do_update_va_mapping_otherdomain(
- unsigned long va,
- uint64_t val64,
- unsigned long flags,
- domid_t domid);
-
-extern long
-do_mmuext_op(
- XEN_GUEST_HANDLE_PARAM(mmuext_op_t) uops,
- unsigned int count,
- XEN_GUEST_HANDLE_PARAM(uint) pdone,
- unsigned int foreigndom);
-
-extern long do_callback_op(
- int cmd, XEN_GUEST_HANDLE_PARAM(const_void) arg);
-
-extern long
-do_iret(
- void);
-
-extern long
-do_set_callbacks(
- unsigned long event_address,
- unsigned long failsafe_address,
- unsigned long syscall_address);
-
-extern long
-do_set_segment_base(
- unsigned int which,
- unsigned long base);
-
-long do_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-
-long do_xenpmu_op(unsigned int op,
- XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg);
-
-long do_paging_domctl_cont(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
-
#ifdef CONFIG_COMPAT
#include <compat/arch-x86/xen.h>
@@ -159,41 +61,6 @@ extern int
compat_common_vcpu_op(
int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
-extern int compat_mmuext_op(
- XEN_GUEST_HANDLE_PARAM(void) arg,
- unsigned int count,
- XEN_GUEST_HANDLE_PARAM(uint) pdone,
- unsigned int foreigndom);
-
-extern int compat_callback_op(
- int cmd, XEN_GUEST_HANDLE(const_void) arg);
-
-extern int compat_update_va_mapping(
- unsigned int va, uint32_t lo, uint32_t hi, unsigned int flags);
-
-extern int compat_update_va_mapping_otherdomain(
- unsigned int va, uint32_t lo, uint32_t hi, unsigned int 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(
- uint32_t pa_lo, uint32_t pa_hi, uint32_t desc_lo, uint32_t desc_hi);
-
-extern int compat_iret(void);
-
-extern int compat_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern int 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 8247a7ffc8..cf6a3c463d 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -18,33 +18,19 @@
#include <asm/hypercall.h>
#include <xsm/xsm.h>
-extern long
-do_sched_op(
- int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern long
-do_domctl(
- XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
+/* Needs to be after asm/hypercall.h. */
+#include <xen/hypercall-defs.h>
extern long
arch_do_domctl(
struct xen_domctl *domctl, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
-extern long
-do_sysctl(
- XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl);
-
extern long
arch_do_sysctl(
struct xen_sysctl *sysctl,
XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl);
-extern long
-do_platform_op(
- XEN_GUEST_HANDLE_PARAM(void) u_xenpf_op);
-
extern long
pci_physdev_op(
int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
@@ -62,170 +48,11 @@ pci_physdev_op(
#define MEMOP_EXTENT_SHIFT 6 /* cmd[:6] == start_extent */
#define MEMOP_CMD_MASK ((1 << MEMOP_EXTENT_SHIFT) - 1)
-extern long
-do_memory_op(
- unsigned long cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern long
-do_multicall(
- XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list,
- unsigned int nr_calls);
-
-extern long
-do_set_timer_op(
- s_time_t timeout);
-
-extern long
-do_event_channel_op(
- int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern long
-do_xen_version(
- int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern long
-do_console_io(
- unsigned int cmd,
- unsigned int count,
- XEN_GUEST_HANDLE_PARAM(char) buffer);
-
-extern long
-do_grant_table_op(
- unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(void) uop,
- unsigned int count);
-
-extern long
-do_vm_assist(
- unsigned int cmd,
- unsigned int type);
-
-extern long
-do_vcpu_op(
- int cmd,
- unsigned int vcpuid,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-struct vcpu;
extern long
common_vcpu_op(int cmd,
struct vcpu *v,
XEN_GUEST_HANDLE_PARAM(void) arg);
-extern long
-do_hvm_op(
- unsigned long op,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern long
-do_kexec_op(
- unsigned int op,
- XEN_GUEST_HANDLE_PARAM(void) uarg);
-
-extern long
-do_xsm_op(
- XEN_GUEST_HANDLE_PARAM(void) u_xsm_op);
-
-long do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-
-#ifdef CONFIG_ARGO
-extern long do_argo_op(
- unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg1,
- XEN_GUEST_HANDLE_PARAM(void) arg2,
- unsigned long arg3,
- unsigned long arg4);
-#endif
-
-extern long
-do_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern long
-do_dm_op(
- domid_t domid,
- unsigned int nr_bufs,
- XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
-
-#ifdef CONFIG_HYPFS
-extern long
-do_hypfs_op(
- unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(const_char) arg1,
- unsigned long arg2,
- XEN_GUEST_HANDLE_PARAM(void) arg3,
- unsigned long arg4);
-#endif
-
-#ifdef CONFIG_COMPAT
-
-extern int
-compat_memory_op(
- unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern int
-compat_grant_table_op(
- unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(void) uop,
- unsigned int count);
-
-extern int
-compat_vcpu_op(
- int cmd,
- unsigned int vcpuid,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern int
-compat_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern int
-compat_xen_version(
- int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern int
-compat_sched_op(
- int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg);
-
-extern int
-compat_set_timer_op(
- uint32_t lo,
- int32_t hi);
-
-extern int compat_xsm_op(
- XEN_GUEST_HANDLE_PARAM(void) op);
-
-extern int compat_kexec_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg);
-
-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);
-
-int compat_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-
-int compat_platform_op(XEN_GUEST_HANDLE_PARAM(void) u_xenpf_op);
-
-#ifdef CONFIG_ARGO
-extern int compat_argo_op(
- unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(void) arg1,
- XEN_GUEST_HANDLE_PARAM(void) arg2,
- unsigned long arg3,
- unsigned long arg4);
-#endif
-
-extern int
-compat_dm_op(
- domid_t domid,
- unsigned int nr_bufs,
- XEN_GUEST_HANDLE_PARAM(void) bufs);
-
-#endif
-
void arch_get_xen_caps(xen_capabilities_info_t *info);
#endif /* __XEN_HYPERCALL_H__ */
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 08/12] x86/pv-shim: don't modify hypercall table
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (6 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 07/12] xen: use generated prototypes for hypercall handlers Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-15 13:57 ` Jan Beulich
2021-10-15 12:51 ` [PATCH 09/12] xen/x86: don't use hypercall table for calling compat hypercalls Juergen Gross
` (3 subsequent siblings)
11 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini
When running as pv-shim the hypercall is modified today in order to
replace the functions for __HYPERVISOR_event_channel_op and
__HYPERVISOR_grant_table_op hypercalls.
Change this to call the related functions from the normal handlers
instead when running as shim. The performance implications are not
really relevant, as a normal production hypervisor will not be
configured to support shim mode, so the related calls will be dropped
due to optimization of the compiler.
Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy
wrapper do_grant_table_op() needed, as in this case grant_table.c
isn't being built.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/pv/shim.c | 48 +++++++++++++++--------------------
xen/common/event_channel.c | 9 +++++++
xen/common/grant_table.c | 9 +++++++
xen/include/asm-x86/pv/shim.h | 3 +++
4 files changed, 42 insertions(+), 27 deletions(-)
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index d9704121a7..8f66aa3957 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -56,11 +56,6 @@ static DEFINE_SPINLOCK(balloon_lock);
static struct platform_bad_page __initdata reserved_pages[2];
-static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-static long pv_shim_grant_table_op(unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(void) uop,
- unsigned int count);
-
/*
* By default give the shim 1MB of free memory slack. Some users may wish to
* tune this constants for better memory utilization. This can be achieved
@@ -203,7 +198,6 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
start_info_t *si)
{
bool compat = is_pv_32bit_domain(d);
- pv_hypercall_table_t *rw_pv_hypercall_table;
uint64_t param = 0;
long rc;
@@ -249,23 +243,6 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
consoled_set_ring_addr(page);
}
- /*
- * Locate pv_hypercall_table[] (usually .rodata) in the directmap (which
- * is writeable) and insert some shim-specific hypercall handlers.
- */
- rw_pv_hypercall_table = __va(__pa(pv_hypercall_table));
- rw_pv_hypercall_table[__HYPERVISOR_event_channel_op].native =
- (hypercall_fn_t *)pv_shim_event_channel_op;
- rw_pv_hypercall_table[__HYPERVISOR_grant_table_op].native =
- (hypercall_fn_t *)pv_shim_grant_table_op;
-
-#ifdef CONFIG_PV32
- rw_pv_hypercall_table[__HYPERVISOR_event_channel_op].compat =
- (hypercall_fn_t *)pv_shim_event_channel_op;
- rw_pv_hypercall_table[__HYPERVISOR_grant_table_op].compat =
- (hypercall_fn_t *)pv_shim_grant_table_op;
-#endif
-
guest = d;
/*
@@ -435,7 +412,7 @@ int pv_shim_shutdown(uint8_t reason)
return 0;
}
-static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
struct domain *d = current->domain;
struct evtchn_close close;
@@ -683,9 +660,9 @@ void pv_shim_inject_evtchn(unsigned int port)
# define compat_handle_okay guest_handle_okay
#endif
-static long pv_shim_grant_table_op(unsigned int cmd,
- XEN_GUEST_HANDLE_PARAM(void) uop,
- unsigned int count)
+long pv_shim_grant_table_op(unsigned int cmd,
+ XEN_GUEST_HANDLE_PARAM(void) uop,
+ unsigned int count)
{
struct domain *d = current->domain;
long rc = 0;
@@ -845,6 +822,23 @@ static long pv_shim_grant_table_op(unsigned int cmd,
return rc;
}
+#ifndef CONFIG_GRANT_TABLE
+/* Thin wrapper(s) needed. */
+long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
+ unsigned int count)
+{
+ return pv_shim_grant_table_op(cmd, uop, count);
+}
+
+#ifdef CONFIG_PV32
+int compat_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
+ unsigned int count)
+{
+ return pv_shim_grant_table_op(cmd, uop, count);
+}
+#endif
+#endif
+
long pv_shim_cpu_up(void *data)
{
struct vcpu *v = data;
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 12006f592e..a15f986b32 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -32,6 +32,10 @@
#include <public/event_channel.h>
#include <xsm/xsm.h>
+#ifdef CONFIG_PV_SHIM
+#include <asm/guest.h>
+#endif
+
#define ERROR_EXIT(_errno) \
do { \
gdprintk(XENLOG_WARNING, \
@@ -1190,6 +1194,11 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
int rc;
+#ifdef CONFIG_PV_SHIM
+ if ( pv_shim )
+ return pv_shim_event_channel_op(cmd, arg);
+#endif
+
switch ( cmd )
{
case EVTCHNOP_alloc_unbound: {
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 49e12621ac..de097c0eda 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -45,6 +45,10 @@
#include <asm/flushtlb.h>
#include <asm/guest_atomics.h>
+#ifdef CONFIG_PV_SHIM
+#include <asm/guest.h>
+#endif
+
/* Per-domain grant information. */
struct grant_table {
/*
@@ -3526,6 +3530,11 @@ do_grant_table_op(
long rc;
unsigned int opaque_in = cmd & GNTTABOP_ARG_MASK, opaque_out = 0;
+#ifdef CONFIG_PV_SHIM
+ if ( pv_shim )
+ return pv_shim_grant_table_op(cmd, uop, count);
+#endif
+
if ( (int)count < 0 )
return -EINVAL;
diff --git a/xen/include/asm-x86/pv/shim.h b/xen/include/asm-x86/pv/shim.h
index 8a91f4f9df..6415f8068e 100644
--- a/xen/include/asm-x86/pv/shim.h
+++ b/xen/include/asm-x86/pv/shim.h
@@ -19,6 +19,7 @@
#ifndef __X86_PV_SHIM_H__
#define __X86_PV_SHIM_H__
+#include <xen/hypercall.h>
#include <xen/types.h>
#if defined(CONFIG_PV_SHIM_EXCLUSIVE)
@@ -45,6 +46,8 @@ domid_t get_initial_domain_id(void);
uint64_t pv_shim_mem(uint64_t avail);
void pv_shim_fixup_e820(struct e820map *e820);
const struct platform_bad_page *pv_shim_reserved_pages(unsigned int *size);
+typeof(do_event_channel_op) pv_shim_event_channel_op;
+typeof(do_grant_table_op) pv_shim_grant_table_op;
#else
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 09/12] xen/x86: don't use hypercall table for calling compat hypercalls
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (7 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 08/12] x86/pv-shim: don't modify hypercall table Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 10/12] xen/x86: call hypercall handlers via switch statement Juergen Gross
` (2 subsequent siblings)
11 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu
Today the *_op_compat hypercalls call the modern handler functions by
using the entries from the hypercall table. This is resulting in a
not needed indirect function call which can be avoided by using the
correct handler function directly. This is basically a revert of
commit 1252e282311734 ("86/pv: Export pv_hypercall_table[] rather
than working around it in several ways"), which reasoning no longer
applies, as shim no longer modifies the hypercall table.
The hypercall table can now be made static as there is no external
reference to it any longer.
Commit 834cb8761051f7 ("x86/PV32: fix physdev_op_compat handling")
can be reverted, too, as using the direct call of the correct handler
is already handled fine without that patch.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/compat.c | 14 ++++----------
xen/arch/x86/pv/hypercall.c | 9 ++++++++-
xen/arch/x86/x86_64/compat.c | 1 -
xen/include/asm-x86/hypercall.h | 8 --------
4 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/xen/arch/x86/compat.c b/xen/arch/x86/compat.c
index 58b202f701..939b449dec 100644
--- a/xen/arch/x86/compat.c
+++ b/xen/arch/x86/compat.c
@@ -17,14 +17,12 @@ typedef long ret_t;
/* Legacy hypercall (as of 0x00030202). */
ret_t do_physdev_op_compat(XEN_GUEST_HANDLE_PARAM(physdev_op_t) uop)
{
- typeof(do_physdev_op) *fn =
- (void *)pv_hypercall_table[__HYPERVISOR_physdev_op].native;
struct physdev_op op;
if ( unlikely(copy_from_guest(&op, uop, 1) != 0) )
return -EFAULT;
- return fn(op.cmd, guest_handle_from_ptr(&uop.p->u, void));
+ return do_physdev_op(op.cmd, guest_handle_from_ptr(&uop.p->u, void));
}
#ifndef COMPAT
@@ -32,14 +30,11 @@ ret_t do_physdev_op_compat(XEN_GUEST_HANDLE_PARAM(physdev_op_t) uop)
/* Legacy hypercall (as of 0x00030101). */
long do_sched_op_compat(int cmd, unsigned long arg)
{
- typeof(do_sched_op) *fn =
- (void *)pv_hypercall_table[__HYPERVISOR_sched_op].native;
-
switch ( cmd )
{
case SCHEDOP_yield:
case SCHEDOP_block:
- return fn(cmd, guest_handle_from_ptr(NULL, void));
+ return do_sched_op(cmd, guest_handle_from_ptr(NULL, void));
case SCHEDOP_shutdown:
TRACE_3D(TRC_SCHED_SHUTDOWN,
@@ -57,8 +52,6 @@ long do_sched_op_compat(int cmd, unsigned long arg)
/* Legacy hypercall (as of 0x00030202). */
long do_event_channel_op_compat(XEN_GUEST_HANDLE_PARAM(evtchn_op_t) uop)
{
- typeof(do_event_channel_op) *fn =
- (void *)pv_hypercall_table[__HYPERVISOR_event_channel_op].native;
struct evtchn_op op;
if ( unlikely(copy_from_guest(&op, uop, 1) != 0) )
@@ -76,7 +69,8 @@ long do_event_channel_op_compat(XEN_GUEST_HANDLE_PARAM(evtchn_op_t) uop)
case EVTCHNOP_bind_ipi:
case EVTCHNOP_bind_vcpu:
case EVTCHNOP_unmask:
- return fn(op.cmd, guest_handle_from_ptr(&uop.p->u, void));
+ return do_event_channel_op(op.cmd,
+ guest_handle_from_ptr(&uop.p->u, void));
default:
return -ENOSYS;
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 7e99dbda34..6c4a32d2a6 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -27,6 +27,13 @@
#include <asm/multicall.h>
#include <irq_vectors.h>
+typedef struct {
+ hypercall_fn_t *native;
+#ifdef CONFIG_PV32
+ hypercall_fn_t *compat;
+#endif
+} pv_hypercall_table_t;
+
#ifdef CONFIG_PV32
#define HYPERCALL(x) \
[ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
@@ -40,7 +47,7 @@
#define COMPAT_CALL(x) HYPERCALL(x)
#endif
-const pv_hypercall_table_t pv_hypercall_table[] = {
+static const pv_hypercall_table_t pv_hypercall_table[] = {
COMPAT_CALL(set_trap_table),
HYPERCALL(mmu_update),
COMPAT_CALL(set_gdt),
diff --git a/xen/arch/x86/x86_64/compat.c b/xen/arch/x86/x86_64/compat.c
index fcbc1cc0d7..0e4c71f2aa 100644
--- a/xen/arch/x86/x86_64/compat.c
+++ b/xen/arch/x86/x86_64/compat.c
@@ -12,7 +12,6 @@ EMIT_FILE;
#define physdev_op_t physdev_op_compat_t
#define do_physdev_op compat_physdev_op
#define do_physdev_op_compat(x) compat_physdev_op_compat(_##x)
-#define native compat
#define COMPAT
#define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index 2547572ccd..eb2907b5b6 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -21,13 +21,6 @@ typedef unsigned long hypercall_fn_t(
unsigned long, unsigned long, unsigned long,
unsigned long, unsigned long);
-typedef struct {
- hypercall_fn_t *native;
-#ifdef CONFIG_PV32
- hypercall_fn_t *compat;
-#endif
-} pv_hypercall_table_t;
-
typedef struct {
uint8_t native;
#ifdef CONFIG_COMPAT
@@ -38,7 +31,6 @@ typedef struct {
extern const hypercall_args_t hypercall_args_table[NR_hypercalls];
#ifdef CONFIG_PV
-extern const pv_hypercall_table_t pv_hypercall_table[];
void pv_hypercall(struct cpu_user_regs *regs);
#endif
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 10/12] xen/x86: call hypercall handlers via switch statement
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (8 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 09/12] xen/x86: don't use hypercall table for calling compat hypercalls Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-21 14:41 ` Jan Beulich
2021-10-15 12:51 ` [PATCH 11/12] xen/arm: " Juergen Gross
2021-10-15 12:51 ` [PATCH 12/12] xen/x86: add hypercall performance counters for hvm, correct pv Juergen Gross
11 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu
Instead of using a function table use the generated switch statement
macros for calling the appropriate hypercall handlers.
This is beneficial to performance and avoids speculation issues.
With calling the handlers using the correct number of parameters now
it is possible to do the parameter register clobbering in the NDEBUG
case after returning from the handler. This in turn removes the only
users of hypercall_args_table[] which can be removed now.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/hvm/hypercall.c | 144 +++----------------------
xen/arch/x86/hypercall.c | 59 -----------
xen/arch/x86/pv/hypercall.c | 180 +++-----------------------------
xen/include/asm-x86/hypercall.h | 43 +++++---
4 files changed, 60 insertions(+), 366 deletions(-)
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 85b7a33523..e766cf4c72 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -108,56 +108,10 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
return compat_physdev_op(cmd, arg);
}
-#define HYPERCALL(x) \
- [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
- (hypercall_fn_t *) do_ ## x }
-
-#define HVM_CALL(x) \
- [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) hvm_ ## x, \
- (hypercall_fn_t *) hvm_ ## x }
-
-#define COMPAT_CALL(x) \
- [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
- (hypercall_fn_t *) compat_ ## x }
-
-static const struct {
- hypercall_fn_t *native, *compat;
-} hvm_hypercall_table[] = {
- HVM_CALL(memory_op),
- COMPAT_CALL(multicall),
-#ifdef CONFIG_GRANT_TABLE
- HVM_CALL(grant_table_op),
-#endif
- HYPERCALL(vm_assist),
- COMPAT_CALL(vcpu_op),
- HVM_CALL(physdev_op),
- COMPAT_CALL(xen_version),
- HYPERCALL(console_io),
- HYPERCALL(event_channel_op),
- COMPAT_CALL(sched_op),
- COMPAT_CALL(set_timer_op),
- COMPAT_CALL(xsm_op),
- HYPERCALL(hvm_op),
- HYPERCALL(sysctl),
- HYPERCALL(domctl),
-#ifdef CONFIG_ARGO
- COMPAT_CALL(argo_op),
-#endif
- COMPAT_CALL(platform_op),
-#ifdef CONFIG_PV
- COMPAT_CALL(mmuext_op),
-#endif
- HYPERCALL(xenpmu_op),
- COMPAT_CALL(dm_op),
-#ifdef CONFIG_HYPFS
- HYPERCALL(hypfs_op),
+#ifndef NDEBUG
+static unsigned char hypercall_args_64[] = hypercall_args_hvm64;
+static unsigned char hypercall_args_32[] = hypercall_args_hvm32;
#endif
- HYPERCALL(paging_domctl_cont)
-};
-
-#undef HYPERCALL
-#undef HVM_CALL
-#undef COMPAT_CALL
int hvm_hypercall(struct cpu_user_regs *regs)
{
@@ -203,23 +157,6 @@ int hvm_hypercall(struct cpu_user_regs *regs)
return ret;
}
- BUILD_BUG_ON(ARRAY_SIZE(hvm_hypercall_table) >
- ARRAY_SIZE(hypercall_args_table));
-
- if ( eax >= ARRAY_SIZE(hvm_hypercall_table) )
- {
- regs->rax = -ENOSYS;
- return HVM_HCALL_completed;
- }
-
- eax = array_index_nospec(eax, ARRAY_SIZE(hvm_hypercall_table));
-
- if ( !hvm_hypercall_table[eax].native )
- {
- regs->rax = -ENOSYS;
- return HVM_HCALL_completed;
- }
-
/*
* Caching is intended for instruction emulation only. Disable it
* for any accesses by hypercall argument copy-in / copy-out.
@@ -239,33 +176,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx)",
eax, rdi, rsi, rdx, r10, r8);
-#ifndef NDEBUG
- /* Deliberately corrupt parameter regs not used by this hypercall. */
- switch ( hypercall_args_table[eax].native )
- {
- case 0: rdi = 0xdeadbeefdeadf00dUL; fallthrough;
- case 1: rsi = 0xdeadbeefdeadf00dUL; fallthrough;
- case 2: rdx = 0xdeadbeefdeadf00dUL; fallthrough;
- case 3: r10 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 4: r8 = 0xdeadbeefdeadf00dUL;
- }
-#endif
-
- regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8);
+ call_handlers_hvm64(eax, regs->rax, rdi, rsi, rdx, r10, r8);
#ifndef NDEBUG
- if ( !curr->hcall_preempted )
- {
- /* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( hypercall_args_table[eax].native )
- {
- case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
- case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
- case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
- }
- }
+ if ( !curr->hcall_preempted && regs->rax != -ENOSYS )
+ clobber_regs(regs, hypercall_args_64[eax]);
#endif
}
else
@@ -279,35 +194,13 @@ int hvm_hypercall(struct cpu_user_regs *regs)
HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%x, %x, %x, %x, %x)", eax,
ebx, ecx, edx, esi, edi);
-#ifndef NDEBUG
- /* Deliberately corrupt parameter regs not used by this hypercall. */
- switch ( hypercall_args_table[eax].compat )
- {
- case 0: ebx = 0xdeadf00d; fallthrough;
- case 1: ecx = 0xdeadf00d; fallthrough;
- case 2: edx = 0xdeadf00d; fallthrough;
- case 3: esi = 0xdeadf00d; fallthrough;
- case 4: edi = 0xdeadf00d;
- }
-#endif
-
curr->hcall_compat = true;
- regs->eax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi);
+ call_handlers_hvm32(eax, regs->eax, ebx, ecx, edx, esi, edi);
curr->hcall_compat = false;
#ifndef NDEBUG
- if ( !curr->hcall_preempted )
- {
- /* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( hypercall_args_table[eax].compat )
- {
- case 5: regs->rdi = 0xdeadf00d; fallthrough;
- case 4: regs->rsi = 0xdeadf00d; fallthrough;
- case 3: regs->rdx = 0xdeadf00d; fallthrough;
- case 2: regs->rcx = 0xdeadf00d; fallthrough;
- case 1: regs->rbx = 0xdeadf00d;
- }
- }
+ if ( !curr->hcall_preempted && regs->eax != -ENOSYS )
+ clobber_regs32(regs, hypercall_args_32[eax]);
#endif
}
@@ -327,31 +220,20 @@ int hvm_hypercall(struct cpu_user_regs *regs)
enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
{
struct vcpu *curr = current;
- hypercall_fn_t *func = NULL;
if ( hvm_guest_x86_mode(curr) == 8 )
{
struct multicall_entry *call = &state->call;
- if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
- func = array_access_nospec(hvm_hypercall_table, call->op).native;
- if ( func )
- call->result = func(call->args[0], call->args[1], call->args[2],
- call->args[3], call->args[4]);
- else
- call->result = -ENOSYS;
+ call_handlers_hvm64(call->op, call->result, call->args[0], call->args[1],
+ call->args[2], call->args[3], call->args[4]);
}
else
{
struct compat_multicall_entry *call = &state->compat_call;
- if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
- func = array_access_nospec(hvm_hypercall_table, call->op).compat;
- if ( func )
- call->result = func(call->args[0], call->args[1], call->args[2],
- call->args[3], call->args[4]);
- else
- call->result = -ENOSYS;
+ call_handlers_hvm32(call->op, call->result, call->args[0], call->args[1],
+ call->args[2], call->args[3], call->args[4]);
}
return !hvm_get_cpl(curr) ? mc_continue : mc_preempt;
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 07e1a45ef5..6b73cff9b9 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -22,65 +22,6 @@
#include <xen/hypercall.h>
#include <asm/multicall.h>
-#ifdef CONFIG_COMPAT
-#define ARGS(x, n) \
- [ __HYPERVISOR_ ## x ] = { n, n }
-#define COMP(x, n, c) \
- [ __HYPERVISOR_ ## x ] = { n, c }
-#else
-#define ARGS(x, n) [ __HYPERVISOR_ ## x ] = { n }
-#define COMP(x, n, c) ARGS(x, n)
-#endif
-
-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),
- 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),
- COMP(update_descriptor, 2, 4),
- ARGS(memory_op, 2),
- ARGS(multicall, 2),
- 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),
- 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),
- ARGS(sched_op, 2),
- ARGS(callback_op, 2),
- ARGS(xenoprof_op, 2),
- ARGS(event_channel_op, 2),
- ARGS(physdev_op, 2),
- ARGS(sysctl, 1),
- ARGS(domctl, 1),
- ARGS(kexec_op, 2),
- ARGS(argo_op, 5),
- ARGS(xenpmu_op, 2),
- ARGS(hvm_op, 2),
- ARGS(dm_op, 3),
- ARGS(hypfs_op, 5),
- ARGS(mca, 1),
- ARGS(paging_domctl_cont, 1),
-};
-
-#undef COMP
-#undef ARGS
-
#define NEXT_ARG(fmt, args) \
({ \
unsigned long __arg; \
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 6c4a32d2a6..9b575e5c0b 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -27,119 +27,22 @@
#include <asm/multicall.h>
#include <irq_vectors.h>
-typedef struct {
- hypercall_fn_t *native;
-#ifdef CONFIG_PV32
- hypercall_fn_t *compat;
-#endif
-} pv_hypercall_table_t;
-
+#ifndef NDEBUG
+static unsigned char hypercall_args_64[] = hypercall_args_pv64;
#ifdef CONFIG_PV32
-#define HYPERCALL(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 }
-#else
-#define HYPERCALL(x) \
- [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x }
-#define COMPAT_CALL(x) HYPERCALL(x)
-#endif
-
-static const pv_hypercall_table_t pv_hypercall_table[] = {
- 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),
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
- COMPAT_CALL(platform_op),
-#endif
- 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),
-#ifdef CONFIG_GRANT_TABLE
- COMPAT_CALL(grant_table_op),
-#endif
- HYPERCALL(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),
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
- HYPERCALL(sysctl),
- HYPERCALL(domctl),
+static unsigned char hypercall_args_32[] = hypercall_args_pv32;
#endif
-#ifdef CONFIG_KEXEC
- COMPAT_CALL(kexec_op),
#endif
-#ifdef CONFIG_ARGO
- COMPAT_CALL(argo_op),
-#endif
- HYPERCALL(xenpmu_op),
-#ifdef CONFIG_HVM
- HYPERCALL(hvm_op),
- COMPAT_CALL(dm_op),
-#endif
-#ifdef CONFIG_HYPFS
- HYPERCALL(hypfs_op),
-#endif
- HYPERCALL(mca),
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
- HYPERCALL(paging_domctl_cont),
-#endif
-};
-
-#undef COMPAT_CALL
-#undef HYPERCALL
/* Forced inline to cause 'compat' to be evaluated at compile time. */
static void always_inline
_pv_hypercall(struct cpu_user_regs *regs, bool compat)
{
struct vcpu *curr = current;
- unsigned long eax = compat ? regs->eax : regs->rax;
+ unsigned long eax;
ASSERT(guest_kernel_mode(curr, regs));
- BUILD_BUG_ON(ARRAY_SIZE(pv_hypercall_table) >
- ARRAY_SIZE(hypercall_args_table));
-
- if ( eax >= ARRAY_SIZE(pv_hypercall_table) )
- {
- regs->rax = -ENOSYS;
- return;
- }
-
- eax = array_index_nospec(eax, ARRAY_SIZE(pv_hypercall_table));
-
- if ( !pv_hypercall_table[eax].native )
- {
- regs->rax = -ENOSYS;
- return;
- }
-
curr->hcall_preempted = false;
if ( !compat )
@@ -150,17 +53,8 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
unsigned long r10 = regs->r10;
unsigned long r8 = regs->r8;
-#ifndef NDEBUG
- /* Deliberately corrupt parameter regs not used by this hypercall. */
- switch ( hypercall_args_table[eax].native )
- {
- case 0: rdi = 0xdeadbeefdeadf00dUL; fallthrough;
- case 1: rsi = 0xdeadbeefdeadf00dUL; fallthrough;
- case 2: rdx = 0xdeadbeefdeadf00dUL; fallthrough;
- case 3: r10 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 4: r8 = 0xdeadbeefdeadf00dUL;
- }
-#endif
+ eax = regs->rax;
+
if ( unlikely(tb_init_done) )
{
unsigned long args[5] = { rdi, rsi, rdx, r10, r8 };
@@ -168,21 +62,11 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
__trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args);
}
- regs->rax = pv_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8);
+ call_handlers_pv64(eax, regs->rax, rdi, rsi, rdx, r10, r8);
#ifndef NDEBUG
- if ( !curr->hcall_preempted )
- {
- /* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( hypercall_args_table[eax].native )
- {
- case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
- case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
- case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
- case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
- }
- }
+ if ( !curr->hcall_preempted && regs->rax != -ENOSYS )
+ clobber_regs(regs, hypercall_args_64[eax]);
#endif
}
#ifdef CONFIG_PV32
@@ -194,17 +78,7 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
unsigned int esi = regs->esi;
unsigned int edi = regs->edi;
-#ifndef NDEBUG
- /* Deliberately corrupt parameter regs not used by this hypercall. */
- switch ( hypercall_args_table[eax].compat )
- {
- case 0: ebx = 0xdeadf00d; fallthrough;
- case 1: ecx = 0xdeadf00d; fallthrough;
- case 2: edx = 0xdeadf00d; fallthrough;
- case 3: esi = 0xdeadf00d; fallthrough;
- case 4: edi = 0xdeadf00d;
- }
-#endif
+ eax = regs->eax;
if ( unlikely(tb_init_done) )
{
@@ -214,22 +88,12 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
}
curr->hcall_compat = true;
- regs->eax = pv_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi);
+ call_handlers_pv32(eax, regs->eax, ebx, ecx, edx, esi, edi);
curr->hcall_compat = false;
#ifndef NDEBUG
- if ( !curr->hcall_preempted )
- {
- /* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( hypercall_args_table[eax].compat )
- {
- case 5: regs->edi = 0xdeadf00d; fallthrough;
- case 4: regs->esi = 0xdeadf00d; fallthrough;
- case 3: regs->edx = 0xdeadf00d; fallthrough;
- case 2: regs->ecx = 0xdeadf00d; fallthrough;
- case 1: regs->ebx = 0xdeadf00d;
- }
- }
+ if ( !curr->hcall_preempted && regs->eax != -ENOSYS )
+ clobber_regs32(regs, hypercall_args_32[eax]);
#endif
}
#endif /* CONFIG_PV32 */
@@ -256,13 +120,8 @@ enum mc_disposition pv_do_multicall_call(struct mc_state *state)
struct compat_multicall_entry *call = &state->compat_call;
op = call->op;
- if ( (op < ARRAY_SIZE(pv_hypercall_table)) &&
- pv_hypercall_table[op].compat )
- call->result = pv_hypercall_table[op].compat(
- call->args[0], call->args[1], call->args[2],
- call->args[3], call->args[4]);
- else
- call->result = -ENOSYS;
+ call_handlers_pv32(op, call->result, call->args[0], call->args[1],
+ call->args[2], call->args[3], call->args[4]);
}
else
#endif
@@ -270,13 +129,8 @@ enum mc_disposition pv_do_multicall_call(struct mc_state *state)
struct multicall_entry *call = &state->call;
op = call->op;
- if ( (op < ARRAY_SIZE(pv_hypercall_table)) &&
- pv_hypercall_table[op].native )
- call->result = pv_hypercall_table[op].native(
- call->args[0], call->args[1], call->args[2],
- call->args[3], call->args[4]);
- else
- call->result = -ENOSYS;
+ call_handlers_pv64(op, call->result, call->args[0], call->args[1],
+ call->args[2], call->args[3], call->args[4]);
}
return unlikely(op == __HYPERVISOR_iret)
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index eb2907b5b6..f2db3f3c21 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -17,19 +17,6 @@
#define __HYPERVISOR_paging_domctl_cont __HYPERVISOR_arch_1
-typedef unsigned long hypercall_fn_t(
- unsigned long, unsigned long, unsigned long,
- unsigned long, unsigned long);
-
-typedef struct {
- uint8_t native;
-#ifdef CONFIG_COMPAT
- uint8_t compat;
-#endif
-} hypercall_args_t;
-
-extern const hypercall_args_t hypercall_args_table[NR_hypercalls];
-
#ifdef CONFIG_PV
void pv_hypercall(struct cpu_user_regs *regs);
#endif
@@ -55,4 +42,34 @@ compat_common_vcpu_op(
#endif /* CONFIG_COMPAT */
+#ifndef NDEBUG
+static inline void clobber_regs(struct cpu_user_regs *regs,
+ unsigned int nargs)
+{
+ /* Deliberately corrupt used parameter regs. */
+ switch ( nargs )
+ {
+ case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
+ case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
+ case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
+ case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
+ case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
+ }
+}
+
+static inline void clobber_regs32(struct cpu_user_regs *regs,
+ unsigned int nargs)
+{
+ /* Deliberately corrupt used parameter regs. */
+ switch ( nargs )
+ {
+ case 5: regs->edi = 0xdeadf00dUL; fallthrough;
+ case 4: regs->esi = 0xdeadf00dUL; fallthrough;
+ case 3: regs->edx = 0xdeadf00dUL; fallthrough;
+ case 2: regs->ecx = 0xdeadf00dUL; fallthrough;
+ case 1: regs->ebx = 0xdeadf00dUL;
+ }
+}
+#endif
+
#endif /* __ASM_X86_HYPERCALL_H__ */
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 11/12] xen/arm: call hypercall handlers via switch statement
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (9 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 10/12] xen/x86: call hypercall handlers via switch statement Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 12/12] xen/x86: add hypercall performance counters for hvm, correct pv Juergen Gross
11 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Volodymyr Babchuk
Instead of using a function table use the generated switch statement
macros for calling the appropriate hypercall handlers.
This makes the calls of the handlers type safe.
For deprecated hypercalls define stub functions.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/arm/traps.c | 119 ++++++++++---------------------------------
1 file changed, 26 insertions(+), 93 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7abc28848e..72e914030f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1337,62 +1337,20 @@ static register_t do_deprecated_hypercall(void)
return -ENOSYS;
}
-typedef register_t (*arm_hypercall_fn_t)(
- register_t, register_t, register_t, register_t, register_t);
-
-typedef struct {
- arm_hypercall_fn_t fn;
- int nr_args;
-} arm_hypercall_t;
-
-#define HYPERCALL(_name, _nr_args) \
- [ __HYPERVISOR_ ## _name ] = { \
- .fn = (arm_hypercall_fn_t) &do_ ## _name, \
- .nr_args = _nr_args, \
- }
+long dep_sched_op_compat(int cmd, unsigned long arg)
+{
+ return do_deprecated_hypercall();
+}
-/*
- * Only use this for hypercalls which were deprecated (i.e. replaced
- * by something else) before Xen on ARM was created, i.e. *not* for
- * hypercalls which are simply not yet used on ARM.
- */
-#define HYPERCALL_DEPRECATED(_name, _nr_args) \
- [ __HYPERVISOR_##_name ] = { \
- .fn = (arm_hypercall_fn_t) &do_deprecated_hypercall, \
- .nr_args = _nr_args, \
- }
+long dep_event_channel_op_compat(XEN_GUEST_HANDLE_PARAM(evtchn_op_t) uop)
+{
+ return do_deprecated_hypercall();
+}
-static arm_hypercall_t arm_hypercall_table[] = {
- HYPERCALL(memory_op, 2),
- HYPERCALL(domctl, 1),
- HYPERCALL(sched_op, 2),
- HYPERCALL_DEPRECATED(sched_op_compat, 2),
- HYPERCALL(console_io, 3),
- HYPERCALL(xen_version, 2),
- HYPERCALL(xsm_op, 1),
- HYPERCALL(event_channel_op, 2),
- HYPERCALL_DEPRECATED(event_channel_op_compat, 1),
- HYPERCALL(physdev_op, 2),
- HYPERCALL_DEPRECATED(physdev_op_compat, 1),
- HYPERCALL(sysctl, 2),
- HYPERCALL(hvm_op, 2),
-#ifdef CONFIG_GRANT_TABLE
- HYPERCALL(grant_table_op, 3),
-#endif
- HYPERCALL(multicall, 2),
- HYPERCALL(platform_op, 1),
- HYPERCALL(vcpu_op, 3),
- HYPERCALL(vm_assist, 2),
-#ifdef CONFIG_ARGO
- HYPERCALL(argo_op, 5),
-#endif
-#ifdef CONFIG_HYPFS
- HYPERCALL(hypfs_op, 5),
-#endif
-#ifdef CONFIG_IOREQ_SERVER
- HYPERCALL(dm_op, 3),
-#endif
-};
+long dep_physdev_op_compat(XEN_GUEST_HANDLE_PARAM(physdev_op_t) uop)
+{
+ return do_deprecated_hypercall();
+}
#ifndef NDEBUG
static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
@@ -1431,7 +1389,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
#define HYPERCALL_ARG3(r) (r)->x2
#define HYPERCALL_ARG4(r) (r)->x3
#define HYPERCALL_ARG5(r) (r)->x4
-#define HYPERCALL_ARGS(r) (r)->x0, (r)->x1, (r)->x2, (r)->x3, (r)->x4
#else
#define HYPERCALL_RESULT_REG(r) (r)->r0
#define HYPERCALL_ARG1(r) (r)->r0
@@ -1439,52 +1396,40 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
#define HYPERCALL_ARG3(r) (r)->r2
#define HYPERCALL_ARG4(r) (r)->r3
#define HYPERCALL_ARG5(r) (r)->r4
-#define HYPERCALL_ARGS(r) (r)->r0, (r)->r1, (r)->r2, (r)->r3, (r)->r4
#endif
+static unsigned char hypercall_args[] = hypercall_args_arm;
+
static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
const union hsr hsr)
{
- arm_hypercall_fn_t call = NULL;
struct vcpu *curr = current;
- BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
-
if ( hsr.iss != XEN_HYPERCALL_TAG )
{
gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
return inject_undef_exception(regs, hsr);
}
- if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
- {
- perfc_incr(invalid_hypercalls);
- HYPERCALL_RESULT_REG(regs) = -ENOSYS;
- return;
- }
-
curr->hcall_preempted = false;
perfc_incra(hypercalls, *nr);
- call = arm_hypercall_table[*nr].fn;
- if ( call == NULL )
- {
- HYPERCALL_RESULT_REG(regs) = -ENOSYS;
- return;
- }
- HYPERCALL_RESULT_REG(regs) = call(HYPERCALL_ARGS(regs));
+ call_handlers_arm(*nr, HYPERCALL_RESULT_REG(regs), HYPERCALL_ARG1(regs),
+ HYPERCALL_ARG2(regs), HYPERCALL_ARG3(regs),
+ HYPERCALL_ARG4(regs), HYPERCALL_ARG5(regs));
#ifndef NDEBUG
- if ( !curr->hcall_preempted )
+ if ( !curr->hcall_preempted && HYPERCALL_RESULT_REG(regs) != -ENOSYS )
{
/* Deliberately corrupt parameter regs used by this hypercall. */
- switch ( arm_hypercall_table[*nr].nr_args ) {
+ switch ( hypercall_args[*nr] ) {
case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEF;
case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEF;
case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEF;
case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEF;
case 1: /* Don't clobber x0/r0 -- it's the return value */
+ case 0: /* -ENOSYS case */
break;
default: BUG();
}
@@ -1521,7 +1466,10 @@ static bool check_multicall_32bit_clean(struct multicall_entry *multi)
{
int i;
- for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ )
+ if ( multi->op >= ARRAY_SIZE(hypercall_args) )
+ return true;
+
+ for ( i = 0; i < hypercall_args[multi->op]; i++ )
{
if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) )
{
@@ -1538,28 +1486,13 @@ static bool check_multicall_32bit_clean(struct multicall_entry *multi)
enum mc_disposition 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) )
- {
- multi->result = -ENOSYS;
- return mc_continue;
- }
-
- call = arm_hypercall_table[multi->op].fn;
- if ( call == NULL )
- {
- multi->result = -ENOSYS;
- return mc_continue;
- }
if ( is_32bit_domain(current->domain) &&
!check_multicall_32bit_clean(multi) )
return mc_continue;
- multi->result = call(multi->args[0], multi->args[1],
- multi->args[2], multi->args[3],
- multi->args[4]);
+ call_handlers_arm(multi->op, multi->result, multi->args[0], multi->args[1],
+ multi->args[2], multi->args[3], multi->args[4]);
return likely(!psr_mode_is_user(guest_cpu_user_regs()))
? mc_continue : mc_preempt;
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 12/12] xen/x86: add hypercall performance counters for hvm, correct pv
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
` (10 preceding siblings ...)
2021-10-15 12:51 ` [PATCH 11/12] xen/arm: " Juergen Gross
@ 2021-10-15 12:51 ` Juergen Gross
2021-10-21 15:19 ` Jan Beulich
11 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 12:51 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu
The HVM hypercall handler is missing incrementing the per hypercall
counters. Add that.
The counters for PV are handled wrong, as they are not using
perf_incra() with the number of the hypercall as index, but are
incrementing the total number of hypercalls only. Fix that.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/hvm/hypercall.c | 2 ++
xen/arch/x86/pv/hypercall.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index e766cf4c72..599921dc48 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -214,6 +214,8 @@ int hvm_hypercall(struct cpu_user_regs *regs)
ioreq_signal_mapcache_invalidate();
}
+ perfc_incra(hypercalls, eax);
+
return curr->hcall_preempted ? HVM_HCALL_preempted : HVM_HCALL_completed;
}
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 9b575e5c0b..ec8d2c7f87 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -106,7 +106,7 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat)
if ( curr->hcall_preempted )
regs->rip -= 2;
- perfc_incr(hypercalls);
+ perfc_incra(hypercalls, eax);
}
enum mc_disposition pv_do_multicall_call(struct mc_state *state)
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 08/12] x86/pv-shim: don't modify hypercall table
2021-10-15 12:51 ` [PATCH 08/12] x86/pv-shim: don't modify hypercall table Juergen Gross
@ 2021-10-15 13:57 ` Jan Beulich
2021-10-15 14:23 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-15 13:57 UTC (permalink / raw)
To: Juergen Gross
Cc: Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, xen-devel
On 15.10.2021 14:51, Juergen Gross wrote:
> When running as pv-shim the hypercall is modified today in order to
> replace the functions for __HYPERVISOR_event_channel_op and
> __HYPERVISOR_grant_table_op hypercalls.
>
> Change this to call the related functions from the normal handlers
> instead when running as shim. The performance implications are not
> really relevant, as a normal production hypervisor will not be
> configured to support shim mode, so the related calls will be dropped
> due to optimization of the compiler.
>
> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy
> wrapper do_grant_table_op() needed, as in this case grant_table.c
> isn't being built.
While you say CONFIG_PV_SHIM_EXCLUSIVE here, ...
> @@ -845,6 +822,23 @@ static long pv_shim_grant_table_op(unsigned int cmd,
> return rc;
> }
>
> +#ifndef CONFIG_GRANT_TABLE
... you don't actually enforce this here. I also don't see why it would
be needed in the "exclusive" case only. A binary usable both ways would
still need these, wouldn't it?
> +/* Thin wrapper(s) needed. */
> +long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
> + unsigned int count)
> +{
> + return pv_shim_grant_table_op(cmd, uop, count);
> +}
> +
> +#ifdef CONFIG_PV32
> +int compat_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
> + unsigned int count)
> +{
> + return pv_shim_grant_table_op(cmd, uop, count);
> +}
> +#endif
> +#endif
Don't you also need to adjust the respective #ifdef in
pv_hypercall_table[]? Otherwise I don't see how, at this point of the
series, the functions would actually get hooked up. In a bi-modal
binary further guarding will then be needed inside the wrappers, I
think. (While the table is going to go away, that guarding is going
to be needed even at the end of this series, I believe.)
Talking of wrappers - do you need actual code to be emitted for this?
Can't you simply put in place an alias each, which is permitted now
that pv_shim_grant_table_op() isn't static anymore? (Albeit that's
partly moot if guarding code gets added to the functions - in that
case only compat_grant_table_op() could become an alias of
do_grant_table_op().)
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 08/12] x86/pv-shim: don't modify hypercall table
2021-10-15 13:57 ` Jan Beulich
@ 2021-10-15 14:23 ` Juergen Gross
0 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-15 14:23 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2840 bytes --]
On 15.10.21 15:57, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
>> When running as pv-shim the hypercall is modified today in order to
>> replace the functions for __HYPERVISOR_event_channel_op and
>> __HYPERVISOR_grant_table_op hypercalls.
>>
>> Change this to call the related functions from the normal handlers
>> instead when running as shim. The performance implications are not
>> really relevant, as a normal production hypervisor will not be
>> configured to support shim mode, so the related calls will be dropped
>> due to optimization of the compiler.
>>
>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy
>> wrapper do_grant_table_op() needed, as in this case grant_table.c
>> isn't being built.
>
> While you say CONFIG_PV_SHIM_EXCLUSIVE here, ...
>
>> @@ -845,6 +822,23 @@ static long pv_shim_grant_table_op(unsigned int cmd,
>> return rc;
>> }
>>
>> +#ifndef CONFIG_GRANT_TABLE
>
> ... you don't actually enforce this here. I also don't see why it would
> be needed in the "exclusive" case only. A binary usable both ways would
> still need these, wouldn't it?
The "exclusive" case is normally the one where CONFIG_GRANT_TABLE is not
set. I highlighted this as it is a common situation.
>
>> +/* Thin wrapper(s) needed. */
>> +long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
>> + unsigned int count)
>> +{
>> + return pv_shim_grant_table_op(cmd, uop, count);
>> +}
>> +
>> +#ifdef CONFIG_PV32
>> +int compat_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
>> + unsigned int count)
>> +{
>> + return pv_shim_grant_table_op(cmd, uop, count);
>> +}
>> +#endif
>> +#endif
>
> Don't you also need to adjust the respective #ifdef in
> pv_hypercall_table[]? Otherwise I don't see how, at this point of the
> series, the functions would actually get hooked up.
Ah, right.
> In a bi-modal
> binary further guarding will then be needed inside the wrappers, I
> think. (While the table is going to go away, that guarding is going
> to be needed even at the end of this series, I believe.)
Oh, you mean for the weird case of !pv_shim? Yes, I need to return
-ENOSYS in this case.
> Talking of wrappers - do you need actual code to be emitted for this?
> Can't you simply put in place an alias each, which is permitted now
> that pv_shim_grant_table_op() isn't static anymore? (Albeit that's
> partly moot if guarding code gets added to the functions - in that
> case only compat_grant_table_op() could become an alias of
> do_grant_table_op().)
I didn't think of an alias. But I don't think I can make
compat_grant_table_op() an alias of do_grant_table_op(), as they have
different return types.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 03/12] xen: harmonize return types of hypercall handlers
2021-10-15 12:51 ` [PATCH 03/12] xen: harmonize return types of hypercall handlers Juergen Gross
@ 2021-10-18 11:55 ` Jan Beulich
2021-10-18 13:24 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-18 11:55 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 15.10.2021 14:51, Juergen Gross wrote:
> Today most hypercall handlers have a return type of long, while the
> compat ones return an int. There are a few exceptions from that rule,
> however.
>
> Get rid of the exceptions by letting compat handlers always return int
> and others always return long.
>
> For the compat hvm case use eax instead of rax for the stored result as
> it should have been from the beginning.
>
> Additionally move some prototypes to include/asm-x86/hypercall.h
> as they are x86 specific. Move the do_physdev_op() prototype from both
> architecture dependant headers to the common one. Move the
> compat_platform_op() prototype to the common header.
>
> Switch some non style compliant types (u32, s32, s64) to style compliant
> ones.
>
> Rename paging_domctl_continuation() to do_paging_domctl_cont() and add
> a matching define for the associated hypercall.
>
> Make do_callback_op() and compat_callback_op() more similar by adding
> the const attribute to compat_callback_op()'s 2nd parameter.
>
> The do_platform_op() prototype needs to be modified in order to better
> match its compat variant.
"Better" in what direction? So far both have been using typed handles,
which I consider better than void ones. You also don't seem to have
had a reason to switch e.g. multicall or dm_op, where (different)
typed handles are also in use. So I wonder what the reason for this
change is.
> Change the type of the cmd parameter for [do|compat]_kexec_op() to
> unsigned int, as this is more appropriate for the compat case.
The change for the compat case is fine, but for native you change
behavior for callers passing values equaling valid KEXEC_CMD_*
modulo 2³².
> --- a/xen/arch/x86/pv/misc-hypercalls.c
> +++ b/xen/arch/x86/pv/misc-hypercalls.c
> @@ -28,12 +28,16 @@ long do_set_debugreg(int reg, unsigned long value)
> return set_debugreg(current, reg, value);
> }
>
> -unsigned long do_get_debugreg(int reg)
> +long do_get_debugreg(int reg)
> {
> - unsigned long val;
> - int res = x86emul_read_dr(reg, &val, NULL);
> -
> - return res == X86EMUL_OKAY ? val : -ENODEV;
> + /* Avoid undefined behavior due to casting an unsigned long to long. */
Nit: unsigned -> signed conversion is implementation-defined, not
undefined.
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -2207,13 +2207,13 @@ do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> }
>
> #ifdef CONFIG_COMPAT
> -long
> -compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> - XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> - unsigned long arg4)
> +int compat_argo_op(unsigned int cmd,
> + XEN_GUEST_HANDLE_PARAM(void) arg1,
> + XEN_GUEST_HANDLE_PARAM(void) arg2,
> + unsigned long arg3, unsigned long arg4)
Strictly speaking arg3 and arg4 also ought to be unsigned int here.
But that's perhaps for a separate patch at another time.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype
2021-10-15 12:51 ` [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype Juergen Gross
@ 2021-10-18 12:31 ` Jan Beulich
2021-10-18 13:27 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-18 12:31 UTC (permalink / raw)
To: Juergen Gross; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 15.10.2021 14:51, Juergen Gross wrote:
> hvm_memory_op() should take an unsigned long as cmd, like
> do_memory_op().
>
> As hvm_memory_op() is basically just calling do_memory_op() (or
> compat_memory_op()) passing through the parameters the cmd parameter
> should have no smaller size than that of the called functions.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Nevertheless ...
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -31,7 +31,7 @@
> #include <public/hvm/hvm_op.h>
> #include <public/hvm/params.h>
>
> -static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> +static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> long rc;
... I think this would even better be dealt with by splitting the
function into a native one (using unsigned long) and a compat one
(using unsigned int).
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 05/12] xen: don't include asm/hypercall.h from C sources
2021-10-15 12:51 ` [PATCH 05/12] xen: don't include asm/hypercall.h from C sources Juergen Gross
@ 2021-10-18 12:39 ` Jan Beulich
2021-10-18 15:20 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-18 12:39 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Roger Pau Monné,
xen-devel
On 15.10.2021 14:51, Juergen Gross wrote:
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -32,7 +32,6 @@ EMIT_FILE;
> #include <asm/page.h>
> #include <asm/flushtlb.h>
> #include <asm/fixmap.h>
> -#include <asm/hypercall.h>
> #include <asm/msr.h>
> #include <asm/pv/domain.h>
> #include <asm/setup.h>
Afaict xen/hypercall.h also doesn't need including here. But I can
agree that this is at best a partly related change.
> --- a/xen/arch/x86/x86_64/platform_hypercall.c
> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
> @@ -41,8 +41,8 @@ CHECK_pf_resource_entry;
> #undef xen_pf_resource_entry
>
> #define COMPAT
> -#define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
> -#define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
> +#undef guest_handle_okay
> +#define guest_handle_okay compat_handle_okay
Is this a change that slipped in here from patch 3, accompanying
-#define do_platform_op(x) compat_platform_op(_##x)
+#define do_platform_op(x) compat_platform_op(x)
there? Or does that change belong here? I have to admit anyway that
it's not entirely clear to me why this adjustment needs making.
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -4,6 +4,7 @@
> */
>
> #include <compat/grant_table.h>
> +#include <xen/hypercall.h>
Nit: Generally compat/*.h go last.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/12] xen: generate hypercall interface related code
2021-10-15 12:51 ` [PATCH 06/12] xen: generate hypercall interface related code Juergen Gross
@ 2021-10-18 12:58 ` Jan Beulich
2021-10-18 15:28 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-18 12:58 UTC (permalink / raw)
To: Juergen Gross
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
On 15.10.2021 14:51, Juergen Gross wrote:
> Instead of repeating similar data multiple times use a single source
> file and a generator script for producing prototypes and call sequences
> of the hypercalls.
>
> As the script already knows the number of parameters used add generating
> a macro for populating an array with the number of parameters per
> hypercall.
Isn't that array intended to go away?
> --- a/.gitignore
> +++ b/.gitignore
> @@ -332,10 +332,12 @@ xen/include/asm-x86/asm-macros.h
> xen/include/compat/*
> xen/include/config/
> xen/include/generated/
> +xen/include//hypercall-defs.i
Nit: Stray double slash (unless this has a meaning I'm unaware of).
Yet then I wonder: Shouldn't *.i be among the patterns at the top of
the file, like *.o is?
> @@ -466,6 +468,14 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: asm-offsets.s
> echo ""; \
> echo "#endif") <$< >$@
>
> +quiet_cmd_genhyp = GEN $@
> +define cmd_genhyp
> + awk -f scripts/gen_hypercall.awk <$< >$@
> +endef
> +
> +include/xen/hypercall-defs.h: include/hypercall-defs.i scripts/gen_hypercall.awk FORCE
> + $(call if_changed,genhyp)
As per patch 5 there are quite a few sources including xen/hypercall.h
and hence (in one of the next patches) the header generated here. If
this one gets re-generated for a benign reason (i.e. without changing
the header), all dependents will get rebuilt for no reason. Use
$(move-if-changed ...)?
> +prefix: do
> +set_timer_op(s_time_t timeout)
> +console_io(unsigned int cmd, unsigned int count, char *buffer)
> +vm_assist(unsigned int cmd, unsigned int type)
> +event_channel_op(int cmd, void *arg)
> +mmuext_op(mmuext_op_t *uops, unsigned int count, unsigned int *pdone, unsigned int foreigndom)
> +multicall(multicall_entry_t *call_list, unsigned int nr_calls)
> +#ifdef CONFIG_PV
> +mmu_update(mmu_update_t *ureqs, unsigned int count, unsigned int *pdone, unsigned int foreigndom)
> +stack_switch(unsigned long ss, unsigned long esp)
> +fpu_taskswitch(int set)
> +set_debugreg(int reg, unsigned long value)
> +get_debugreg(int reg)
> +set_segment_base(unsigned int which, unsigned long base)
> +mca(xen_mc_t *u_xen_mc)
> +set_trap_table(const_trap_info_t *traps)
> +set_gdt(xen_ulong_t *frame_list, unsigned int entries)
> +set_callbacks(unsigned long event_address, unsigned long failsafe_address, unsigned long syscall_address)
> +update_descriptor(uint64_t gaddr, seg_desc_t desc)
> +update_va_mapping(unsigned long va, uint64_t val64, unsigned long flags)
> +update_va_mapping_otherdomain(unsigned long va, uint64_t val64, unsigned long flags, domid_t domid)
> +#endif
> +#ifdef CONFIG_IOREQ_SERVER
> +dm_op(domid_t domid, unsigned int nr_bufs, xen_dm_op_buf_t *bufs)
> +#endif
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +sysctl(xen_sysctl_t *u_sysctl)
> +domctl(xen_domctl_t *u_domctl)
> +paging_domctl_cont(xen_domctl_t *u_domctl)
> +#endif
> +#ifdef CONFIG_HVM
> +hvm_op(unsigned long op, void *arg)
> +#endif
> +#ifdef CONFIG_HYPFS
> +hypfs_op(unsigned int cmd, const char *arg1, unsigned long arg2, void *arg3, unsigned long arg4)
> +#endif
> +#ifdef CONFIG_X86
> +xenpmu_op(unsigned int op, xen_pmu_params_t *arg)
> +#endif
> +
> +#ifdef CONFIG_PV
> +caller: pv64
> +#ifdef CONFIG_PV32
> +caller: pv32
> +#endif
> +#endif
> +#ifdef CONFIG_HVM
> +caller: hvm64
> +#ifdef CONFIG_COMPAT
> +caller: hvm32
> +#endif
HVM selects COMPAT, so I don't see a point in this inner conditional.
> +#endif
> +#ifdef CONFIG_ARM
> +caller: arm
> +#endif
> +
> +table: pv32 pv64 hvm32 hvm64 arm
> +set_trap_table compat do - - -
> +mmu_update do do - - -
> +set_gdt compat do - - -
> +stack_switch do do - - -
> +set_callbacks compat do - - -
> +fpu_taskswitch do do - - -
> +sched_op_compat do do - - dep
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +platform_op compat do compat do do
> +#endif
> +set_debugreg do do - - -
> +get_debugreg do do - - -
> +update_descriptor compat do - - -
> +memory_op compat do hvm hvm do
> +multicall compat do compat do do
> +update_va_mapping compat do - - -
> +set_timer_op compat do compat do -
> +event_channel_op_compat do do - - dep
> +xen_version compat do compat do do
> +console_io do do do do do
> +physdev_op_compat compat do - - dep
> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
> +grant_table_op compat do hvm hvm do
> +#endif
> +vm_assist do do do do do
> +update_va_mapping_otherdomain compat do - - -
> +iret compat do - - -
> +vcpu_op compat do compat do do
> +set_segment_base do do - - -
> +#ifdef CONFIG_PV
> +mmuext_op compat do compat do -
> +#endif
> +xsm_op compat do compat do do
> +nmi_op compat do - - -
> +sched_op compat do compat do do
> +callback_op compat do - - -
> +#ifdef CONFIG_XENOPROF
> +xenoprof_op compat do - - -
> +#endif
> +event_channel_op do do do do do
> +physdev_op compat do hvm hvm do
> +#ifdef CONFIG_HVM
> +hvm_op do do do do do
> +#endif
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +sysctl do do do do do
> +domctl do do do do do
> +#endif
> +#ifdef CONFIG_KEXEC
> +kexec_op compat do - - -
> +#endif
> +tmem_op - - - - -
> +#ifdef CONFIG_ARGO
> +argo_op compat do compat do do
> +#endif
> +xenpmu_op do do do do -
> +#ifdef CONFIG_IOREQ_SERVER
> +dm_op compat do compat do do
> +#endif
> +#ifdef CONFIG_HYPFS
> +hypfs_op do do do do do
> +#endif
> +mca do do - - -
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +paging_domctl_cont do do do do -
> +#endif
I assume the intention here is to sort by hypercall number. This results
in 3 PV_SHIM_EXCLUSIVE conditionals when one might do. Just a remark for
consideration, not strictly a request to change anything.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 07/12] xen: use generated prototypes for hypercall handlers
2021-10-15 12:51 ` [PATCH 07/12] xen: use generated prototypes for hypercall handlers Juergen Gross
@ 2021-10-18 13:01 ` Jan Beulich
0 siblings, 0 replies; 38+ messages in thread
From: Jan Beulich @ 2021-10-18 13:01 UTC (permalink / raw)
To: Juergen Gross
Cc: Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, xen-devel
On 15.10.2021 14:51, Juergen Gross wrote:
> Remove the hypercall handler's prototypes in the related header files
> and use the generated ones instead.
>
> Some handlers having been static before need to be made globally
> visible.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 03/12] xen: harmonize return types of hypercall handlers
2021-10-18 11:55 ` Jan Beulich
@ 2021-10-18 13:24 ` Juergen Gross
2021-10-18 14:25 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-18 13:24 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 4070 bytes --]
On 18.10.21 13:55, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
>> Today most hypercall handlers have a return type of long, while the
>> compat ones return an int. There are a few exceptions from that rule,
>> however.
>>
>> Get rid of the exceptions by letting compat handlers always return int
>> and others always return long.
>>
>> For the compat hvm case use eax instead of rax for the stored result as
>> it should have been from the beginning.
>>
>> Additionally move some prototypes to include/asm-x86/hypercall.h
>> as they are x86 specific. Move the do_physdev_op() prototype from both
>> architecture dependant headers to the common one. Move the
>> compat_platform_op() prototype to the common header.
>>
>> Switch some non style compliant types (u32, s32, s64) to style compliant
>> ones.
>>
>> Rename paging_domctl_continuation() to do_paging_domctl_cont() and add
>> a matching define for the associated hypercall.
>>
>> Make do_callback_op() and compat_callback_op() more similar by adding
>> the const attribute to compat_callback_op()'s 2nd parameter.
>>
>> The do_platform_op() prototype needs to be modified in order to better
>> match its compat variant.
>
> "Better" in what direction? So far both have been using typed handles,
> which I consider better than void ones. You also don't seem to have
> had a reason to switch e.g. multicall or dm_op, where (different)
> typed handles are also in use. So I wonder what the reason for this
> change is.
I had some problems making this build. Before my patches
platform_hypercall.c didn't include the header with the prototype,
so there was no mismatch detected.
Thanks for the pointers above. dm_op is no good example, as the
compat variant is explicitly a different implementation compared
to the normal one. But with the multicall example I can have
another try converting platform_op to a type safe variant using
non-void handles.
>
>> Change the type of the cmd parameter for [do|compat]_kexec_op() to
>> unsigned int, as this is more appropriate for the compat case.
>
> The change for the compat case is fine, but for native you change
> behavior for callers passing values equaling valid KEXEC_CMD_*
> modulo 2³².
TBH, I don't think this is really a problem. Or do you think there
really is a user of this interface relying on a -ENOSYS in this
case?
>
>> --- a/xen/arch/x86/pv/misc-hypercalls.c
>> +++ b/xen/arch/x86/pv/misc-hypercalls.c
>> @@ -28,12 +28,16 @@ long do_set_debugreg(int reg, unsigned long value)
>> return set_debugreg(current, reg, value);
>> }
>>
>> -unsigned long do_get_debugreg(int reg)
>> +long do_get_debugreg(int reg)
>> {
>> - unsigned long val;
>> - int res = x86emul_read_dr(reg, &val, NULL);
>> -
>> - return res == X86EMUL_OKAY ? val : -ENODEV;
>> + /* Avoid undefined behavior due to casting an unsigned long to long. */
>
> Nit: unsigned -> signed conversion is implementation-defined, not
> undefined.
Okay, will change the comment.
>
>> --- a/xen/common/argo.c
>> +++ b/xen/common/argo.c
>> @@ -2207,13 +2207,13 @@ do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
>> }
>>
>> #ifdef CONFIG_COMPAT
>> -long
>> -compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
>> - XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
>> - unsigned long arg4)
>> +int compat_argo_op(unsigned int cmd,
>> + XEN_GUEST_HANDLE_PARAM(void) arg1,
>> + XEN_GUEST_HANDLE_PARAM(void) arg2,
>> + unsigned long arg3, unsigned long arg4)
>
> Strictly speaking arg3 and arg4 also ought to be unsigned int here.
> But that's perhaps for a separate patch at another time.
I'd rather leave it as is, as this way I can use the same definition for
both cases in patch 6. And I don't see how anything could go wrong this
way, as expanding a 32-bit unsigned value to 64 bits is in no way
ambiguous.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype
2021-10-18 12:31 ` Jan Beulich
@ 2021-10-18 13:27 ` Juergen Gross
2021-10-18 14:28 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-18 13:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 1234 bytes --]
On 18.10.21 14:31, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
>> hvm_memory_op() should take an unsigned long as cmd, like
>> do_memory_op().
>>
>> As hvm_memory_op() is basically just calling do_memory_op() (or
>> compat_memory_op()) passing through the parameters the cmd parameter
>> should have no smaller size than that of the called functions.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Nevertheless ...
>
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -31,7 +31,7 @@
>> #include <public/hvm/hvm_op.h>
>> #include <public/hvm/params.h>
>>
>> -static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> {
>> long rc;
>
> ... I think this would even better be dealt with by splitting the
> function into a native one (using unsigned long) and a compat one
> (using unsigned int).
Why? In 32-bit case the value is naturally limited to 32 bits width
zero-extending perfectly fine to unsigned long.
Otherwise I couldn't use the same definition later.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 03/12] xen: harmonize return types of hypercall handlers
2021-10-18 13:24 ` Juergen Gross
@ 2021-10-18 14:25 ` Jan Beulich
2021-10-18 15:31 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-18 14:25 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 18.10.2021 15:24, Juergen Gross wrote:
> On 18.10.21 13:55, Jan Beulich wrote:
>> On 15.10.2021 14:51, Juergen Gross wrote:
>>> Change the type of the cmd parameter for [do|compat]_kexec_op() to
>>> unsigned int, as this is more appropriate for the compat case.
>>
>> The change for the compat case is fine, but for native you change
>> behavior for callers passing values equaling valid KEXEC_CMD_*
>> modulo 2³².
>
> TBH, I don't think this is really a problem. Or do you think there
> really is a user of this interface relying on a -ENOSYS in this
> case?
That's a secondary consideration of mine only. The primary one is
that invoking with an invalid sub-op should fail, such that in the
future we can assign meaning to the upper bits, if need be. See
their use for continuations in memory-op, for example.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype
2021-10-18 13:27 ` Juergen Gross
@ 2021-10-18 14:28 ` Jan Beulich
2021-10-18 15:34 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-18 14:28 UTC (permalink / raw)
To: Juergen Gross; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 18.10.2021 15:27, Juergen Gross wrote:
> On 18.10.21 14:31, Jan Beulich wrote:
>> On 15.10.2021 14:51, Juergen Gross wrote:
>>> hvm_memory_op() should take an unsigned long as cmd, like
>>> do_memory_op().
>>>
>>> As hvm_memory_op() is basically just calling do_memory_op() (or
>>> compat_memory_op()) passing through the parameters the cmd parameter
>>> should have no smaller size than that of the called functions.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Nevertheless ...
>>
>>> --- a/xen/arch/x86/hvm/hypercall.c
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -31,7 +31,7 @@
>>> #include <public/hvm/hvm_op.h>
>>> #include <public/hvm/params.h>
>>>
>>> -static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>> +static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>> {
>>> long rc;
>>
>> ... I think this would even better be dealt with by splitting the
>> function into a native one (using unsigned long) and a compat one
>> (using unsigned int).
>
> Why? In 32-bit case the value is naturally limited to 32 bits width
> zero-extending perfectly fine to unsigned long.
It all ends up working fine, yes. Else I wouldn't have given R-b.
But the .compat slot of the hypercall table really should use a
prototype without unsigned long, and then the calls wouldn't
zero-extend the arguments anymore. And then the declaration would
be wrong, as then it would need to be the callee to zero-extend if
it wants to use 64-bit values.
> Otherwise I couldn't use the same definition later.
Right. And this will be less of a problem once the function pointer
tables are gone, as then the compiler sees the real parameter types
for the individual functions.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 05/12] xen: don't include asm/hypercall.h from C sources
2021-10-18 12:39 ` Jan Beulich
@ 2021-10-18 15:20 ` Juergen Gross
0 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-18 15:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Roger Pau Monné,
xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 1702 bytes --]
On 18.10.21 14:39, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
>> --- a/xen/arch/x86/x86_64/mm.c
>> +++ b/xen/arch/x86/x86_64/mm.c
>> @@ -32,7 +32,6 @@ EMIT_FILE;
>> #include <asm/page.h>
>> #include <asm/flushtlb.h>
>> #include <asm/fixmap.h>
>> -#include <asm/hypercall.h>
>> #include <asm/msr.h>
>> #include <asm/pv/domain.h>
>> #include <asm/setup.h>
>
> Afaict xen/hypercall.h also doesn't need including here. But I can
> agree that this is at best a partly related change.
Will check and delete it in case not needed.
>
>> --- a/xen/arch/x86/x86_64/platform_hypercall.c
>> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
>> @@ -41,8 +41,8 @@ CHECK_pf_resource_entry;
>> #undef xen_pf_resource_entry
>>
>> #define COMPAT
>> -#define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
>> -#define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
>> +#undef guest_handle_okay
>> +#define guest_handle_okay compat_handle_okay
>
> Is this a change that slipped in here from patch 3, accompanying
>
> -#define do_platform_op(x) compat_platform_op(_##x)
> +#define do_platform_op(x) compat_platform_op(x)
>
> there? Or does that change belong here? I have to admit anyway that
> it's not entirely clear to me why this adjustment needs making.
Oh yes, this is a leftover to make things work with prototypes being
made seen in platform_hypercall.c.
>
>> --- a/xen/common/compat/grant_table.c
>> +++ b/xen/common/compat/grant_table.c
>> @@ -4,6 +4,7 @@
>> */
>>
>> #include <compat/grant_table.h>
>> +#include <xen/hypercall.h>
>
> Nit: Generally compat/*.h go last.
Okay.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/12] xen: generate hypercall interface related code
2021-10-18 12:58 ` Jan Beulich
@ 2021-10-18 15:28 ` Juergen Gross
2021-10-18 15:39 ` Jan Beulich
2021-10-20 7:02 ` Juergen Gross
0 siblings, 2 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-18 15:28 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 8457 bytes --]
On 18.10.21 14:58, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
>> Instead of repeating similar data multiple times use a single source
>> file and a generator script for producing prototypes and call sequences
>> of the hypercalls.
>>
>> As the script already knows the number of parameters used add generating
>> a macro for populating an array with the number of parameters per
>> hypercall.
>
> Isn't that array intended to go away?
I thought so, yes, but on Arm there is a case where it is needed.
So generating it from the available data is the sensible thing to do
IMO.
>
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -332,10 +332,12 @@ xen/include/asm-x86/asm-macros.h
>> xen/include/compat/*
>> xen/include/config/
>> xen/include/generated/
>> +xen/include//hypercall-defs.i
>
> Nit: Stray double slash (unless this has a meaning I'm unaware of).
Oh, right. No special meaning AFAIK.
> Yet then I wonder: Shouldn't *.i be among the patterns at the top of
> the file, like *.o is?
Yes, I can do that. Probably via a separate patch then.
>> @@ -466,6 +468,14 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: asm-offsets.s
>> echo ""; \
>> echo "#endif") <$< >$@
>>
>> +quiet_cmd_genhyp = GEN $@
>> +define cmd_genhyp
>> + awk -f scripts/gen_hypercall.awk <$< >$@
>> +endef
>> +
>> +include/xen/hypercall-defs.h: include/hypercall-defs.i scripts/gen_hypercall.awk FORCE
>> + $(call if_changed,genhyp)
>
> As per patch 5 there are quite a few sources including xen/hypercall.h
> and hence (in one of the next patches) the header generated here. If
> this one gets re-generated for a benign reason (i.e. without changing
> the header), all dependents will get rebuilt for no reason. Use
> $(move-if-changed ...)?
The reasons for re-generating are quite limited. The most probable case
is a .config change, which will trigger quite some rebuild anyway.
If you think its is really worth the effort, I can use move-if-changed.
>
>> +prefix: do
>> +set_timer_op(s_time_t timeout)
>> +console_io(unsigned int cmd, unsigned int count, char *buffer)
>> +vm_assist(unsigned int cmd, unsigned int type)
>> +event_channel_op(int cmd, void *arg)
>> +mmuext_op(mmuext_op_t *uops, unsigned int count, unsigned int *pdone, unsigned int foreigndom)
>> +multicall(multicall_entry_t *call_list, unsigned int nr_calls)
>> +#ifdef CONFIG_PV
>> +mmu_update(mmu_update_t *ureqs, unsigned int count, unsigned int *pdone, unsigned int foreigndom)
>> +stack_switch(unsigned long ss, unsigned long esp)
>> +fpu_taskswitch(int set)
>> +set_debugreg(int reg, unsigned long value)
>> +get_debugreg(int reg)
>> +set_segment_base(unsigned int which, unsigned long base)
>> +mca(xen_mc_t *u_xen_mc)
>> +set_trap_table(const_trap_info_t *traps)
>> +set_gdt(xen_ulong_t *frame_list, unsigned int entries)
>> +set_callbacks(unsigned long event_address, unsigned long failsafe_address, unsigned long syscall_address)
>> +update_descriptor(uint64_t gaddr, seg_desc_t desc)
>> +update_va_mapping(unsigned long va, uint64_t val64, unsigned long flags)
>> +update_va_mapping_otherdomain(unsigned long va, uint64_t val64, unsigned long flags, domid_t domid)
>> +#endif
>> +#ifdef CONFIG_IOREQ_SERVER
>> +dm_op(domid_t domid, unsigned int nr_bufs, xen_dm_op_buf_t *bufs)
>> +#endif
>> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
>> +sysctl(xen_sysctl_t *u_sysctl)
>> +domctl(xen_domctl_t *u_domctl)
>> +paging_domctl_cont(xen_domctl_t *u_domctl)
>> +#endif
>> +#ifdef CONFIG_HVM
>> +hvm_op(unsigned long op, void *arg)
>> +#endif
>> +#ifdef CONFIG_HYPFS
>> +hypfs_op(unsigned int cmd, const char *arg1, unsigned long arg2, void *arg3, unsigned long arg4)
>> +#endif
>> +#ifdef CONFIG_X86
>> +xenpmu_op(unsigned int op, xen_pmu_params_t *arg)
>> +#endif
>> +
>> +#ifdef CONFIG_PV
>> +caller: pv64
>> +#ifdef CONFIG_PV32
>> +caller: pv32
>> +#endif
>> +#endif
>> +#ifdef CONFIG_HVM
>> +caller: hvm64
>> +#ifdef CONFIG_COMPAT
>> +caller: hvm32
>> +#endif
>
> HVM selects COMPAT, so I don't see a point in this inner conditional.
Ah, indeed.
>
>> +#endif
>> +#ifdef CONFIG_ARM
>> +caller: arm
>> +#endif
>> +
>> +table: pv32 pv64 hvm32 hvm64 arm
>> +set_trap_table compat do - - -
>> +mmu_update do do - - -
>> +set_gdt compat do - - -
>> +stack_switch do do - - -
>> +set_callbacks compat do - - -
>> +fpu_taskswitch do do - - -
>> +sched_op_compat do do - - dep
>> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
>> +platform_op compat do compat do do
>> +#endif
>> +set_debugreg do do - - -
>> +get_debugreg do do - - -
>> +update_descriptor compat do - - -
>> +memory_op compat do hvm hvm do
>> +multicall compat do compat do do
>> +update_va_mapping compat do - - -
>> +set_timer_op compat do compat do -
>> +event_channel_op_compat do do - - dep
>> +xen_version compat do compat do do
>> +console_io do do do do do
>> +physdev_op_compat compat do - - dep
>> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
>> +grant_table_op compat do hvm hvm do
>> +#endif
>> +vm_assist do do do do do
>> +update_va_mapping_otherdomain compat do - - -
>> +iret compat do - - -
>> +vcpu_op compat do compat do do
>> +set_segment_base do do - - -
>> +#ifdef CONFIG_PV
>> +mmuext_op compat do compat do -
>> +#endif
>> +xsm_op compat do compat do do
>> +nmi_op compat do - - -
>> +sched_op compat do compat do do
>> +callback_op compat do - - -
>> +#ifdef CONFIG_XENOPROF
>> +xenoprof_op compat do - - -
>> +#endif
>> +event_channel_op do do do do do
>> +physdev_op compat do hvm hvm do
>> +#ifdef CONFIG_HVM
>> +hvm_op do do do do do
>> +#endif
>> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
>> +sysctl do do do do do
>> +domctl do do do do do
>> +#endif
>> +#ifdef CONFIG_KEXEC
>> +kexec_op compat do - - -
>> +#endif
>> +tmem_op - - - - -
>> +#ifdef CONFIG_ARGO
>> +argo_op compat do compat do do
>> +#endif
>> +xenpmu_op do do do do -
>> +#ifdef CONFIG_IOREQ_SERVER
>> +dm_op compat do compat do do
>> +#endif
>> +#ifdef CONFIG_HYPFS
>> +hypfs_op do do do do do
>> +#endif
>> +mca do do - - -
>> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
>> +paging_domctl_cont do do do do -
>> +#endif
>
> I assume the intention here is to sort by hypercall number. This results
> in 3 PV_SHIM_EXCLUSIVE conditionals when one might do. Just a remark for
> consideration, not strictly a request to change anything.
I intentionally sorted this by hypercall number, yes.
This makes it much easier to spot any missing case IMO. I can change
that if wanted.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 03/12] xen: harmonize return types of hypercall handlers
2021-10-18 14:25 ` Jan Beulich
@ 2021-10-18 15:31 ` Juergen Gross
0 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-18 15:31 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 1032 bytes --]
On 18.10.21 16:25, Jan Beulich wrote:
> On 18.10.2021 15:24, Juergen Gross wrote:
>> On 18.10.21 13:55, Jan Beulich wrote:
>>> On 15.10.2021 14:51, Juergen Gross wrote:
>>>> Change the type of the cmd parameter for [do|compat]_kexec_op() to
>>>> unsigned int, as this is more appropriate for the compat case.
>>>
>>> The change for the compat case is fine, but for native you change
>>> behavior for callers passing values equaling valid KEXEC_CMD_*
>>> modulo 2³².
>>
>> TBH, I don't think this is really a problem. Or do you think there
>> really is a user of this interface relying on a -ENOSYS in this
>> case?
>
> That's a secondary consideration of mine only. The primary one is
> that invoking with an invalid sub-op should fail, such that in the
> future we can assign meaning to the upper bits, if need be. See
> their use for continuations in memory-op, for example.
But this would mean to exclude such usage of upper bits for 32 bit
systems in case this difference should matter.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype
2021-10-18 14:28 ` Jan Beulich
@ 2021-10-18 15:34 ` Juergen Gross
0 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-18 15:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2035 bytes --]
On 18.10.21 16:28, Jan Beulich wrote:
> On 18.10.2021 15:27, Juergen Gross wrote:
>> On 18.10.21 14:31, Jan Beulich wrote:
>>> On 15.10.2021 14:51, Juergen Gross wrote:
>>>> hvm_memory_op() should take an unsigned long as cmd, like
>>>> do_memory_op().
>>>>
>>>> As hvm_memory_op() is basically just calling do_memory_op() (or
>>>> compat_memory_op()) passing through the parameters the cmd parameter
>>>> should have no smaller size than that of the called functions.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Nevertheless ...
>>>
>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>> @@ -31,7 +31,7 @@
>>>> #include <public/hvm/hvm_op.h>
>>>> #include <public/hvm/params.h>
>>>>
>>>> -static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>> +static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>> {
>>>> long rc;
>>>
>>> ... I think this would even better be dealt with by splitting the
>>> function into a native one (using unsigned long) and a compat one
>>> (using unsigned int).
>>
>> Why? In 32-bit case the value is naturally limited to 32 bits width
>> zero-extending perfectly fine to unsigned long.
>
> It all ends up working fine, yes. Else I wouldn't have given R-b.
> But the .compat slot of the hypercall table really should use a
> prototype without unsigned long, and then the calls wouldn't
> zero-extend the arguments anymore. And then the declaration would
> be wrong, as then it would need to be the callee to zero-extend if
> it wants to use 64-bit values.
>
>> Otherwise I couldn't use the same definition later.
>
> Right. And this will be less of a problem once the function pointer
> tables are gone, as then the compiler sees the real parameter types
> for the individual functions.
Okay, I understand that.
I'd prefer to do that as a followup patch (series) then.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/12] xen: generate hypercall interface related code
2021-10-18 15:28 ` Juergen Gross
@ 2021-10-18 15:39 ` Jan Beulich
2021-10-20 7:02 ` Juergen Gross
1 sibling, 0 replies; 38+ messages in thread
From: Jan Beulich @ 2021-10-18 15:39 UTC (permalink / raw)
To: Juergen Gross
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
On 18.10.2021 17:28, Juergen Gross wrote:
> On 18.10.21 14:58, Jan Beulich wrote:
>> On 15.10.2021 14:51, Juergen Gross wrote:
>>> Instead of repeating similar data multiple times use a single source
>>> file and a generator script for producing prototypes and call sequences
>>> of the hypercalls.
>>>
>>> As the script already knows the number of parameters used add generating
>>> a macro for populating an array with the number of parameters per
>>> hypercall.
>>
>> Isn't that array intended to go away?
>
> I thought so, yes, but on Arm there is a case where it is needed.
>
> So generating it from the available data is the sensible thing to do
> IMO.
Absolutely, if such a table continues to be needed.
>>> @@ -466,6 +468,14 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: asm-offsets.s
>>> echo ""; \
>>> echo "#endif") <$< >$@
>>>
>>> +quiet_cmd_genhyp = GEN $@
>>> +define cmd_genhyp
>>> + awk -f scripts/gen_hypercall.awk <$< >$@
>>> +endef
>>> +
>>> +include/xen/hypercall-defs.h: include/hypercall-defs.i scripts/gen_hypercall.awk FORCE
>>> + $(call if_changed,genhyp)
>>
>> As per patch 5 there are quite a few sources including xen/hypercall.h
>> and hence (in one of the next patches) the header generated here. If
>> this one gets re-generated for a benign reason (i.e. without changing
>> the header), all dependents will get rebuilt for no reason. Use
>> $(move-if-changed ...)?
>
> The reasons for re-generating are quite limited. The most probable case
> is a .config change, which will trigger quite some rebuild anyway.
Oh, good point - I should also have considered the dependencies here,
which are pretty limited. Please disregard my remark then.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/12] xen: generate hypercall interface related code
2021-10-18 15:28 ` Juergen Gross
2021-10-18 15:39 ` Jan Beulich
@ 2021-10-20 7:02 ` Juergen Gross
2021-10-20 7:11 ` Jan Beulich
1 sibling, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-20 7:02 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 927 bytes --]
On 18.10.21 17:28, Juergen Gross wrote:
> On 18.10.21 14:58, Jan Beulich wrote:
>> On 15.10.2021 14:51, Juergen Gross wrote:
>>> --- a/.gitignore
>>> +++ b/.gitignore
>>> @@ -332,10 +332,12 @@ xen/include/asm-x86/asm-macros.h
>>> xen/include/compat/*
>>> xen/include/config/
>>> xen/include/generated/
>>> +xen/include//hypercall-defs.i
>>
>> Nit: Stray double slash (unless this has a meaning I'm unaware of).
>
> Oh, right. No special meaning AFAIK.
>
>> Yet then I wonder: Shouldn't *.i be among the patterns at the top of
>> the file, like *.o is?
>
> Yes, I can do that. Probably via a separate patch then.
I can't do that, as we have one source file in our git tree matching
this pattern: tools/libs/stat/bindings/swig/xenstat.i is used as an
input file for swig for generating perl and python bindings. And the
.i suffix seems to be the common one for swig input files.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/12] xen: generate hypercall interface related code
2021-10-20 7:02 ` Juergen Gross
@ 2021-10-20 7:11 ` Jan Beulich
2021-10-20 7:18 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-20 7:11 UTC (permalink / raw)
To: Juergen Gross
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
On 20.10.2021 09:02, Juergen Gross wrote:
> On 18.10.21 17:28, Juergen Gross wrote:
>> On 18.10.21 14:58, Jan Beulich wrote:
>>> On 15.10.2021 14:51, Juergen Gross wrote:
>>>> --- a/.gitignore
>>>> +++ b/.gitignore
>>>> @@ -332,10 +332,12 @@ xen/include/asm-x86/asm-macros.h
>>>> xen/include/compat/*
>>>> xen/include/config/
>>>> xen/include/generated/
>>>> +xen/include//hypercall-defs.i
>>>
>>> Nit: Stray double slash (unless this has a meaning I'm unaware of).
>>
>> Oh, right. No special meaning AFAIK.
>>
>>> Yet then I wonder: Shouldn't *.i be among the patterns at the top of
>>> the file, like *.o is?
>>
>> Yes, I can do that. Probably via a separate patch then.
>
> I can't do that, as we have one source file in our git tree matching
> this pattern: tools/libs/stat/bindings/swig/xenstat.i is used as an
> input file for swig for generating perl and python bindings. And the
> .i suffix seems to be the common one for swig input files.
Ugly. Since we have a rule to produce *.i in xen/Rules.mk, I think we
really should have these ignored. Perhaps a good enough reason to put
*.i in xen/.gitignore? And while at it perhaps also *.s? Unless
there's a way to specify a pattern for an entire subtree - it's not
clear to me whether xen/*.i in ./.gitignore would cover subdirs of
xen/ as well ...
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/12] xen: generate hypercall interface related code
2021-10-20 7:11 ` Jan Beulich
@ 2021-10-20 7:18 ` Juergen Gross
0 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-20 7:18 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 1750 bytes --]
On 20.10.21 09:11, Jan Beulich wrote:
> On 20.10.2021 09:02, Juergen Gross wrote:
>> On 18.10.21 17:28, Juergen Gross wrote:
>>> On 18.10.21 14:58, Jan Beulich wrote:
>>>> On 15.10.2021 14:51, Juergen Gross wrote:
>>>>> --- a/.gitignore
>>>>> +++ b/.gitignore
>>>>> @@ -332,10 +332,12 @@ xen/include/asm-x86/asm-macros.h
>>>>> xen/include/compat/*
>>>>> xen/include/config/
>>>>> xen/include/generated/
>>>>> +xen/include//hypercall-defs.i
>>>>
>>>> Nit: Stray double slash (unless this has a meaning I'm unaware of).
>>>
>>> Oh, right. No special meaning AFAIK.
>>>
>>>> Yet then I wonder: Shouldn't *.i be among the patterns at the top of
>>>> the file, like *.o is?
>>>
>>> Yes, I can do that. Probably via a separate patch then.
>>
>> I can't do that, as we have one source file in our git tree matching
>> this pattern: tools/libs/stat/bindings/swig/xenstat.i is used as an
>> input file for swig for generating perl and python bindings. And the
>> .i suffix seems to be the common one for swig input files.
>
> Ugly. Since we have a rule to produce *.i in xen/Rules.mk, I think we
> really should have these ignored. Perhaps a good enough reason to put
> *.i in xen/.gitignore? And while at it perhaps also *.s? Unless
> there's a way to specify a pattern for an entire subtree - it's not
> clear to me whether xen/*.i in ./.gitignore would cover subdirs of
> xen/ as well ...
xen/**/*.i will do that. From the gitignore syntax description:
A slash followed by two consecutive asterisks then a slash matches
zero or more directories. For example, "a/**/b" matches "a/b",
"a/x/b", "a/x/y/b" and so on.
So I'll go with adding xen/**/*.i and xen/**/*.s to .gitignore.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 10/12] xen/x86: call hypercall handlers via switch statement
2021-10-15 12:51 ` [PATCH 10/12] xen/x86: call hypercall handlers via switch statement Juergen Gross
@ 2021-10-21 14:41 ` Jan Beulich
2021-10-28 14:32 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-21 14:41 UTC (permalink / raw)
To: Juergen Gross; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 15.10.2021 14:51, Juergen Gross wrote:
> Instead of using a function table use the generated switch statement
> macros for calling the appropriate hypercall handlers.
>
> This is beneficial to performance and avoids speculation issues.
>
> With calling the handlers using the correct number of parameters now
> it is possible to do the parameter register clobbering in the NDEBUG
> case after returning from the handler. This in turn removes the only
> users of hypercall_args_table[] which can be removed now.
"removed" reads misleading to me: You really replace it by new tables,
using script-generated initializers. Also it looks like you're doubling
the data, as the same sets were previously used by pv64/hvm64 and
pv32/hvm32 respectively.
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -108,56 +108,10 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> return compat_physdev_op(cmd, arg);
> }
>
> -#define HYPERCALL(x) \
> - [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
> - (hypercall_fn_t *) do_ ## x }
> -
> -#define HVM_CALL(x) \
> - [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) hvm_ ## x, \
> - (hypercall_fn_t *) hvm_ ## x }
> -
> -#define COMPAT_CALL(x) \
> - [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
> - (hypercall_fn_t *) compat_ ## x }
> -
> -static const struct {
> - hypercall_fn_t *native, *compat;
> -} hvm_hypercall_table[] = {
> - HVM_CALL(memory_op),
> - COMPAT_CALL(multicall),
> -#ifdef CONFIG_GRANT_TABLE
> - HVM_CALL(grant_table_op),
> -#endif
> - HYPERCALL(vm_assist),
> - COMPAT_CALL(vcpu_op),
> - HVM_CALL(physdev_op),
> - COMPAT_CALL(xen_version),
> - HYPERCALL(console_io),
> - HYPERCALL(event_channel_op),
> - COMPAT_CALL(sched_op),
> - COMPAT_CALL(set_timer_op),
> - COMPAT_CALL(xsm_op),
> - HYPERCALL(hvm_op),
> - HYPERCALL(sysctl),
> - HYPERCALL(domctl),
> -#ifdef CONFIG_ARGO
> - COMPAT_CALL(argo_op),
> -#endif
> - COMPAT_CALL(platform_op),
> -#ifdef CONFIG_PV
> - COMPAT_CALL(mmuext_op),
> -#endif
> - HYPERCALL(xenpmu_op),
> - COMPAT_CALL(dm_op),
> -#ifdef CONFIG_HYPFS
> - HYPERCALL(hypfs_op),
> +#ifndef NDEBUG
> +static unsigned char hypercall_args_64[] = hypercall_args_hvm64;
> +static unsigned char hypercall_args_32[] = hypercall_args_hvm32;
Irrespective of this being debugging-only: Const?
> @@ -239,33 +176,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
> HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx)",
> eax, rdi, rsi, rdx, r10, r8);
>
> -#ifndef NDEBUG
> - /* Deliberately corrupt parameter regs not used by this hypercall. */
> - switch ( hypercall_args_table[eax].native )
> - {
> - case 0: rdi = 0xdeadbeefdeadf00dUL; fallthrough;
> - case 1: rsi = 0xdeadbeefdeadf00dUL; fallthrough;
> - case 2: rdx = 0xdeadbeefdeadf00dUL; fallthrough;
> - case 3: r10 = 0xdeadbeefdeadf00dUL; fallthrough;
> - case 4: r8 = 0xdeadbeefdeadf00dUL;
> - }
> -#endif
> -
> - regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8);
> + call_handlers_hvm64(eax, regs->rax, rdi, rsi, rdx, r10, r8);
>
> #ifndef NDEBUG
> - if ( !curr->hcall_preempted )
> - {
> - /* Deliberately corrupt parameter regs used by this hypercall. */
> - switch ( hypercall_args_table[eax].native )
> - {
> - case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
> - case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
> - case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
> - case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
> - case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
> - }
> - }
> + if ( !curr->hcall_preempted && regs->rax != -ENOSYS )
> + clobber_regs(regs, hypercall_args_64[eax]);
I'm not fundamentally opposed, but sadly -ENOSYS comes back also in undue
situations, e.g. various hypercalls still produce this for "unknown
sub-function". Hence the weakened clobbering wants at least mentioning,
perhaps also justifying, in the description.
> @@ -55,4 +42,34 @@ compat_common_vcpu_op(
>
> #endif /* CONFIG_COMPAT */
>
> +#ifndef NDEBUG
Hmm, I was actuall hoping for the conditional to actually live ...
> +static inline void clobber_regs(struct cpu_user_regs *regs,
> + unsigned int nargs)
> +{
... here and ...
> + /* Deliberately corrupt used parameter regs. */
> + switch ( nargs )
> + {
> + case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
> + case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
> + case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
> + case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
> + case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
> + }
> +}
> +
> +static inline void clobber_regs32(struct cpu_user_regs *regs,
> + unsigned int nargs)
> +{
... here, such that the conditionals in the .c files could go away
altogether.
> + /* Deliberately corrupt used parameter regs. */
> + switch ( nargs )
> + {
> + case 5: regs->edi = 0xdeadf00dUL; fallthrough;
> + case 4: regs->esi = 0xdeadf00dUL; fallthrough;
> + case 3: regs->edx = 0xdeadf00dUL; fallthrough;
> + case 2: regs->ecx = 0xdeadf00dUL; fallthrough;
> + case 1: regs->ebx = 0xdeadf00dUL;
No need for the UL suffixes here afaics; U ones may want to be there.
Overall, besides these mainly cosmetic aspects the main thing missing
is an approach to prioritize the handful most frequently used functions,
for them to be pulled out of the switch() so we don't depend on the
compiler's choice for the order of comparisons done.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 12/12] xen/x86: add hypercall performance counters for hvm, correct pv
2021-10-15 12:51 ` [PATCH 12/12] xen/x86: add hypercall performance counters for hvm, correct pv Juergen Gross
@ 2021-10-21 15:19 ` Jan Beulich
2021-10-28 14:35 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-10-21 15:19 UTC (permalink / raw)
To: Juergen Gross; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 15.10.2021 14:51, Juergen Gross wrote:
> The HVM hypercall handler is missing incrementing the per hypercall
> counters. Add that.
>
> The counters for PV are handled wrong, as they are not using
> perf_incra() with the number of the hypercall as index, but are
> incrementing the total number of hypercalls only. Fix that.
Why do you say "total number"? Isn't it that all accounting goes into
set_trap_table's slot, effectively making that slot a "total number"
despite not being labeled that way?
Also this fix renders largely redundant the calls_to_multicall counter.
Could I talk you into deleting that at the same time? (As to the "not
fully redundant": I consider it suspicious that this counter gets
incremented at the bottom of the function, not at the top.)
Finally I take it that with the Kconfig setting being under DEBUG, we
don't consider security supported builds with PERF_COUNTERS enabled.
Otherwise as a prereq I would think perfc_incra() would need teaching
of array_index_nospec().
In any event, preferably with at least the description slightly
adjusted,
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 10/12] xen/x86: call hypercall handlers via switch statement
2021-10-21 14:41 ` Jan Beulich
@ 2021-10-28 14:32 ` Juergen Gross
2021-11-02 9:54 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Juergen Gross @ 2021-10-28 14:32 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 10064 bytes --]
On 21.10.21 16:41, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
>> Instead of using a function table use the generated switch statement
>> macros for calling the appropriate hypercall handlers.
>>
>> This is beneficial to performance and avoids speculation issues.
>>
>> With calling the handlers using the correct number of parameters now
>> it is possible to do the parameter register clobbering in the NDEBUG
>> case after returning from the handler. This in turn removes the only
>> users of hypercall_args_table[] which can be removed now.
>
> "removed" reads misleading to me: You really replace it by new tables,
> using script-generated initializers. Also it looks like you're doubling
> the data, as the same sets were previously used by pv64/hvm64 and
> pv32/hvm32 respectively.
Yes, I'll change that paragraph.
Regarding having 4 tables on x86 now: merging the pv/hvm tables would be
possible, but this would add some complexity to the script generating
the tables (it should test whether the number of parameters of pv and
hvm match). As the tables are present in debug build only I don't think
this is a real issue.
>
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -108,56 +108,10 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> return compat_physdev_op(cmd, arg);
>> }
>>
>> -#define HYPERCALL(x) \
>> - [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
>> - (hypercall_fn_t *) do_ ## x }
>> -
>> -#define HVM_CALL(x) \
>> - [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) hvm_ ## x, \
>> - (hypercall_fn_t *) hvm_ ## x }
>> -
>> -#define COMPAT_CALL(x) \
>> - [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \
>> - (hypercall_fn_t *) compat_ ## x }
>> -
>> -static const struct {
>> - hypercall_fn_t *native, *compat;
>> -} hvm_hypercall_table[] = {
>> - HVM_CALL(memory_op),
>> - COMPAT_CALL(multicall),
>> -#ifdef CONFIG_GRANT_TABLE
>> - HVM_CALL(grant_table_op),
>> -#endif
>> - HYPERCALL(vm_assist),
>> - COMPAT_CALL(vcpu_op),
>> - HVM_CALL(physdev_op),
>> - COMPAT_CALL(xen_version),
>> - HYPERCALL(console_io),
>> - HYPERCALL(event_channel_op),
>> - COMPAT_CALL(sched_op),
>> - COMPAT_CALL(set_timer_op),
>> - COMPAT_CALL(xsm_op),
>> - HYPERCALL(hvm_op),
>> - HYPERCALL(sysctl),
>> - HYPERCALL(domctl),
>> -#ifdef CONFIG_ARGO
>> - COMPAT_CALL(argo_op),
>> -#endif
>> - COMPAT_CALL(platform_op),
>> -#ifdef CONFIG_PV
>> - COMPAT_CALL(mmuext_op),
>> -#endif
>> - HYPERCALL(xenpmu_op),
>> - COMPAT_CALL(dm_op),
>> -#ifdef CONFIG_HYPFS
>> - HYPERCALL(hypfs_op),
>> +#ifndef NDEBUG
>> +static unsigned char hypercall_args_64[] = hypercall_args_hvm64;
>> +static unsigned char hypercall_args_32[] = hypercall_args_hvm32;
>
> Irrespective of this being debugging-only: Const?
Yes.
>
>> @@ -239,33 +176,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>> HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx)",
>> eax, rdi, rsi, rdx, r10, r8);
>>
>> -#ifndef NDEBUG
>> - /* Deliberately corrupt parameter regs not used by this hypercall. */
>> - switch ( hypercall_args_table[eax].native )
>> - {
>> - case 0: rdi = 0xdeadbeefdeadf00dUL; fallthrough;
>> - case 1: rsi = 0xdeadbeefdeadf00dUL; fallthrough;
>> - case 2: rdx = 0xdeadbeefdeadf00dUL; fallthrough;
>> - case 3: r10 = 0xdeadbeefdeadf00dUL; fallthrough;
>> - case 4: r8 = 0xdeadbeefdeadf00dUL;
>> - }
>> -#endif
>> -
>> - regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8);
>> + call_handlers_hvm64(eax, regs->rax, rdi, rsi, rdx, r10, r8);
>>
>> #ifndef NDEBUG
>> - if ( !curr->hcall_preempted )
>> - {
>> - /* Deliberately corrupt parameter regs used by this hypercall. */
>> - switch ( hypercall_args_table[eax].native )
>> - {
>> - case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
>> - case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
>> - case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
>> - case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
>> - case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
>> - }
>> - }
>> + if ( !curr->hcall_preempted && regs->rax != -ENOSYS )
>> + clobber_regs(regs, hypercall_args_64[eax]);
>
> I'm not fundamentally opposed, but sadly -ENOSYS comes back also in undue
> situations, e.g. various hypercalls still produce this for "unknown
> sub-function". Hence the weakened clobbering wants at least mentioning,
> perhaps also justifying, in the description.
Okay.
>
>> @@ -55,4 +42,34 @@ compat_common_vcpu_op(
>>
>> #endif /* CONFIG_COMPAT */
>>
>> +#ifndef NDEBUG
>
> Hmm, I was actuall hoping for the conditional to actually live ...
>
>> +static inline void clobber_regs(struct cpu_user_regs *regs,
>> + unsigned int nargs)
>> +{
>
> ... here and ...
>
>> + /* Deliberately corrupt used parameter regs. */
>> + switch ( nargs )
>> + {
>> + case 5: regs->r8 = 0xdeadbeefdeadf00dUL; fallthrough;
>> + case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
>> + case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
>> + case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
>> + case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
>> + }
>> +}
>> +
>> +static inline void clobber_regs32(struct cpu_user_regs *regs,
>> + unsigned int nargs)
>> +{
>
> ... here, such that the conditionals in the .c files could go away
> altogether.
I didn't do that in order to be able to have the tables with the
number of parameters inside #ifndef NDEBUG sections.
I think I can change that by using a macro for reading the table
values.
>
>> + /* Deliberately corrupt used parameter regs. */
>> + switch ( nargs )
>> + {
>> + case 5: regs->edi = 0xdeadf00dUL; fallthrough;
>> + case 4: regs->esi = 0xdeadf00dUL; fallthrough;
>> + case 3: regs->edx = 0xdeadf00dUL; fallthrough;
>> + case 2: regs->ecx = 0xdeadf00dUL; fallthrough;
>> + case 1: regs->ebx = 0xdeadf00dUL;
>
> No need for the UL suffixes here afaics; U ones may want to be there.
Okay.
> Overall, besides these mainly cosmetic aspects the main thing missing
> is an approach to prioritize the handful most frequently used functions,
> for them to be pulled out of the switch() so we don't depend on the
> compiler's choice for the order of comparisons done.
I have already prepared that step by generating the complete call
sequence, so any change for prioritizing some hypercalls can be local to
the generator script and the used input data.
The main question is how to do that. I've collected some hypercall
statistics data for PV and PVH guests running some simple tests (once a
build of the Xen hypervisor, and once a scp of a large file). The data
is split between guest and dom0 (PV) counts. There is no clear "winner"
which hypercall should be fastest, but several hypercalls are clearly
not important.
Here is the data:
PV-hypercall PV-guest build PV-guest scp dom0 build dom0 scp
mmu_update 186175729 2865 20936 33725
stack_switch 1273311 62381 108589 270764
multicall 2182803 50 302 524
update_va_mapping 571868 10 60 80
xen_version 73061 850 859 5432
grant_table_op 0 0 35557 139110
iret 75673006 484132 268157 757958
vcpu_op 453037 71199 138224 334988
set_segment_base 1650249 62387 108645 270823
mmuext_op 11225681 188 7239 3426
sched_op 280153 134645 70729 137943
event_channel_op 192327 66204 71409 214191
physdev_op 0 0 7721 4315
(the dom0 values are for the guest running the build or scp test, so
dom0 acting as backend)
HVM-hypercall PVH-guest build PVH-guest scp
vcpu_op 277684 2324
event_channel_op 350233 57383
(the related dom0 counter values are in the same range as with the test
running in the PV guest)
It should be noted that during boot of the guests the numbers for the PV
guest are more like the ones for the build test with the exception of
iret and sched_op being higher, while for PVH sched_op is by far the
most often used hypercall.
I'm not sure how to translate those numbers into a good algorithm for
generating the call sequence.
I could add priorities to each hypercall in hypercall-defs.c and have a
cascade of if (likely(foo)) call_foo; else if (likely(bla)) ... else
switch(rest).
Or I could have groups of hypercalls with a priority for each group and:
mask = 1ULL << num;
if (likely(mask & prio_1_mask)) switch(num) ...
else if (likely(mask & prio_2_mask)) switch (num) ...
...
else switch (num) ...
Or I could combine those approaches using the mask variant for cases of
multiple entries having the same priority and the direct call variant
for the cases of only a single entry having a specific priority.
And then there is the problem to set the priorities (fairly simple for
HVM, PV is more diffcult).
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 12/12] xen/x86: add hypercall performance counters for hvm, correct pv
2021-10-21 15:19 ` Jan Beulich
@ 2021-10-28 14:35 ` Juergen Gross
0 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-10-28 14:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 1342 bytes --]
On 21.10.21 17:19, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
>> The HVM hypercall handler is missing incrementing the per hypercall
>> counters. Add that.
>>
>> The counters for PV are handled wrong, as they are not using
>> perf_incra() with the number of the hypercall as index, but are
>> incrementing the total number of hypercalls only. Fix that.
>
> Why do you say "total number"? Isn't it that all accounting goes into
> set_trap_table's slot, effectively making that slot a "total number"
> despite not being labeled that way?
Oh, right. Will correct.
> Also this fix renders largely redundant the calls_to_multicall counter.
> Could I talk you into deleting that at the same time? (As to the "not
> fully redundant": I consider it suspicious that this counter gets
> incremented at the bottom of the function, not at the top.)
I think I'll just another patch doing that.
> Finally I take it that with the Kconfig setting being under DEBUG, we
> don't consider security supported builds with PERF_COUNTERS enabled.
> Otherwise as a prereq I would think perfc_incra() would need teaching
> of array_index_nospec().
I agree.
> In any event, preferably with at least the description slightly
> adjusted,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 10/12] xen/x86: call hypercall handlers via switch statement
2021-10-28 14:32 ` Juergen Gross
@ 2021-11-02 9:54 ` Jan Beulich
2021-11-02 10:04 ` Juergen Gross
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2021-11-02 9:54 UTC (permalink / raw)
To: Juergen Gross; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 28.10.2021 16:32, Juergen Gross wrote:
> On 21.10.21 16:41, Jan Beulich wrote:
>> On 15.10.2021 14:51, Juergen Gross wrote:
>>> Instead of using a function table use the generated switch statement
>>> macros for calling the appropriate hypercall handlers.
>>>
>>> This is beneficial to performance and avoids speculation issues.
>>>
>>> With calling the handlers using the correct number of parameters now
>>> it is possible to do the parameter register clobbering in the NDEBUG
>>> case after returning from the handler. This in turn removes the only
>>> users of hypercall_args_table[] which can be removed now.
>>
>> "removed" reads misleading to me: You really replace it by new tables,
>> using script-generated initializers. Also it looks like you're doubling
>> the data, as the same sets were previously used by pv64/hvm64 and
>> pv32/hvm32 respectively.
>
> Yes, I'll change that paragraph.
>
> Regarding having 4 tables on x86 now: merging the pv/hvm tables would be
> possible, but this would add some complexity to the script generating
> the tables (it should test whether the number of parameters of pv and
> hvm match). As the tables are present in debug build only I don't think
> this is a real issue.
Sure, but that imo wants saying in the description.
>> Overall, besides these mainly cosmetic aspects the main thing missing
>> is an approach to prioritize the handful most frequently used functions,
>> for them to be pulled out of the switch() so we don't depend on the
>> compiler's choice for the order of comparisons done.
>
> I have already prepared that step by generating the complete call
> sequence, so any change for prioritizing some hypercalls can be local to
> the generator script and the used input data.
>
> The main question is how to do that. I've collected some hypercall
> statistics data for PV and PVH guests running some simple tests (once a
> build of the Xen hypervisor, and once a scp of a large file). The data
> is split between guest and dom0 (PV) counts. There is no clear "winner"
> which hypercall should be fastest, but several hypercalls are clearly
> not important.
>
> Here is the data:
>
> PV-hypercall PV-guest build PV-guest scp dom0 build dom0 scp
> mmu_update 186175729 2865 20936 33725
Builds should be local to the guest and I/O should involve gnttab ops
but no mmu-update. Hence I have a hard time seeing where the huge
difference here would be coming from. Did you have any thoughts here?
> stack_switch 1273311 62381 108589 270764
> multicall 2182803 50 302 524
A fair amount of the mmu-updates is going to be coming through
muticalls, I would guess. Priorities therefore may even differ for
the two separate dispatch points.
> update_va_mapping 571868 10 60 80
> xen_version 73061 850 859 5432
> grant_table_op 0 0 35557 139110
> iret 75673006 484132 268157 757958
The huge differences for builds is puzzling mere here ...
> vcpu_op 453037 71199 138224 334988
> set_segment_base 1650249 62387 108645 270823
> mmuext_op 11225681 188 7239 3426
... and here as well. Did Dom0 and DomU use identical numbers of
vCPU-s and identical -j make option values?
> sched_op 280153 134645 70729 137943
> event_channel_op 192327 66204 71409 214191
> physdev_op 0 0 7721 4315
> (the dom0 values are for the guest running the build or scp test, so
> dom0 acting as backend)
>
> HVM-hypercall PVH-guest build PVH-guest scp
> vcpu_op 277684 2324
> event_channel_op 350233 57383
> (the related dom0 counter values are in the same range as with the test
> running in the PV guest)
>
> It should be noted that during boot of the guests the numbers for the PV
> guest are more like the ones for the build test with the exception of
> iret and sched_op being higher, while for PVH sched_op is by far the
> most often used hypercall.
>
> I'm not sure how to translate those numbers into a good algorithm for
> generating the call sequence.
Well, there's never going to be a clear cut fitting everything, I
suppose.
> I could add priorities to each hypercall in hypercall-defs.c and have a
> cascade of if (likely(foo)) call_foo; else if (likely(bla)) ... else
> switch(rest).
Personally I'd lean to an approach like this one; perhaps there's not
even a need to specify priorities for every hypercall, but just the
ones we deem most frequently used?
Jan
> Or I could have groups of hypercalls with a priority for each group and:
>
> mask = 1ULL << num;
> if (likely(mask & prio_1_mask)) switch(num) ...
> else if (likely(mask & prio_2_mask)) switch (num) ...
> ...
> else switch (num) ...
>
> Or I could combine those approaches using the mask variant for cases of
> multiple entries having the same priority and the direct call variant
> for the cases of only a single entry having a specific priority.
>
> And then there is the problem to set the priorities (fairly simple for
> HVM, PV is more diffcult).
>
>
> Juergen
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 10/12] xen/x86: call hypercall handlers via switch statement
2021-11-02 9:54 ` Jan Beulich
@ 2021-11-02 10:04 ` Juergen Gross
0 siblings, 0 replies; 38+ messages in thread
From: Juergen Gross @ 2021-11-02 10:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 5620 bytes --]
On 02.11.21 10:54, Jan Beulich wrote:
> On 28.10.2021 16:32, Juergen Gross wrote:
>> On 21.10.21 16:41, Jan Beulich wrote:
>>> On 15.10.2021 14:51, Juergen Gross wrote:
>>>> Instead of using a function table use the generated switch statement
>>>> macros for calling the appropriate hypercall handlers.
>>>>
>>>> This is beneficial to performance and avoids speculation issues.
>>>>
>>>> With calling the handlers using the correct number of parameters now
>>>> it is possible to do the parameter register clobbering in the NDEBUG
>>>> case after returning from the handler. This in turn removes the only
>>>> users of hypercall_args_table[] which can be removed now.
>>>
>>> "removed" reads misleading to me: You really replace it by new tables,
>>> using script-generated initializers. Also it looks like you're doubling
>>> the data, as the same sets were previously used by pv64/hvm64 and
>>> pv32/hvm32 respectively.
>>
>> Yes, I'll change that paragraph.
>>
>> Regarding having 4 tables on x86 now: merging the pv/hvm tables would be
>> possible, but this would add some complexity to the script generating
>> the tables (it should test whether the number of parameters of pv and
>> hvm match). As the tables are present in debug build only I don't think
>> this is a real issue.
>
> Sure, but that imo wants saying in the description.
>
>>> Overall, besides these mainly cosmetic aspects the main thing missing
>>> is an approach to prioritize the handful most frequently used functions,
>>> for them to be pulled out of the switch() so we don't depend on the
>>> compiler's choice for the order of comparisons done.
>>
>> I have already prepared that step by generating the complete call
>> sequence, so any change for prioritizing some hypercalls can be local to
>> the generator script and the used input data.
>>
>> The main question is how to do that. I've collected some hypercall
>> statistics data for PV and PVH guests running some simple tests (once a
>> build of the Xen hypervisor, and once a scp of a large file). The data
>> is split between guest and dom0 (PV) counts. There is no clear "winner"
>> which hypercall should be fastest, but several hypercalls are clearly
>> not important.
>>
>> Here is the data:
>>
>> PV-hypercall PV-guest build PV-guest scp dom0 build dom0 scp
>> mmu_update 186175729 2865 20936 33725
>
> Builds should be local to the guest and I/O should involve gnttab ops
> but no mmu-update. Hence I have a hard time seeing where the huge
> difference here would be coming from. Did you have any thoughts here?
I think you misunderstood the columns.
The first column of data is the build job running in domU and the number
of hypercalls done by that domU. The 3rd data column is the same test
(build running in domU), but the number of hypercalls done by dom0 (so
pure backend hypercall activity).
The missing gnttab ops on domU side are fine, as granting a page doesn't
require a hypercall.
>
>> stack_switch 1273311 62381 108589 270764
>> multicall 2182803 50 302 524
>
> A fair amount of the mmu-updates is going to be coming through
> muticalls, I would guess. Priorities therefore may even differ for
> the two separate dispatch points.
I can look into collecting some data here.
>
>> update_va_mapping 571868 10 60 80
>> xen_version 73061 850 859 5432
>> grant_table_op 0 0 35557 139110
>> iret 75673006 484132 268157 757958
>
> The huge differences for builds is puzzling mere here ...
>
>> vcpu_op 453037 71199 138224 334988
>> set_segment_base 1650249 62387 108645 270823
>> mmuext_op 11225681 188 7239 3426
>
> ... and here as well. Did Dom0 and DomU use identical numbers of
> vCPU-s and identical -j make option values?
>
>> sched_op 280153 134645 70729 137943
>> event_channel_op 192327 66204 71409 214191
>> physdev_op 0 0 7721 4315
>> (the dom0 values are for the guest running the build or scp test, so
>> dom0 acting as backend)
>>
>> HVM-hypercall PVH-guest build PVH-guest scp
>> vcpu_op 277684 2324
>> event_channel_op 350233 57383
>> (the related dom0 counter values are in the same range as with the test
>> running in the PV guest)
>>
>> It should be noted that during boot of the guests the numbers for the PV
>> guest are more like the ones for the build test with the exception of
>> iret and sched_op being higher, while for PVH sched_op is by far the
>> most often used hypercall.
>>
>> I'm not sure how to translate those numbers into a good algorithm for
>> generating the call sequence.
>
> Well, there's never going to be a clear cut fitting everything, I
> suppose.
>
>> I could add priorities to each hypercall in hypercall-defs.c and have a
>> cascade of if (likely(foo)) call_foo; else if (likely(bla)) ... else
>> switch(rest).
>
> Personally I'd lean to an approach like this one; perhaps there's not
> even a need to specify priorities for every hypercall, but just the
> ones we deem most frequently used?
See my new series.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2021-11-02 10:04 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-15 12:51 [PATCH 00/12] xen: drop hypercall function tables Juergen Gross
2021-10-15 12:51 ` [PATCH 01/12] xen: limit number of hypercall parameters to 5 Juergen Gross
2021-10-15 12:51 ` [PATCH 02/12] xen: move do_vcpu_op() to arch specific code Juergen Gross
2021-10-15 12:51 ` [PATCH 03/12] xen: harmonize return types of hypercall handlers Juergen Gross
2021-10-18 11:55 ` Jan Beulich
2021-10-18 13:24 ` Juergen Gross
2021-10-18 14:25 ` Jan Beulich
2021-10-18 15:31 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype Juergen Gross
2021-10-18 12:31 ` Jan Beulich
2021-10-18 13:27 ` Juergen Gross
2021-10-18 14:28 ` Jan Beulich
2021-10-18 15:34 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 05/12] xen: don't include asm/hypercall.h from C sources Juergen Gross
2021-10-18 12:39 ` Jan Beulich
2021-10-18 15:20 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 06/12] xen: generate hypercall interface related code Juergen Gross
2021-10-18 12:58 ` Jan Beulich
2021-10-18 15:28 ` Juergen Gross
2021-10-18 15:39 ` Jan Beulich
2021-10-20 7:02 ` Juergen Gross
2021-10-20 7:11 ` Jan Beulich
2021-10-20 7:18 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 07/12] xen: use generated prototypes for hypercall handlers Juergen Gross
2021-10-18 13:01 ` Jan Beulich
2021-10-15 12:51 ` [PATCH 08/12] x86/pv-shim: don't modify hypercall table Juergen Gross
2021-10-15 13:57 ` Jan Beulich
2021-10-15 14:23 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 09/12] xen/x86: don't use hypercall table for calling compat hypercalls Juergen Gross
2021-10-15 12:51 ` [PATCH 10/12] xen/x86: call hypercall handlers via switch statement Juergen Gross
2021-10-21 14:41 ` Jan Beulich
2021-10-28 14:32 ` Juergen Gross
2021-11-02 9:54 ` Jan Beulich
2021-11-02 10:04 ` Juergen Gross
2021-10-15 12:51 ` [PATCH 11/12] xen/arm: " Juergen Gross
2021-10-15 12:51 ` [PATCH 12/12] xen/x86: add hypercall performance counters for hvm, correct pv Juergen Gross
2021-10-21 15:19 ` Jan Beulich
2021-10-28 14:35 ` Juergen Gross
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.