* [PATCH v3 01/13] xen: move do_vcpu_op() to arch specific code
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
@ 2021-12-08 15:55 ` Juergen Gross
2021-12-14 17:21 ` Julien Grall
2021-12-08 15:55 ` [PATCH v3 02/13] xen: harmonize return types of hypercall handlers Juergen Gross
` (13 subsequent siblings)
14 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:55 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Andrew Cooper,
George Dunlap, 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 96e1b23550..691a14277e 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1074,23 +1074,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 093bb4403f..5fadd5c196 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1558,14 +1558,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 )
{
@@ -1737,7 +1734,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] 52+ messages in thread* Re: [PATCH v3 01/13] xen: move do_vcpu_op() to arch specific code
2021-12-08 15:55 ` [PATCH v3 01/13] xen: move do_vcpu_op() to arch specific code Juergen Gross
@ 2021-12-14 17:21 ` Julien Grall
2021-12-15 7:12 ` Juergen Gross
0 siblings, 1 reply; 52+ messages in thread
From: Julien Grall @ 2021-12-14 17:21 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné
Hi Juergen,
On 08/12/2021 15:55, Juergen Gross wrote:
> Today Arm is using another entry point for the vcpu_op hypercall as
NIT: The 'as' doesn't sound right here. Did you mean 'compare to'?
> 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
Typo: s/filetrs/filters/
> 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>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 01/13] xen: move do_vcpu_op() to arch specific code
2021-12-14 17:21 ` Julien Grall
@ 2021-12-15 7:12 ` Juergen Gross
2021-12-15 9:40 ` Julien Grall
0 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-15 7:12 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné
[-- Attachment #1.1.1: Type: text/plain, Size: 1292 bytes --]
On 14.12.21 18:21, Julien Grall wrote:
> Hi Juergen,
>
> On 08/12/2021 15:55, Juergen Gross wrote:
>> Today Arm is using another entry point for the vcpu_op hypercall as
>
> NIT: The 'as' doesn't sound right here. Did you mean 'compare to'?
Hmm, it should even be "different" instead of "another". And then it
should be:
The entry point used for the vcpu_op hypercall on Arm is different
from the one on x86 today.
>
>> 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
>
> Typo: s/filetrs/filters/
Yes.
>
>> 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>
>
> Reviewed-by: Julien Grall <jgrall@amazon.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] 52+ messages in thread
* Re: [PATCH v3 01/13] xen: move do_vcpu_op() to arch specific code
2021-12-15 7:12 ` Juergen Gross
@ 2021-12-15 9:40 ` Julien Grall
0 siblings, 0 replies; 52+ messages in thread
From: Julien Grall @ 2021-12-15 9:40 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné
Hi Juergen,
On 15/12/2021 07:12, Juergen Gross wrote:
> On 14.12.21 18:21, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 08/12/2021 15:55, Juergen Gross wrote:
>>> Today Arm is using another entry point for the vcpu_op hypercall as
>>
>> NIT: The 'as' doesn't sound right here. Did you mean 'compare to'?
>
> Hmm, it should even be "different" instead of "another". And then it
> should be:
>
> The entry point used for the vcpu_op hypercall on Arm is different
> from the one on x86 today.
LGTM.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
2021-12-08 15:55 ` [PATCH v3 01/13] xen: move do_vcpu_op() to arch specific code Juergen Gross
@ 2021-12-08 15:55 ` Juergen Gross
2021-12-14 17:36 ` Julien Grall
2021-12-08 15:55 ` [PATCH v3 03/13] xen: don't include asm/hypercall.h from C sources Juergen Gross
` (12 subsequent siblings)
14 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:55 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Andrew Cooper,
George Dunlap, 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.
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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- rework platform_op compat handling (Jan Beulich)
V3:
- remove include of types.h (Jan Beulich)
---
xen/arch/arm/physdev.c | 2 +-
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/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 | 2 +-
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 +++++++-------
17 files changed, 74 insertions(+), 83 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/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 1f04ffb272..9d3b193bad 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/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 8ba65178e9..e0d74365f2 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..aaaf70eb63 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 implementation defined behavior casting 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..f84252bac6 100644
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -4,10 +4,10 @@
EMIT_FILE;
+#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)
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 308f1115dd..9cc5d383a4 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -234,9 +234,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..9266bc86e9 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -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,27 @@ 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);
+
+typedef struct compat_platform_op compat_platform_op_t;
+DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t);
+int compat_platform_op(XEN_GUEST_HANDLE_PARAM(compat_platform_op_t) 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] 52+ messages in thread* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-08 15:55 ` [PATCH v3 02/13] xen: harmonize return types of hypercall handlers Juergen Gross
@ 2021-12-14 17:36 ` Julien Grall
2021-12-15 7:03 ` Juergen Gross
0 siblings, 1 reply; 52+ messages in thread
From: Julien Grall @ 2021-12-14 17:36 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné,
Christopher Clark
Hi,
On 08/12/2021 15:55, 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.
So on Arm64, I don't think you can make use of the full 64-bit because a
32-bit domain would not be able to see the top 32-bit.
In fact, this could potentially cause us some trouble (see [1]) in Xen.
So it feels like the hypercalls should always return a 32-bit signed
value on Arm.
The other advantage is it would be clear that the top 32-bit are not
usuable. Stefano, what do you think?
>
> 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.
TBH, I think this should have been split because the modifications are
done on lines that are untouched.
The extra patch would have made the review easier (even if this patch is
still quite small).
>
> 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.
>
> 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>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Cheers,
[1] [1]
https://lore.kernel.org/xen-devel/20211206142032.27536-1-michal.orzel@arm.com/
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-14 17:36 ` Julien Grall
@ 2021-12-15 7:03 ` Juergen Gross
2021-12-16 2:10 ` Stefano Stabellini
2021-12-17 10:04 ` Julien Grall
0 siblings, 2 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-15 7:03 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné,
Christopher Clark
[-- Attachment #1.1.1: Type: text/plain, Size: 1869 bytes --]
On 14.12.21 18:36, Julien Grall wrote:
> Hi,
>
> On 08/12/2021 15:55, 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.
>
> So on Arm64, I don't think you can make use of the full 64-bit because a
> 32-bit domain would not be able to see the top 32-bit.
>
> In fact, this could potentially cause us some trouble (see [1]) in Xen.
> So it feels like the hypercalls should always return a 32-bit signed
> value on Arm.
This would break hypercalls like XENMEM_maximum_ram_page which are able
to return larger values, right?
> The other advantage is it would be clear that the top 32-bit are not
> usuable. Stefano, what do you think?
Wouldn't it make more sense to check the return value to be a sign
extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
The question is what to return if this is not the case. -EDOM?
>
>>
>> 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.
>
> TBH, I think this should have been split because the modifications are
> done on lines that are untouched.
>
> The extra patch would have made the review easier (even if this patch is
> still quite small).
I can split the patch if you want.
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] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-15 7:03 ` Juergen Gross
@ 2021-12-16 2:10 ` Stefano Stabellini
2021-12-16 5:13 ` Juergen Gross
2021-12-17 10:04 ` Julien Grall
1 sibling, 1 reply; 52+ messages in thread
From: Stefano Stabellini @ 2021-12-16 2:10 UTC (permalink / raw)
To: Juergen Gross
Cc: Julien Grall, xen-devel, Stefano Stabellini, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné,
Christopher Clark
On Wed, 15 Dec 2021, Juergen Gross wrote:
> On 14.12.21 18:36, Julien Grall wrote:
> > Hi,
> >
> > On 08/12/2021 15:55, 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.
> >
> > So on Arm64, I don't think you can make use of the full 64-bit because a
> > 32-bit domain would not be able to see the top 32-bit.
> >
> > In fact, this could potentially cause us some trouble (see [1]) in Xen.
> > So it feels like the hypercalls should always return a 32-bit signed value
> > on Arm.
>
> This would break hypercalls like XENMEM_maximum_ram_page which are able
> to return larger values, right?
>
> > The other advantage is it would be clear that the top 32-bit are not
> > usuable. Stefano, what do you think?
>
> Wouldn't it make more sense to check the return value to be a sign
> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
>
> The question is what to return if this is not the case. -EDOM?
I can see where Julien is coming from: we have been trying to keep the
arm32 and arm64 ABIs identical since the beginning of the project. So,
like Julien, my preference would be to always return 32-bit on ARM, both
aarch32 and aarch64. It would make things simple.
The case of XENMEM_maximum_ram_page is interesting but it is not a
problem in reality because the max physical address size is only 40-bit
for aarch32 guests, so 32-bit are always enough to return the highest
page in memory for 32-bit guests.
So XENMEM_maximum_ram_page could be the exception and return "long"
which is 4 bytes on aarch32 and 8 bytes on aarch64, and it is exactly
what we need.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-16 2:10 ` Stefano Stabellini
@ 2021-12-16 5:13 ` Juergen Gross
2021-12-16 20:43 ` Stefano Stabellini
0 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-16 5:13 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Julien Grall, xen-devel, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné,
Christopher Clark
[-- Attachment #1.1.1: Type: text/plain, Size: 1825 bytes --]
On 16.12.21 03:10, Stefano Stabellini wrote:
> On Wed, 15 Dec 2021, Juergen Gross wrote:
>> On 14.12.21 18:36, Julien Grall wrote:
>>> Hi,
>>>
>>> On 08/12/2021 15:55, 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.
>>>
>>> So on Arm64, I don't think you can make use of the full 64-bit because a
>>> 32-bit domain would not be able to see the top 32-bit.
>>>
>>> In fact, this could potentially cause us some trouble (see [1]) in Xen.
>>> So it feels like the hypercalls should always return a 32-bit signed value
>>> on Arm.
>>
>> This would break hypercalls like XENMEM_maximum_ram_page which are able
>> to return larger values, right?
>>
>>> The other advantage is it would be clear that the top 32-bit are not
>>> usuable. Stefano, what do you think?
>>
>> Wouldn't it make more sense to check the return value to be a sign
>> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
>>
>> The question is what to return if this is not the case. -EDOM?
>
>
> I can see where Julien is coming from: we have been trying to keep the
> arm32 and arm64 ABIs identical since the beginning of the project. So,
> like Julien, my preference would be to always return 32-bit on ARM, both
> aarch32 and aarch64. It would make things simple.
>
> The case of XENMEM_maximum_ram_page is interesting but it is not a
> problem in reality because the max physical address size is only 40-bit
> for aarch32 guests, so 32-bit are always enough to return the highest
> page in memory for 32-bit guests.
You are aware that this isn't the guest's max page, but the host's?
And all of this is about a 32-bit guest on a 64-bit host.
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] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-16 5:13 ` Juergen Gross
@ 2021-12-16 20:43 ` Stefano Stabellini
2021-12-16 21:15 ` Stefano Stabellini
0 siblings, 1 reply; 52+ messages in thread
From: Stefano Stabellini @ 2021-12-16 20:43 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, xen-devel, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné,
Christopher Clark
On Thu, 16 Dec 2021, Juergen Gross wrote:
> On 16.12.21 03:10, Stefano Stabellini wrote:
> > On Wed, 15 Dec 2021, Juergen Gross wrote:
> > > On 14.12.21 18:36, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 08/12/2021 15:55, 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.
> > > >
> > > > So on Arm64, I don't think you can make use of the full 64-bit because a
> > > > 32-bit domain would not be able to see the top 32-bit.
> > > >
> > > > In fact, this could potentially cause us some trouble (see [1]) in Xen.
> > > > So it feels like the hypercalls should always return a 32-bit signed
> > > > value
> > > > on Arm.
> > >
> > > This would break hypercalls like XENMEM_maximum_ram_page which are able
> > > to return larger values, right?
> > >
> > > > The other advantage is it would be clear that the top 32-bit are not
> > > > usuable. Stefano, what do you think?
> > >
> > > Wouldn't it make more sense to check the return value to be a sign
> > > extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
> > >
> > > The question is what to return if this is not the case. -EDOM?
> >
> >
> > I can see where Julien is coming from: we have been trying to keep the
> > arm32 and arm64 ABIs identical since the beginning of the project. So,
> > like Julien, my preference would be to always return 32-bit on ARM, both
> > aarch32 and aarch64. It would make things simple.
> >
> > The case of XENMEM_maximum_ram_page is interesting but it is not a
> > problem in reality because the max physical address size is only 40-bit
> > for aarch32 guests, so 32-bit are always enough to return the highest
> > page in memory for 32-bit guests.
>
> You are aware that this isn't the guest's max page, but the host's?
>
> And all of this is about a 32-bit guest on a 64-bit host.
Yes, I am following, and it is always difficult to get the right
information out of the ARM Reference Manual, but from my search
ARMv8/aarch32 (which is 32-bit mode on 64-bit capable hardware) supports
max 40 bits.
From G5.1.3 (ARM DDI 0487G.a):
---
For execution in AArch32 state, the Armv8 architecture supports:
- A VA space of up to 32 bits. The actual width is IMPLEMENTATION DEFINED .
- An IPA space of up to 40 bits. The translation tables and associated System registers define the width of the
implemented address space.
---
Julien, Bertrand, am I reading this right?
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-16 20:43 ` Stefano Stabellini
@ 2021-12-16 21:15 ` Stefano Stabellini
2021-12-17 5:34 ` Juergen Gross
` (2 more replies)
0 siblings, 3 replies; 52+ messages in thread
From: Stefano Stabellini @ 2021-12-16 21:15 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Juergen Gross, Julien Grall, xen-devel, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné,
Christopher Clark
On Thu, 16 Dec 2021, Stefano Stabellini wrote:
> On Thu, 16 Dec 2021, Juergen Gross wrote:
> > On 16.12.21 03:10, Stefano Stabellini wrote:
> > > On Wed, 15 Dec 2021, Juergen Gross wrote:
> > > > On 14.12.21 18:36, Julien Grall wrote:
> > > > > Hi,
> > > > >
> > > > > On 08/12/2021 15:55, 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.
> > > > >
> > > > > So on Arm64, I don't think you can make use of the full 64-bit because a
> > > > > 32-bit domain would not be able to see the top 32-bit.
> > > > >
> > > > > In fact, this could potentially cause us some trouble (see [1]) in Xen.
> > > > > So it feels like the hypercalls should always return a 32-bit signed
> > > > > value
> > > > > on Arm.
> > > >
> > > > This would break hypercalls like XENMEM_maximum_ram_page which are able
> > > > to return larger values, right?
> > > >
> > > > > The other advantage is it would be clear that the top 32-bit are not
> > > > > usuable. Stefano, what do you think?
> > > >
> > > > Wouldn't it make more sense to check the return value to be a sign
> > > > extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
> > > >
> > > > The question is what to return if this is not the case. -EDOM?
> > >
> > >
> > > I can see where Julien is coming from: we have been trying to keep the
> > > arm32 and arm64 ABIs identical since the beginning of the project. So,
> > > like Julien, my preference would be to always return 32-bit on ARM, both
> > > aarch32 and aarch64. It would make things simple.
> > >
> > > The case of XENMEM_maximum_ram_page is interesting but it is not a
> > > problem in reality because the max physical address size is only 40-bit
> > > for aarch32 guests, so 32-bit are always enough to return the highest
> > > page in memory for 32-bit guests.
> >
> > You are aware that this isn't the guest's max page, but the host's?
I can see now that you meant to say that, no matter what is the max
pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is
supposed to return the max memory page, which could go above the
addressibility limit of the VM.
So XENMEM_maximum_ram_page should potentially be able to return (1<<44)
even when called by an aarch32 VM, with max IPA 40-bit.
I would imagine it could be useful if dom0 is 32-bit but domUs are
64-bit on a 64-bit hypervisor (which I think it would be a very rare
configuration on ARM.)
Then it looks like XENMEM_maximum_ram_page needs to be able to return a
value > 32-bit when called by a 32-bit guest.
The hypercall ABI follows the ARM C calling convention, so a 64-bit
value should be returned using r0 and r1. But looking at
xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1
today. Only r0 is set, so effectively we only support 32-bit return
values on aarch32 and for aarch32 guests.
In other words, today all hypercalls on ARM return 64-bit to 64-bit
guests and 32-bit to 32-bit guests. Which in the case of memory_op is
"technically" the correct thing to do because it matches the C
declaration in xen/include/xen/hypercall.h:
extern long
do_memory_op(
unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg);
So... I guess the conclusion is that on ARM do_memory_op should return
"long" although it is not actually enough for a correct implementation
of XENMEM_maximum_ram_page for aarch32 guests ?
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-16 21:15 ` Stefano Stabellini
@ 2021-12-17 5:34 ` Juergen Gross
2021-12-17 7:45 ` Jan Beulich
2021-12-17 7:39 ` Jan Beulich
2021-12-17 10:38 ` Julien Grall
2 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-17 5:34 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Julien Grall, xen-devel, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné,
Christopher Clark
[-- Attachment #1.1.1: Type: text/plain, Size: 3834 bytes --]
On 16.12.21 22:15, Stefano Stabellini wrote:
> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>> On Wed, 15 Dec 2021, Juergen Gross wrote:
>>>>> On 14.12.21 18:36, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 08/12/2021 15:55, 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.
>>>>>>
>>>>>> So on Arm64, I don't think you can make use of the full 64-bit because a
>>>>>> 32-bit domain would not be able to see the top 32-bit.
>>>>>>
>>>>>> In fact, this could potentially cause us some trouble (see [1]) in Xen.
>>>>>> So it feels like the hypercalls should always return a 32-bit signed
>>>>>> value
>>>>>> on Arm.
>>>>>
>>>>> This would break hypercalls like XENMEM_maximum_ram_page which are able
>>>>> to return larger values, right?
>>>>>
>>>>>> The other advantage is it would be clear that the top 32-bit are not
>>>>>> usuable. Stefano, what do you think?
>>>>>
>>>>> Wouldn't it make more sense to check the return value to be a sign
>>>>> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
>>>>>
>>>>> The question is what to return if this is not the case. -EDOM?
>>>>
>>>>
>>>> I can see where Julien is coming from: we have been trying to keep the
>>>> arm32 and arm64 ABIs identical since the beginning of the project. So,
>>>> like Julien, my preference would be to always return 32-bit on ARM, both
>>>> aarch32 and aarch64. It would make things simple.
>>>>
>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a
>>>> problem in reality because the max physical address size is only 40-bit
>>>> for aarch32 guests, so 32-bit are always enough to return the highest
>>>> page in memory for 32-bit guests.
>>>
>>> You are aware that this isn't the guest's max page, but the host's?
>
> I can see now that you meant to say that, no matter what is the max
> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is
> supposed to return the max memory page, which could go above the
> addressibility limit of the VM.
>
> So XENMEM_maximum_ram_page should potentially be able to return (1<<44)
> even when called by an aarch32 VM, with max IPA 40-bit.
>
> I would imagine it could be useful if dom0 is 32-bit but domUs are
> 64-bit on a 64-bit hypervisor (which I think it would be a very rare
> configuration on ARM.)
>
> Then it looks like XENMEM_maximum_ram_page needs to be able to return a
> value > 32-bit when called by a 32-bit guest.
>
> The hypercall ABI follows the ARM C calling convention, so a 64-bit
> value should be returned using r0 and r1. But looking at
> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1
> today. Only r0 is set, so effectively we only support 32-bit return
> values on aarch32 and for aarch32 guests.
>
> In other words, today all hypercalls on ARM return 64-bit to 64-bit
> guests and 32-bit to 32-bit guests. Which in the case of memory_op is
> "technically" the correct thing to do because it matches the C
> declaration in xen/include/xen/hypercall.h:
>
> extern long
> do_memory_op(
> unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg);
>
> So... I guess the conclusion is that on ARM do_memory_op should return
> "long" although it is not actually enough for a correct implementation
> of XENMEM_maximum_ram_page for aarch32 guests ?
>
Hence my suggestion to check the return value of _all_ hypercalls to be
proper sign extended int values for 32-bit guests. This would fix all
potential issues without silently returning truncated values.
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] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-17 5:34 ` Juergen Gross
@ 2021-12-17 7:45 ` Jan Beulich
2021-12-17 8:50 ` Juergen Gross
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-12-17 7:45 UTC (permalink / raw)
To: Juergen Gross
Cc: Julien Grall, xen-devel, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Christopher Clark, Stefano Stabellini
On 17.12.2021 06:34, Juergen Gross wrote:
> On 16.12.21 22:15, Stefano Stabellini wrote:
>> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a
>>>>> problem in reality because the max physical address size is only 40-bit
>>>>> for aarch32 guests, so 32-bit are always enough to return the highest
>>>>> page in memory for 32-bit guests.
>>>>
>>>> You are aware that this isn't the guest's max page, but the host's?
>>
>> I can see now that you meant to say that, no matter what is the max
>> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is
>> supposed to return the max memory page, which could go above the
>> addressibility limit of the VM.
>>
>> So XENMEM_maximum_ram_page should potentially be able to return (1<<44)
>> even when called by an aarch32 VM, with max IPA 40-bit.
>>
>> I would imagine it could be useful if dom0 is 32-bit but domUs are
>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare
>> configuration on ARM.)
>>
>> Then it looks like XENMEM_maximum_ram_page needs to be able to return a
>> value > 32-bit when called by a 32-bit guest.
>>
>> The hypercall ABI follows the ARM C calling convention, so a 64-bit
>> value should be returned using r0 and r1. But looking at
>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1
>> today. Only r0 is set, so effectively we only support 32-bit return
>> values on aarch32 and for aarch32 guests.
>>
>> In other words, today all hypercalls on ARM return 64-bit to 64-bit
>> guests and 32-bit to 32-bit guests. Which in the case of memory_op is
>> "technically" the correct thing to do because it matches the C
>> declaration in xen/include/xen/hypercall.h:
>>
>> extern long
>> do_memory_op(
>> unsigned long cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg);
>>
>> So... I guess the conclusion is that on ARM do_memory_op should return
>> "long" although it is not actually enough for a correct implementation
>> of XENMEM_maximum_ram_page for aarch32 guests ?
>>
>
> Hence my suggestion to check the return value of _all_ hypercalls to be
> proper sign extended int values for 32-bit guests. This would fix all
> potential issues without silently returning truncated values.
Are we absolutely certain we have no other paths left where a possibly
large unsigned values might be returned? In fact while
compat_memory_op() does the necessary saturation, I've never been fully
convinced of this being the best way of dealing with things. The range
of error indicators is much smaller than [-INT_MIN,-1], so almost
double the range of effectively unsigned values could be passed back
fine. (Obviously we can't change existing interfaces, so this mem-op
will need to remain as is.)
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-17 7:45 ` Jan Beulich
@ 2021-12-17 8:50 ` Juergen Gross
2021-12-17 10:41 ` Julien Grall
0 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-17 8:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, xen-devel, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Christopher Clark, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 4250 bytes --]
On 17.12.21 08:45, Jan Beulich wrote:
> On 17.12.2021 06:34, Juergen Gross wrote:
>> On 16.12.21 22:15, Stefano Stabellini wrote:
>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>>>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a
>>>>>> problem in reality because the max physical address size is only 40-bit
>>>>>> for aarch32 guests, so 32-bit are always enough to return the highest
>>>>>> page in memory for 32-bit guests.
>>>>>
>>>>> You are aware that this isn't the guest's max page, but the host's?
>>>
>>> I can see now that you meant to say that, no matter what is the max
>>> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is
>>> supposed to return the max memory page, which could go above the
>>> addressibility limit of the VM.
>>>
>>> So XENMEM_maximum_ram_page should potentially be able to return (1<<44)
>>> even when called by an aarch32 VM, with max IPA 40-bit.
>>>
>>> I would imagine it could be useful if dom0 is 32-bit but domUs are
>>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare
>>> configuration on ARM.)
>>>
>>> Then it looks like XENMEM_maximum_ram_page needs to be able to return a
>>> value > 32-bit when called by a 32-bit guest.
>>>
>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit
>>> value should be returned using r0 and r1. But looking at
>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1
>>> today. Only r0 is set, so effectively we only support 32-bit return
>>> values on aarch32 and for aarch32 guests.
>>>
>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit
>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op is
>>> "technically" the correct thing to do because it matches the C
>>> declaration in xen/include/xen/hypercall.h:
>>>
>>> extern long
>>> do_memory_op(
>>> unsigned long cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg);
>>>
>>> So... I guess the conclusion is that on ARM do_memory_op should return
>>> "long" although it is not actually enough for a correct implementation
>>> of XENMEM_maximum_ram_page for aarch32 guests ?
>>>
>>
>> Hence my suggestion to check the return value of _all_ hypercalls to be
>> proper sign extended int values for 32-bit guests. This would fix all
>> potential issues without silently returning truncated values.
>
> Are we absolutely certain we have no other paths left where a possibly
> large unsigned values might be returned? In fact while
> compat_memory_op() does the necessary saturation, I've never been fully
> convinced of this being the best way of dealing with things. The range
> of error indicators is much smaller than [-INT_MIN,-1], so almost
> double the range of effectively unsigned values could be passed back
> fine. (Obviously we can't change existing interfaces, so this mem-op
> will need to remain as is.)
In fact libxenctrl tries do deal with this fact by wrapping a memory_op
for a 32-bit environment into a multicall. This will work fine for a
32-bit Arm guest, as xen_ulong_t is a uint64 there.
So do_memory_op should return long on Arm, yes. OTOH doing so will
continue to be a problem in case a 32-bit guest doesn't use the
multicall technique for handling possible 64-bit return values.
So I continue to argue that on Arm the return value of a hypercall
should be tested to fit into 32 bits. The only really clean alternative
would be to have separate hypercall function classes for Arm 32- and
64-bit guests (which still could share most of the functions by letting
those return "int"). This would allow to use the 64-bit variant even for
32-bit guests in multicall (fine as the return field is 64-bit wide),
and a probably saturating compat version for the 32-bit guest direct
hypercall.
The needed adaptions in my series would be rather limited (an additional
column in the hypercall table, a split which macro to use in
do_trap_hypercall() on Arm depending on the bitness of the guest, the
addition of the Arm compat variant of do_memory_op()).
Thoughts?
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] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-17 8:50 ` Juergen Gross
@ 2021-12-17 10:41 ` Julien Grall
2021-12-17 11:12 ` Juergen Gross
0 siblings, 1 reply; 52+ messages in thread
From: Julien Grall @ 2021-12-17 10:41 UTC (permalink / raw)
To: Juergen Gross, Jan Beulich
Cc: xen-devel, Volodymyr Babchuk, Bertrand Marquis, Andrew Cooper,
George Dunlap, Wei Liu, Roger Pau Monné,
Christopher Clark, Stefano Stabellini
Hi Juergen,
On 17/12/2021 08:50, Juergen Gross wrote:
> On 17.12.21 08:45, Jan Beulich wrote:
>> On 17.12.2021 06:34, Juergen Gross wrote:
>>> On 16.12.21 22:15, Stefano Stabellini wrote:
>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a
>>>>>>> problem in reality because the max physical address size is only
>>>>>>> 40-bit
>>>>>>> for aarch32 guests, so 32-bit are always enough to return the
>>>>>>> highest
>>>>>>> page in memory for 32-bit guests.
>>>>>>
>>>>>> You are aware that this isn't the guest's max page, but the host's?
>>>>
>>>> I can see now that you meant to say that, no matter what is the max
>>>> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is
>>>> supposed to return the max memory page, which could go above the
>>>> addressibility limit of the VM.
>>>>
>>>> So XENMEM_maximum_ram_page should potentially be able to return (1<<44)
>>>> even when called by an aarch32 VM, with max IPA 40-bit.
>>>>
>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are
>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare
>>>> configuration on ARM.)
>>>>
>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to return a
>>>> value > 32-bit when called by a 32-bit guest.
>>>>
>>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit
>>>> value should be returned using r0 and r1. But looking at
>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1
>>>> today. Only r0 is set, so effectively we only support 32-bit return
>>>> values on aarch32 and for aarch32 guests.
>>>>
>>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit
>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op is
>>>> "technically" the correct thing to do because it matches the C
>>>> declaration in xen/include/xen/hypercall.h:
>>>>
>>>> extern long
>>>> do_memory_op(
>>>> unsigned long cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg);
>>>>
>>>> So... I guess the conclusion is that on ARM do_memory_op should return
>>>> "long" although it is not actually enough for a correct implementation
>>>> of XENMEM_maximum_ram_page for aarch32 guests ?
>>>>
>>>
>>> Hence my suggestion to check the return value of _all_ hypercalls to be
>>> proper sign extended int values for 32-bit guests. This would fix all
>>> potential issues without silently returning truncated values.
>>
>> Are we absolutely certain we have no other paths left where a possibly
>> large unsigned values might be returned? In fact while
>> compat_memory_op() does the necessary saturation, I've never been fully
>> convinced of this being the best way of dealing with things. The range
>> of error indicators is much smaller than [-INT_MIN,-1], so almost
>> double the range of effectively unsigned values could be passed back
>> fine. (Obviously we can't change existing interfaces, so this mem-op
>> will need to remain as is.)
>
> In fact libxenctrl tries do deal with this fact by wrapping a memory_op
> for a 32-bit environment into a multicall. This will work fine for a
> 32-bit Arm guest, as xen_ulong_t is a uint64 there.
>
> So do_memory_op should return long on Arm, yes. OTOH doing so will
> continue to be a problem in case a 32-bit guest doesn't use the
> multicall technique for handling possible 64-bit return values.
>
> So I continue to argue that on Arm the return value of a hypercall
> should be tested to fit into 32 bits.
It would make sense. But what would you return if the value doesn't fit?
> The only really clean alternative
> would be to have separate hypercall function classes for Arm 32- and
> 64-bit guests (which still could share most of the functions by letting
> those return "int"). This would allow to use the 64-bit variant even for
> 32-bit guests in multicall (fine as the return field is 64-bit wide),
> and a probably saturating compat version for the 32-bit guest direct
> hypercall.
I am not entirely sure to understand this proposal. Can you clarify it?
>
> The needed adaptions in my series would be rather limited (an additional
> column in the hypercall table, a split which macro to use in
> do_trap_hypercall() on Arm depending on the bitness of the guest, the
> addition of the Arm compat variant of do_memory_op()).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-17 10:41 ` Julien Grall
@ 2021-12-17 11:12 ` Juergen Gross
2021-12-17 23:00 ` Stefano Stabellini
0 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-17 11:12 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: xen-devel, Volodymyr Babchuk, Bertrand Marquis, Andrew Cooper,
George Dunlap, Wei Liu, Roger Pau Monné,
Christopher Clark, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 5473 bytes --]
On 17.12.21 11:41, Julien Grall wrote:
> Hi Juergen,
>
> On 17/12/2021 08:50, Juergen Gross wrote:
>> On 17.12.21 08:45, Jan Beulich wrote:
>>> On 17.12.2021 06:34, Juergen Gross wrote:
>>>> On 16.12.21 22:15, Stefano Stabellini wrote:
>>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a
>>>>>>>> problem in reality because the max physical address size is only
>>>>>>>> 40-bit
>>>>>>>> for aarch32 guests, so 32-bit are always enough to return the
>>>>>>>> highest
>>>>>>>> page in memory for 32-bit guests.
>>>>>>>
>>>>>>> You are aware that this isn't the guest's max page, but the host's?
>>>>>
>>>>> I can see now that you meant to say that, no matter what is the max
>>>>> pseudo-physical address supported by the VM,
>>>>> XENMEM_maximum_ram_page is
>>>>> supposed to return the max memory page, which could go above the
>>>>> addressibility limit of the VM.
>>>>>
>>>>> So XENMEM_maximum_ram_page should potentially be able to return
>>>>> (1<<44)
>>>>> even when called by an aarch32 VM, with max IPA 40-bit.
>>>>>
>>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are
>>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare
>>>>> configuration on ARM.)
>>>>>
>>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to
>>>>> return a
>>>>> value > 32-bit when called by a 32-bit guest.
>>>>>
>>>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit
>>>>> value should be returned using r0 and r1. But looking at
>>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever
>>>>> sets r1
>>>>> today. Only r0 is set, so effectively we only support 32-bit return
>>>>> values on aarch32 and for aarch32 guests.
>>>>>
>>>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit
>>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op is
>>>>> "technically" the correct thing to do because it matches the C
>>>>> declaration in xen/include/xen/hypercall.h:
>>>>>
>>>>> extern long
>>>>> do_memory_op(
>>>>> unsigned long cmd,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg);
>>>>>
>>>>> So... I guess the conclusion is that on ARM do_memory_op should
>>>>> return
>>>>> "long" although it is not actually enough for a correct implementation
>>>>> of XENMEM_maximum_ram_page for aarch32 guests ?
>>>>>
>>>>
>>>> Hence my suggestion to check the return value of _all_ hypercalls to be
>>>> proper sign extended int values for 32-bit guests. This would fix all
>>>> potential issues without silently returning truncated values.
>>>
>>> Are we absolutely certain we have no other paths left where a possibly
>>> large unsigned values might be returned? In fact while
>>> compat_memory_op() does the necessary saturation, I've never been fully
>>> convinced of this being the best way of dealing with things. The range
>>> of error indicators is much smaller than [-INT_MIN,-1], so almost
>>> double the range of effectively unsigned values could be passed back
>>> fine. (Obviously we can't change existing interfaces, so this mem-op
>>> will need to remain as is.)
>>
>> In fact libxenctrl tries do deal with this fact by wrapping a memory_op
>> for a 32-bit environment into a multicall. This will work fine for a
>> 32-bit Arm guest, as xen_ulong_t is a uint64 there.
>>
>> So do_memory_op should return long on Arm, yes. OTOH doing so will
>> continue to be a problem in case a 32-bit guest doesn't use the
>> multicall technique for handling possible 64-bit return values.
>>
>> So I continue to argue that on Arm the return value of a hypercall
>> should be tested to fit into 32 bits.
>
> It would make sense. But what would you return if the value doesn't fit?
I guess some errno value would be appropriate, like -EDOM, -ERANGE or
-E2BIG.
>
>> The only really clean alternative
>> would be to have separate hypercall function classes for Arm 32- and
>> 64-bit guests (which still could share most of the functions by letting
>> those return "int"). This would allow to use the 64-bit variant even for
>> 32-bit guests in multicall (fine as the return field is 64-bit wide),
>> and a probably saturating compat version for the 32-bit guest direct
>> hypercall.
>
> I am not entirely sure to understand this proposal. Can you clarify it?
1. In patch 5 modify the hypercall table by adding another column, so
instead of:
+table: pv32 pv64 hvm32 hvm64 arm
use:
+table: pv32 pv64 hvm32 hvm64 arm32 arm64
2. Let most of the hypercalls just return int instead of long:
+rettype: do int
3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the
compat variant existing already):
+rettype: do64 long
+prefix: do64 PREFIX_hvm
+memory_op(unsigned long cmd, void *arg)
4. Use the appropriate calls in each column:
+memory_op compat do64 hvm hvm compat do64
5. In the Arm hypercall trap handler do:
if ( is_32bit_domain(current->domain) )
call_handlers_arm32(...);
else
call_handlers_arm64(...);
6. In the multicall handler always do:
call_handlers_arm64(...);
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] 52+ messages in thread* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-17 11:12 ` Juergen Gross
@ 2021-12-17 23:00 ` Stefano Stabellini
2021-12-20 17:14 ` Julien Grall
0 siblings, 1 reply; 52+ messages in thread
From: Stefano Stabellini @ 2021-12-17 23:00 UTC (permalink / raw)
To: Juergen Gross
Cc: Julien Grall, Jan Beulich, xen-devel, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 6046 bytes --]
On Fri, 17 Dec 2021, Juergen Gross wrote:
> On 17.12.21 11:41, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 17/12/2021 08:50, Juergen Gross wrote:
> > > On 17.12.21 08:45, Jan Beulich wrote:
> > > > On 17.12.2021 06:34, Juergen Gross wrote:
> > > > > On 16.12.21 22:15, Stefano Stabellini wrote:
> > > > > > On Thu, 16 Dec 2021, Stefano Stabellini wrote:
> > > > > > > On Thu, 16 Dec 2021, Juergen Gross wrote:
> > > > > > > > On 16.12.21 03:10, Stefano Stabellini wrote:
> > > > > > > > > The case of XENMEM_maximum_ram_page is interesting but it is
> > > > > > > > > not a
> > > > > > > > > problem in reality because the max physical address size is
> > > > > > > > > only 40-bit
> > > > > > > > > for aarch32 guests, so 32-bit are always enough to return the
> > > > > > > > > highest
> > > > > > > > > page in memory for 32-bit guests.
> > > > > > > >
> > > > > > > > You are aware that this isn't the guest's max page, but the
> > > > > > > > host's?
> > > > > >
> > > > > > I can see now that you meant to say that, no matter what is the max
> > > > > > pseudo-physical address supported by the VM, XENMEM_maximum_ram_page
> > > > > > is
> > > > > > supposed to return the max memory page, which could go above the
> > > > > > addressibility limit of the VM.
> > > > > >
> > > > > > So XENMEM_maximum_ram_page should potentially be able to return
> > > > > > (1<<44)
> > > > > > even when called by an aarch32 VM, with max IPA 40-bit.
> > > > > >
> > > > > > I would imagine it could be useful if dom0 is 32-bit but domUs are
> > > > > > 64-bit on a 64-bit hypervisor (which I think it would be a very rare
> > > > > > configuration on ARM.)
> > > > > >
> > > > > > Then it looks like XENMEM_maximum_ram_page needs to be able to
> > > > > > return a
> > > > > > value > 32-bit when called by a 32-bit guest.
> > > > > >
> > > > > > The hypercall ABI follows the ARM C calling convention, so a 64-bit
> > > > > > value should be returned using r0 and r1. But looking at
> > > > > > xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets
> > > > > > r1
> > > > > > today. Only r0 is set, so effectively we only support 32-bit return
> > > > > > values on aarch32 and for aarch32 guests.
> > > > > >
> > > > > > In other words, today all hypercalls on ARM return 64-bit to 64-bit
> > > > > > guests and 32-bit to 32-bit guests. Which in the case of memory_op
> > > > > > is
> > > > > > "technically" the correct thing to do because it matches the C
> > > > > > declaration in xen/include/xen/hypercall.h:
> > > > > >
> > > > > > extern long
> > > > > > do_memory_op(
> > > > > > unsigned long cmd,
> > > > > > XEN_GUEST_HANDLE_PARAM(void) arg);
> > > > > >
> > > > > > So... I guess the conclusion is that on ARM do_memory_op should
> > > > > > return
> > > > > > "long" although it is not actually enough for a correct
> > > > > > implementation
> > > > > > of XENMEM_maximum_ram_page for aarch32 guests ?
> > > > > >
> > > > >
> > > > > Hence my suggestion to check the return value of _all_ hypercalls to
> > > > > be
> > > > > proper sign extended int values for 32-bit guests. This would fix all
> > > > > potential issues without silently returning truncated values.
> > > >
> > > > Are we absolutely certain we have no other paths left where a possibly
> > > > large unsigned values might be returned? In fact while
> > > > compat_memory_op() does the necessary saturation, I've never been fully
> > > > convinced of this being the best way of dealing with things. The range
> > > > of error indicators is much smaller than [-INT_MIN,-1], so almost
> > > > double the range of effectively unsigned values could be passed back
> > > > fine. (Obviously we can't change existing interfaces, so this mem-op
> > > > will need to remain as is.)
> > >
> > > In fact libxenctrl tries do deal with this fact by wrapping a memory_op
> > > for a 32-bit environment into a multicall. This will work fine for a
> > > 32-bit Arm guest, as xen_ulong_t is a uint64 there.
> > >
> > > So do_memory_op should return long on Arm, yes. OTOH doing so will
> > > continue to be a problem in case a 32-bit guest doesn't use the
> > > multicall technique for handling possible 64-bit return values.
> > >
> > > So I continue to argue that on Arm the return value of a hypercall
> > > should be tested to fit into 32 bits.
> >
> > It would make sense. But what would you return if the value doesn't fit?
>
> I guess some errno value would be appropriate, like -EDOM, -ERANGE or
> -E2BIG.
This seems to be better than the alternative below as it is a lot
simpler.
> > > The only really clean alternative
> > > would be to have separate hypercall function classes for Arm 32- and
> > > 64-bit guests (which still could share most of the functions by letting
> > > those return "int"). This would allow to use the 64-bit variant even for
> > > 32-bit guests in multicall (fine as the return field is 64-bit wide),
> > > and a probably saturating compat version for the 32-bit guest direct
> > > hypercall.
> >
> > I am not entirely sure to understand this proposal. Can you clarify it?
>
> 1. In patch 5 modify the hypercall table by adding another column, so
> instead of:
> +table: pv32 pv64 hvm32 hvm64 arm
> use:
> +table: pv32 pv64 hvm32 hvm64 arm32 arm64
>
> 2. Let most of the hypercalls just return int instead of long:
> +rettype: do int
>
> 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the
> compat variant existing already):
> +rettype: do64 long
> +prefix: do64 PREFIX_hvm
> +memory_op(unsigned long cmd, void *arg)
>
> 4. Use the appropriate calls in each column:
> +memory_op compat do64 hvm hvm compat do64
>
> 5. In the Arm hypercall trap handler do:
> if ( is_32bit_domain(current->domain) )
> call_handlers_arm32(...);
> else
> call_handlers_arm64(...);
>
> 6. In the multicall handler always do:
> call_handlers_arm64(...);
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-17 23:00 ` Stefano Stabellini
@ 2021-12-20 17:14 ` Julien Grall
2021-12-21 0:08 ` Stefano Stabellini
2021-12-21 7:45 ` Juergen Gross
0 siblings, 2 replies; 52+ messages in thread
From: Julien Grall @ 2021-12-20 17:14 UTC (permalink / raw)
To: Stefano Stabellini, Juergen Gross
Cc: Jan Beulich, xen-devel, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Christopher Clark
Hi,
On 18/12/2021 00:00, Stefano Stabellini wrote:
> On Fri, 17 Dec 2021, Juergen Gross wrote:
>> On 17.12.21 11:41, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 17/12/2021 08:50, Juergen Gross wrote:
>>>> On 17.12.21 08:45, Jan Beulich wrote:
>>>>> On 17.12.2021 06:34, Juergen Gross wrote:
>>>>>> On 16.12.21 22:15, Stefano Stabellini wrote:
>>>>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>>>>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>>>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is
>>>>>>>>>> not a
>>>>>>>>>> problem in reality because the max physical address size is
>>>>>>>>>> only 40-bit
>>>>>>>>>> for aarch32 guests, so 32-bit are always enough to return the
>>>>>>>>>> highest
>>>>>>>>>> page in memory for 32-bit guests.
>>>>>>>>>
>>>>>>>>> You are aware that this isn't the guest's max page, but the
>>>>>>>>> host's?
>>>>>>>
>>>>>>> I can see now that you meant to say that, no matter what is the max
>>>>>>> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page
>>>>>>> is
>>>>>>> supposed to return the max memory page, which could go above the
>>>>>>> addressibility limit of the VM.
>>>>>>>
>>>>>>> So XENMEM_maximum_ram_page should potentially be able to return
>>>>>>> (1<<44)
>>>>>>> even when called by an aarch32 VM, with max IPA 40-bit.
>>>>>>>
>>>>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are
>>>>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare
>>>>>>> configuration on ARM.)
>>>>>>>
>>>>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to
>>>>>>> return a
>>>>>>> value > 32-bit when called by a 32-bit guest.
>>>>>>>
>>>>>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit
>>>>>>> value should be returned using r0 and r1. But looking at
>>>>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets
>>>>>>> r1
>>>>>>> today. Only r0 is set, so effectively we only support 32-bit return
>>>>>>> values on aarch32 and for aarch32 guests.
>>>>>>>
>>>>>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit
>>>>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op
>>>>>>> is
>>>>>>> "technically" the correct thing to do because it matches the C
>>>>>>> declaration in xen/include/xen/hypercall.h:
>>>>>>>
>>>>>>> extern long
>>>>>>> do_memory_op(
>>>>>>> unsigned long cmd,
>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg);
>>>>>>>
>>>>>>> So... I guess the conclusion is that on ARM do_memory_op should
>>>>>>> return
>>>>>>> "long" although it is not actually enough for a correct
>>>>>>> implementation
>>>>>>> of XENMEM_maximum_ram_page for aarch32 guests ?
>>>>>>>
>>>>>>
>>>>>> Hence my suggestion to check the return value of _all_ hypercalls to
>>>>>> be
>>>>>> proper sign extended int values for 32-bit guests. This would fix all
>>>>>> potential issues without silently returning truncated values.
>>>>>
>>>>> Are we absolutely certain we have no other paths left where a possibly
>>>>> large unsigned values might be returned? In fact while
>>>>> compat_memory_op() does the necessary saturation, I've never been fully
>>>>> convinced of this being the best way of dealing with things. The range
>>>>> of error indicators is much smaller than [-INT_MIN,-1], so almost
>>>>> double the range of effectively unsigned values could be passed back
>>>>> fine. (Obviously we can't change existing interfaces, so this mem-op
>>>>> will need to remain as is.)
>>>>
>>>> In fact libxenctrl tries do deal with this fact by wrapping a memory_op
>>>> for a 32-bit environment into a multicall. This will work fine for a
>>>> 32-bit Arm guest, as xen_ulong_t is a uint64 there.
>>>>
>>>> So do_memory_op should return long on Arm, yes. OTOH doing so will
>>>> continue to be a problem in case a 32-bit guest doesn't use the
>>>> multicall technique for handling possible 64-bit return values.
>>>>
>>>> So I continue to argue that on Arm the return value of a hypercall
>>>> should be tested to fit into 32 bits.
>>>
>>> It would make sense. But what would you return if the value doesn't fit?
>>
>> I guess some errno value would be appropriate, like -EDOM, -ERANGE or
>> -E2BIG.
>
> This seems to be better than the alternative below as it is a lot
> simpler.
We would still need to special case XENMEM_maximum_reservation (or
rework the implementation of the sub-op) because the value returned is
an unsigned long. So technically, the unsigned value for -EDOM & co
could be interpreted as the maximum host frame number.
I also would like to see the hypercall returning 'int' when they are
only meant to return 32-bit value. This will make easier to spot someone
that decide to return a 64-bit value.
>
>
>>>> The only really clean alternative
>>>> would be to have separate hypercall function classes for Arm 32- and
>>>> 64-bit guests (which still could share most of the functions by letting
>>>> those return "int"). This would allow to use the 64-bit variant even for
>>>> 32-bit guests in multicall (fine as the return field is 64-bit wide),
>>>> and a probably saturating compat version for the 32-bit guest direct
>>>> hypercall.
>>>
>>> I am not entirely sure to understand this proposal. Can you clarify it?
>>
>> 1. In patch 5 modify the hypercall table by adding another column, so
>> instead of:
>> +table: pv32 pv64 hvm32 hvm64 arm
>> use:
>> +table: pv32 pv64 hvm32 hvm64 arm32 arm64
>>
>> 2. Let most of the hypercalls just return int instead of long:
>> +rettype: do int
>>
>> 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the
>> compat variant existing already):
>> +rettype: do64 long
>> +prefix: do64 PREFIX_hvm
>> +memory_op(unsigned long cmd, void *arg)
>>
>> 4. Use the appropriate calls in each column:
>> +memory_op compat do64 hvm hvm compat do64
>>
>> 5. In the Arm hypercall trap handler do:
>> if ( is_32bit_domain(current->domain) )
>> call_handlers_arm32(...);
>> else
>> call_handlers_arm64(...);
>>
>> 6. In the multicall handler always do:
>> call_handlers_arm64(...);
I am probably missing something. But why do we need to have separate
call handlers for arm32/arm64?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-20 17:14 ` Julien Grall
@ 2021-12-21 0:08 ` Stefano Stabellini
2021-12-21 7:45 ` Juergen Gross
1 sibling, 0 replies; 52+ messages in thread
From: Stefano Stabellini @ 2021-12-21 0:08 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Juergen Gross, Jan Beulich, xen-devel,
Volodymyr Babchuk, Bertrand Marquis, Andrew Cooper,
George Dunlap, Wei Liu, Roger Pau Monné,
Christopher Clark
[-- Attachment #1: Type: text/plain, Size: 6382 bytes --]
On Mon, 20 Dec 2021, Julien Grall wrote:
> On 18/12/2021 00:00, Stefano Stabellini wrote:
> > On Fri, 17 Dec 2021, Juergen Gross wrote:
> > > On 17.12.21 11:41, Julien Grall wrote:
> > > > Hi Juergen,
> > > >
> > > > On 17/12/2021 08:50, Juergen Gross wrote:
> > > > > On 17.12.21 08:45, Jan Beulich wrote:
> > > > > > On 17.12.2021 06:34, Juergen Gross wrote:
> > > > > > > On 16.12.21 22:15, Stefano Stabellini wrote:
> > > > > > > > On Thu, 16 Dec 2021, Stefano Stabellini wrote:
> > > > > > > > > On Thu, 16 Dec 2021, Juergen Gross wrote:
> > > > > > > > > > On 16.12.21 03:10, Stefano Stabellini wrote:
> > > > > > > > > > > The case of XENMEM_maximum_ram_page is interesting but it
> > > > > > > > > > > is
> > > > > > > > > > > not a
> > > > > > > > > > > problem in reality because the max physical address size
> > > > > > > > > > > is
> > > > > > > > > > > only 40-bit
> > > > > > > > > > > for aarch32 guests, so 32-bit are always enough to return
> > > > > > > > > > > the
> > > > > > > > > > > highest
> > > > > > > > > > > page in memory for 32-bit guests.
> > > > > > > > > >
> > > > > > > > > > You are aware that this isn't the guest's max page, but the
> > > > > > > > > > host's?
> > > > > > > >
> > > > > > > > I can see now that you meant to say that, no matter what is the
> > > > > > > > max
> > > > > > > > pseudo-physical address supported by the VM,
> > > > > > > > XENMEM_maximum_ram_page
> > > > > > > > is
> > > > > > > > supposed to return the max memory page, which could go above the
> > > > > > > > addressibility limit of the VM.
> > > > > > > >
> > > > > > > > So XENMEM_maximum_ram_page should potentially be able to return
> > > > > > > > (1<<44)
> > > > > > > > even when called by an aarch32 VM, with max IPA 40-bit.
> > > > > > > >
> > > > > > > > I would imagine it could be useful if dom0 is 32-bit but domUs
> > > > > > > > are
> > > > > > > > 64-bit on a 64-bit hypervisor (which I think it would be a very
> > > > > > > > rare
> > > > > > > > configuration on ARM.)
> > > > > > > >
> > > > > > > > Then it looks like XENMEM_maximum_ram_page needs to be able to
> > > > > > > > return a
> > > > > > > > value > 32-bit when called by a 32-bit guest.
> > > > > > > >
> > > > > > > > The hypercall ABI follows the ARM C calling convention, so a
> > > > > > > > 64-bit
> > > > > > > > value should be returned using r0 and r1. But looking at
> > > > > > > > xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever
> > > > > > > > sets
> > > > > > > > r1
> > > > > > > > today. Only r0 is set, so effectively we only support 32-bit
> > > > > > > > return
> > > > > > > > values on aarch32 and for aarch32 guests.
> > > > > > > >
> > > > > > > > In other words, today all hypercalls on ARM return 64-bit to
> > > > > > > > 64-bit
> > > > > > > > guests and 32-bit to 32-bit guests. Which in the case of
> > > > > > > > memory_op
> > > > > > > > is
> > > > > > > > "technically" the correct thing to do because it matches the C
> > > > > > > > declaration in xen/include/xen/hypercall.h:
> > > > > > > >
> > > > > > > > extern long
> > > > > > > > do_memory_op(
> > > > > > > > unsigned long cmd,
> > > > > > > > XEN_GUEST_HANDLE_PARAM(void) arg);
> > > > > > > >
> > > > > > > > So... I guess the conclusion is that on ARM do_memory_op should
> > > > > > > > return
> > > > > > > > "long" although it is not actually enough for a correct
> > > > > > > > implementation
> > > > > > > > of XENMEM_maximum_ram_page for aarch32 guests ?
> > > > > > > >
> > > > > > >
> > > > > > > Hence my suggestion to check the return value of _all_ hypercalls
> > > > > > > to
> > > > > > > be
> > > > > > > proper sign extended int values for 32-bit guests. This would fix
> > > > > > > all
> > > > > > > potential issues without silently returning truncated values.
> > > > > >
> > > > > > Are we absolutely certain we have no other paths left where a
> > > > > > possibly
> > > > > > large unsigned values might be returned? In fact while
> > > > > > compat_memory_op() does the necessary saturation, I've never been
> > > > > > fully
> > > > > > convinced of this being the best way of dealing with things. The
> > > > > > range
> > > > > > of error indicators is much smaller than [-INT_MIN,-1], so almost
> > > > > > double the range of effectively unsigned values could be passed back
> > > > > > fine. (Obviously we can't change existing interfaces, so this mem-op
> > > > > > will need to remain as is.)
> > > > >
> > > > > In fact libxenctrl tries do deal with this fact by wrapping a
> > > > > memory_op
> > > > > for a 32-bit environment into a multicall. This will work fine for a
> > > > > 32-bit Arm guest, as xen_ulong_t is a uint64 there.
> > > > >
> > > > > So do_memory_op should return long on Arm, yes. OTOH doing so will
> > > > > continue to be a problem in case a 32-bit guest doesn't use the
> > > > > multicall technique for handling possible 64-bit return values.
> > > > >
> > > > > So I continue to argue that on Arm the return value of a hypercall
> > > > > should be tested to fit into 32 bits.
> > > >
> > > > It would make sense. But what would you return if the value doesn't fit?
> > >
> > > I guess some errno value would be appropriate, like -EDOM, -ERANGE or
> > > -E2BIG.
> >
> > This seems to be better than the alternative below as it is a lot
> > simpler.
>
> We would still need to special case XENMEM_maximum_reservation (or rework the
> implementation of the sub-op) because the value returned is an unsigned long.
> So technically, the unsigned value for -EDOM & co could be interpreted as the
> maximum host frame number.
>
> I also would like to see the hypercall returning 'int' when they are only
> meant to return 32-bit value. This will make easier to spot someone that
> decide to return a 64-bit value.
I am completely aligned with you on both points.
XENMEM_maximum_reservation is a bit of a distraction given that is
unused (even unsupported?) on ARM. In general, to switch to "int" as
return type we would have to (manually) check that all the sub-ops of a
given hypercall return 32-bit values, right? Otherwise, how can we be
sure that we don't start to silently truncate the top 32-bit on a sub-op
on arm64?
In theory we could use -Wconversion to automatically spot any
truncations but unfortunately -Wconversion breaks the build at the
moment.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-20 17:14 ` Julien Grall
2021-12-21 0:08 ` Stefano Stabellini
@ 2021-12-21 7:45 ` Juergen Gross
2021-12-21 9:16 ` Julien Grall
1 sibling, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-21 7:45 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini
Cc: Jan Beulich, xen-devel, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Christopher Clark
[-- Attachment #1.1.1: Type: text/plain, Size: 7534 bytes --]
On 20.12.21 18:14, Julien Grall wrote:
> Hi,
>
> On 18/12/2021 00:00, Stefano Stabellini wrote:
>> On Fri, 17 Dec 2021, Juergen Gross wrote:
>>> On 17.12.21 11:41, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 17/12/2021 08:50, Juergen Gross wrote:
>>>>> On 17.12.21 08:45, Jan Beulich wrote:
>>>>>> On 17.12.2021 06:34, Juergen Gross wrote:
>>>>>>> On 16.12.21 22:15, Stefano Stabellini wrote:
>>>>>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>>>>>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>>>>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>>>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is
>>>>>>>>>>> not a
>>>>>>>>>>> problem in reality because the max physical address size is
>>>>>>>>>>> only 40-bit
>>>>>>>>>>> for aarch32 guests, so 32-bit are always enough to return the
>>>>>>>>>>> highest
>>>>>>>>>>> page in memory for 32-bit guests.
>>>>>>>>>>
>>>>>>>>>> You are aware that this isn't the guest's max page, but the
>>>>>>>>>> host's?
>>>>>>>>
>>>>>>>> I can see now that you meant to say that, no matter what is the max
>>>>>>>> pseudo-physical address supported by the VM,
>>>>>>>> XENMEM_maximum_ram_page
>>>>>>>> is
>>>>>>>> supposed to return the max memory page, which could go above the
>>>>>>>> addressibility limit of the VM.
>>>>>>>>
>>>>>>>> So XENMEM_maximum_ram_page should potentially be able to return
>>>>>>>> (1<<44)
>>>>>>>> even when called by an aarch32 VM, with max IPA 40-bit.
>>>>>>>>
>>>>>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are
>>>>>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very
>>>>>>>> rare
>>>>>>>> configuration on ARM.)
>>>>>>>>
>>>>>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to
>>>>>>>> return a
>>>>>>>> value > 32-bit when called by a 32-bit guest.
>>>>>>>>
>>>>>>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit
>>>>>>>> value should be returned using r0 and r1. But looking at
>>>>>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever
>>>>>>>> sets
>>>>>>>> r1
>>>>>>>> today. Only r0 is set, so effectively we only support 32-bit return
>>>>>>>> values on aarch32 and for aarch32 guests.
>>>>>>>>
>>>>>>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit
>>>>>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op
>>>>>>>> is
>>>>>>>> "technically" the correct thing to do because it matches the C
>>>>>>>> declaration in xen/include/xen/hypercall.h:
>>>>>>>>
>>>>>>>> extern long
>>>>>>>> do_memory_op(
>>>>>>>> unsigned long cmd,
>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg);
>>>>>>>>
>>>>>>>> So... I guess the conclusion is that on ARM do_memory_op should
>>>>>>>> return
>>>>>>>> "long" although it is not actually enough for a correct
>>>>>>>> implementation
>>>>>>>> of XENMEM_maximum_ram_page for aarch32 guests ?
>>>>>>>>
>>>>>>>
>>>>>>> Hence my suggestion to check the return value of _all_ hypercalls to
>>>>>>> be
>>>>>>> proper sign extended int values for 32-bit guests. This would fix
>>>>>>> all
>>>>>>> potential issues without silently returning truncated values.
>>>>>>
>>>>>> Are we absolutely certain we have no other paths left where a
>>>>>> possibly
>>>>>> large unsigned values might be returned? In fact while
>>>>>> compat_memory_op() does the necessary saturation, I've never been
>>>>>> fully
>>>>>> convinced of this being the best way of dealing with things. The
>>>>>> range
>>>>>> of error indicators is much smaller than [-INT_MIN,-1], so almost
>>>>>> double the range of effectively unsigned values could be passed back
>>>>>> fine. (Obviously we can't change existing interfaces, so this mem-op
>>>>>> will need to remain as is.)
>>>>>
>>>>> In fact libxenctrl tries do deal with this fact by wrapping a
>>>>> memory_op
>>>>> for a 32-bit environment into a multicall. This will work fine for a
>>>>> 32-bit Arm guest, as xen_ulong_t is a uint64 there.
>>>>>
>>>>> So do_memory_op should return long on Arm, yes. OTOH doing so will
>>>>> continue to be a problem in case a 32-bit guest doesn't use the
>>>>> multicall technique for handling possible 64-bit return values.
>>>>>
>>>>> So I continue to argue that on Arm the return value of a hypercall
>>>>> should be tested to fit into 32 bits.
>>>>
>>>> It would make sense. But what would you return if the value doesn't
>>>> fit?
>>>
>>> I guess some errno value would be appropriate, like -EDOM, -ERANGE or
>>> -E2BIG.
>>
>> This seems to be better than the alternative below as it is a lot
>> simpler.
>
> We would still need to special case XENMEM_maximum_reservation (or
> rework the implementation of the sub-op) because the value returned is
> an unsigned long. So technically, the unsigned value for -EDOM & co
> could be interpreted as the maximum host frame number.
I guess you meant XENMEM_maximum_ram_page.
What about setting -EDOM only if the high 32 bits are not all the same?
This would mean to clamp the highest RAM page to -EDOM in case the
caller is interpreting it as an unsigned value. This would still be
better than silently dropping the high bits, which could lead to a
rather low page number instead.
> I also would like to see the hypercall returning 'int' when they are
> only meant to return 32-bit value. This will make easier to spot someone
> that decide to return a 64-bit value.
I guess this would need to include all hypercalls handled in common
code?
>>>>> The only really clean alternative
>>>>> would be to have separate hypercall function classes for Arm 32- and
>>>>> 64-bit guests (which still could share most of the functions by
>>>>> letting
>>>>> those return "int"). This would allow to use the 64-bit variant
>>>>> even for
>>>>> 32-bit guests in multicall (fine as the return field is 64-bit wide),
>>>>> and a probably saturating compat version for the 32-bit guest direct
>>>>> hypercall.
>>>>
>>>> I am not entirely sure to understand this proposal. Can you clarify it?
>>>
>>> 1. In patch 5 modify the hypercall table by adding another column, so
>>> instead of:
>>> +table: pv32 pv64 hvm32 hvm64 arm
>>> use:
>>> +table: pv32 pv64 hvm32 hvm64 arm32 arm64
>>>
>>> 2. Let most of the hypercalls just return int instead of long:
>>> +rettype: do int
>>>
>>> 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the
>>> compat variant existing already):
>>> +rettype: do64 long
>>> +prefix: do64 PREFIX_hvm
>>> +memory_op(unsigned long cmd, void *arg)
>>>
>>> 4. Use the appropriate calls in each column:
>>> +memory_op compat do64 hvm hvm compat do64
>>>
>>> 5. In the Arm hypercall trap handler do:
>>> if ( is_32bit_domain(current->domain) )
>>> call_handlers_arm32(...);
>>> else
>>> call_handlers_arm64(...);
>>>
>>> 6. In the multicall handler always do:
>>> call_handlers_arm64(...);
> I am probably missing something. But why do we need to have separate
> call handlers for arm32/arm64?
How else could you have different functions called for 32- and 64-bit
guests (other than doing the distinction in the called functions)?
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] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-21 7:45 ` Juergen Gross
@ 2021-12-21 9:16 ` Julien Grall
0 siblings, 0 replies; 52+ messages in thread
From: Julien Grall @ 2021-12-21 9:16 UTC (permalink / raw)
To: Juergen Gross, Stefano Stabellini
Cc: Jan Beulich, xen-devel, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Christopher Clark
Hi Juergen,
On 21/12/2021 08:45, Juergen Gross wrote:
> On 20.12.21 18:14, Julien Grall wrote:
>> Hi,
>>
>> On 18/12/2021 00:00, Stefano Stabellini wrote:
>>> On Fri, 17 Dec 2021, Juergen Gross wrote:
>>>> On 17.12.21 11:41, Julien Grall wrote:
>>>>> Hi Juergen,
>>>>>
>>>>> On 17/12/2021 08:50, Juergen Gross wrote:
>>>>>> On 17.12.21 08:45, Jan Beulich wrote:
>>>>>>> On 17.12.2021 06:34, Juergen Gross wrote:
>>>>>>>> On 16.12.21 22:15, Stefano Stabellini wrote:
>>>>>>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>>>>>>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>>>>>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>>>>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is
>>>>>>>>>>>> not a
>>>>>>>>>>>> problem in reality because the max physical address size is
>>>>>>>>>>>> only 40-bit
>>>>>>>>>>>> for aarch32 guests, so 32-bit are always enough to return the
>>>>>>>>>>>> highest
>>>>>>>>>>>> page in memory for 32-bit guests.
>>>>>>>>>>>
>>>>>>>>>>> You are aware that this isn't the guest's max page, but the
>>>>>>>>>>> host's?
>>>>>>>>>
>>>>>>>>> I can see now that you meant to say that, no matter what is the
>>>>>>>>> max
>>>>>>>>> pseudo-physical address supported by the VM,
>>>>>>>>> XENMEM_maximum_ram_page
>>>>>>>>> is
>>>>>>>>> supposed to return the max memory page, which could go above the
>>>>>>>>> addressibility limit of the VM.
>>>>>>>>>
>>>>>>>>> So XENMEM_maximum_ram_page should potentially be able to return
>>>>>>>>> (1<<44)
>>>>>>>>> even when called by an aarch32 VM, with max IPA 40-bit.
>>>>>>>>>
>>>>>>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are
>>>>>>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very
>>>>>>>>> rare
>>>>>>>>> configuration on ARM.)
>>>>>>>>>
>>>>>>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to
>>>>>>>>> return a
>>>>>>>>> value > 32-bit when called by a 32-bit guest.
>>>>>>>>>
>>>>>>>>> The hypercall ABI follows the ARM C calling convention, so a
>>>>>>>>> 64-bit
>>>>>>>>> value should be returned using r0 and r1. But looking at
>>>>>>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever
>>>>>>>>> sets
>>>>>>>>> r1
>>>>>>>>> today. Only r0 is set, so effectively we only support 32-bit
>>>>>>>>> return
>>>>>>>>> values on aarch32 and for aarch32 guests.
>>>>>>>>>
>>>>>>>>> In other words, today all hypercalls on ARM return 64-bit to
>>>>>>>>> 64-bit
>>>>>>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op
>>>>>>>>> is
>>>>>>>>> "technically" the correct thing to do because it matches the C
>>>>>>>>> declaration in xen/include/xen/hypercall.h:
>>>>>>>>>
>>>>>>>>> extern long
>>>>>>>>> do_memory_op(
>>>>>>>>> unsigned long cmd,
>>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg);
>>>>>>>>>
>>>>>>>>> So... I guess the conclusion is that on ARM do_memory_op should
>>>>>>>>> return
>>>>>>>>> "long" although it is not actually enough for a correct
>>>>>>>>> implementation
>>>>>>>>> of XENMEM_maximum_ram_page for aarch32 guests ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hence my suggestion to check the return value of _all_
>>>>>>>> hypercalls to
>>>>>>>> be
>>>>>>>> proper sign extended int values for 32-bit guests. This would
>>>>>>>> fix all
>>>>>>>> potential issues without silently returning truncated values.
>>>>>>>
>>>>>>> Are we absolutely certain we have no other paths left where a
>>>>>>> possibly
>>>>>>> large unsigned values might be returned? In fact while
>>>>>>> compat_memory_op() does the necessary saturation, I've never been
>>>>>>> fully
>>>>>>> convinced of this being the best way of dealing with things. The
>>>>>>> range
>>>>>>> of error indicators is much smaller than [-INT_MIN,-1], so almost
>>>>>>> double the range of effectively unsigned values could be passed back
>>>>>>> fine. (Obviously we can't change existing interfaces, so this mem-op
>>>>>>> will need to remain as is.)
>>>>>>
>>>>>> In fact libxenctrl tries do deal with this fact by wrapping a
>>>>>> memory_op
>>>>>> for a 32-bit environment into a multicall. This will work fine for a
>>>>>> 32-bit Arm guest, as xen_ulong_t is a uint64 there.
>>>>>>
>>>>>> So do_memory_op should return long on Arm, yes. OTOH doing so will
>>>>>> continue to be a problem in case a 32-bit guest doesn't use the
>>>>>> multicall technique for handling possible 64-bit return values.
>>>>>>
>>>>>> So I continue to argue that on Arm the return value of a hypercall
>>>>>> should be tested to fit into 32 bits.
>>>>>
>>>>> It would make sense. But what would you return if the value doesn't
>>>>> fit?
>>>>
>>>> I guess some errno value would be appropriate, like -EDOM, -ERANGE or
>>>> -E2BIG.
>>>
>>> This seems to be better than the alternative below as it is a lot
>>> simpler.
>>
>> We would still need to special case XENMEM_maximum_reservation (or
>> rework the implementation of the sub-op) because the value returned is
>> an unsigned long. So technically, the unsigned value for -EDOM & co
>> could be interpreted as the maximum host frame number.
>
> I guess you meant XENMEM_maximum_ram_page.
Hmmmm yes.
>
> What about setting -EDOM only if the high 32 bits are not all the same?
> This would mean to clamp the highest RAM page to -EDOM in case the
> caller is interpreting it as an unsigned value. This would still be
> better than silently dropping the high bits, which could lead to a
> rather low page number instead.
This feels like quite a hack. So I would prefer the low page number. I
am interested to hear about the others.
>
>> I also would like to see the hypercall returning 'int' when they are
>> only meant to return 32-bit value. This will make easier to spot
>> someone that decide to return a 64-bit value.
>
> I guess this would need to include all hypercalls handled in common
> code?
Ideally yes.
>
>>>>>> The only really clean alternative
>>>>>> would be to have separate hypercall function classes for Arm 32- and
>>>>>> 64-bit guests (which still could share most of the functions by
>>>>>> letting
>>>>>> those return "int"). This would allow to use the 64-bit variant
>>>>>> even for
>>>>>> 32-bit guests in multicall (fine as the return field is 64-bit wide),
>>>>>> and a probably saturating compat version for the 32-bit guest direct
>>>>>> hypercall.
>>>>>
>>>>> I am not entirely sure to understand this proposal. Can you clarify
>>>>> it?
>>>>
>>>> 1. In patch 5 modify the hypercall table by adding another column, so
>>>> instead of:
>>>> +table: pv32 pv64 hvm32 hvm64 arm
>>>> use:
>>>> +table: pv32 pv64 hvm32 hvm64 arm32
>>>> arm64
>>>>
>>>> 2. Let most of the hypercalls just return int instead of long:
>>>> +rettype: do int
>>>>
>>>> 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the
>>>> compat variant existing already):
>>>> +rettype: do64 long
>>>> +prefix: do64 PREFIX_hvm
>>>> +memory_op(unsigned long cmd, void *arg)
>>>>
>>>> 4. Use the appropriate calls in each column:
>>>> +memory_op compat do64 hvm hvm compat do64
>>>>
>>>> 5. In the Arm hypercall trap handler do:
>>>> if ( is_32bit_domain(current->domain) )
>>>> call_handlers_arm32(...);
>>>> else
>>>> call_handlers_arm64(...);
>>>>
>>>> 6. In the multicall handler always do:
>>>> call_handlers_arm64(...);
>> I am probably missing something. But why do we need to have separate
>> call handlers for arm32/arm64?
>
> How else could you have different functions called for 32- and 64-bit
> guests (other than doing the distinction in the called functions)?
At least for near future, I expect do_memory_op() to be the only one
requiring special distinction. So my preference would be to handle the
difference there rather than adding extra logic/indirection.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-16 21:15 ` Stefano Stabellini
2021-12-17 5:34 ` Juergen Gross
@ 2021-12-17 7:39 ` Jan Beulich
2021-12-17 10:38 ` Julien Grall
2 siblings, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-12-17 7:39 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Juergen Gross, Julien Grall, xen-devel, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark
On 16.12.2021 22:15, Stefano Stabellini wrote:
> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>> On Wed, 15 Dec 2021, Juergen Gross wrote:
>>>>> On 14.12.21 18:36, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 08/12/2021 15:55, 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.
>>>>>>
>>>>>> So on Arm64, I don't think you can make use of the full 64-bit because a
>>>>>> 32-bit domain would not be able to see the top 32-bit.
>>>>>>
>>>>>> In fact, this could potentially cause us some trouble (see [1]) in Xen.
>>>>>> So it feels like the hypercalls should always return a 32-bit signed
>>>>>> value
>>>>>> on Arm.
>>>>>
>>>>> This would break hypercalls like XENMEM_maximum_ram_page which are able
>>>>> to return larger values, right?
>>>>>
>>>>>> The other advantage is it would be clear that the top 32-bit are not
>>>>>> usuable. Stefano, what do you think?
>>>>>
>>>>> Wouldn't it make more sense to check the return value to be a sign
>>>>> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
>>>>>
>>>>> The question is what to return if this is not the case. -EDOM?
>>>>
>>>>
>>>> I can see where Julien is coming from: we have been trying to keep the
>>>> arm32 and arm64 ABIs identical since the beginning of the project. So,
>>>> like Julien, my preference would be to always return 32-bit on ARM, both
>>>> aarch32 and aarch64. It would make things simple.
>>>>
>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a
>>>> problem in reality because the max physical address size is only 40-bit
>>>> for aarch32 guests, so 32-bit are always enough to return the highest
>>>> page in memory for 32-bit guests.
>>>
>>> You are aware that this isn't the guest's max page, but the host's?
>
> I can see now that you meant to say that, no matter what is the max
> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is
> supposed to return the max memory page, which could go above the
> addressibility limit of the VM.
>
> So XENMEM_maximum_ram_page should potentially be able to return (1<<44)
> even when called by an aarch32 VM, with max IPA 40-bit.
>
> I would imagine it could be useful if dom0 is 32-bit but domUs are
> 64-bit on a 64-bit hypervisor (which I think it would be a very rare
> configuration on ARM.)
>
> Then it looks like XENMEM_maximum_ram_page needs to be able to return a
> value > 32-bit when called by a 32-bit guest.
>
> The hypercall ABI follows the ARM C calling convention, so a 64-bit
> value should be returned using r0 and r1. But looking at
> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1
> today. Only r0 is set, so effectively we only support 32-bit return
> values on aarch32 and for aarch32 guests.
>
> In other words, today all hypercalls on ARM return 64-bit to 64-bit
> guests and 32-bit to 32-bit guests. Which in the case of memory_op is
> "technically" the correct thing to do because it matches the C
> declaration in xen/include/xen/hypercall.h:
>
> extern long
> do_memory_op(
> unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg);
>
> So... I guess the conclusion is that on ARM do_memory_op should return
> "long" although it is not actually enough for a correct implementation
> of XENMEM_maximum_ram_page for aarch32 guests ?
For 32-bit guests the value needs to be saturated and allocations be
restricted to the "visible" part of physical address space. Just like
we do on x86. Except of course for Arm the question is in how far
knowledge of the largest physical address in the system is actually
relevant to guests (i.e. like for HVM on x86): This isn't transparent
only for PV, or at least it better would be restricted to PV.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-16 21:15 ` Stefano Stabellini
2021-12-17 5:34 ` Juergen Gross
2021-12-17 7:39 ` Jan Beulich
@ 2021-12-17 10:38 ` Julien Grall
2021-12-17 23:05 ` Stefano Stabellini
2 siblings, 1 reply; 52+ messages in thread
From: Julien Grall @ 2021-12-17 10:38 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Juergen Gross, xen-devel, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné,
Christopher Clark
Hi Stefano,
On 16/12/2021 21:15, Stefano Stabellini wrote:
> On Thu, 16 Dec 2021, Stefano Stabellini wrote:
>> On Thu, 16 Dec 2021, Juergen Gross wrote:
>>> On 16.12.21 03:10, Stefano Stabellini wrote:
>>>> On Wed, 15 Dec 2021, Juergen Gross wrote:
>>>>> On 14.12.21 18:36, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 08/12/2021 15:55, 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.
>>>>>>
>>>>>> So on Arm64, I don't think you can make use of the full 64-bit because a
>>>>>> 32-bit domain would not be able to see the top 32-bit.
>>>>>>
>>>>>> In fact, this could potentially cause us some trouble (see [1]) in Xen.
>>>>>> So it feels like the hypercalls should always return a 32-bit signed
>>>>>> value
>>>>>> on Arm.
>>>>>
>>>>> This would break hypercalls like XENMEM_maximum_ram_page which are able
>>>>> to return larger values, right?
>>>>>
>>>>>> The other advantage is it would be clear that the top 32-bit are not
>>>>>> usuable. Stefano, what do you think?
>>>>>
>>>>> Wouldn't it make more sense to check the return value to be a sign
>>>>> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
>>>>>
>>>>> The question is what to return if this is not the case. -EDOM?
>>>>
>>>>
>>>> I can see where Julien is coming from: we have been trying to keep the
>>>> arm32 and arm64 ABIs identical since the beginning of the project. So,
>>>> like Julien, my preference would be to always return 32-bit on ARM, both
>>>> aarch32 and aarch64. It would make things simple.
>>>>
>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a
>>>> problem in reality because the max physical address size is only 40-bit
>>>> for aarch32 guests, so 32-bit are always enough to return the highest
>>>> page in memory for 32-bit guests.
>>>
>>> You are aware that this isn't the guest's max page, but the host's?
>
> I can see now that you meant to say that, no matter what is the max
> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is
> supposed to return the max memory page, which could go above the
> addressibility limit of the VM.
>
> So XENMEM_maximum_ram_page should potentially be able to return (1<<44)
> even when called by an aarch32 VM, with max IPA 40-bit.
I am a bit confused with what you wrote. Yes, 32-bit VM can only address
40-bit, but this is only limiting its own (guest) physical address
space. Such VM would still be able to map any host physical address
(assuming GFN != MFN).
>
> I would imagine it could be useful if dom0 is 32-bit but domUs are
> 64-bit on a 64-bit hypervisor (which I think it would be a very rare
> configuration on ARM.)
Looking at the implementation, the hypercall is accessible by any
domain. IOW a domU 32-bit could read a wrong value.
That said, it is not clear to me why an Arm or HVM x86 guest would want
to read the value.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-17 10:38 ` Julien Grall
@ 2021-12-17 23:05 ` Stefano Stabellini
0 siblings, 0 replies; 52+ messages in thread
From: Stefano Stabellini @ 2021-12-17 23:05 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Juergen Gross, xen-devel, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné,
Christopher Clark
On Fri, 17 Dec 2021, Julien Grall wrote:
> Hi Stefano,
>
> On 16/12/2021 21:15, Stefano Stabellini wrote:
> > On Thu, 16 Dec 2021, Stefano Stabellini wrote:
> > > On Thu, 16 Dec 2021, Juergen Gross wrote:
> > > > On 16.12.21 03:10, Stefano Stabellini wrote:
> > > > > On Wed, 15 Dec 2021, Juergen Gross wrote:
> > > > > > On 14.12.21 18:36, Julien Grall wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 08/12/2021 15:55, 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.
> > > > > > >
> > > > > > > So on Arm64, I don't think you can make use of the full 64-bit
> > > > > > > because a
> > > > > > > 32-bit domain would not be able to see the top 32-bit.
> > > > > > >
> > > > > > > In fact, this could potentially cause us some trouble (see [1]) in
> > > > > > > Xen.
> > > > > > > So it feels like the hypercalls should always return a 32-bit
> > > > > > > signed
> > > > > > > value
> > > > > > > on Arm.
> > > > > >
> > > > > > This would break hypercalls like XENMEM_maximum_ram_page which are
> > > > > > able
> > > > > > to return larger values, right?
> > > > > >
> > > > > > > The other advantage is it would be clear that the top 32-bit are
> > > > > > > not
> > > > > > > usuable. Stefano, what do you think?
> > > > > >
> > > > > > Wouldn't it make more sense to check the return value to be a sign
> > > > > > extended 32-bit value for 32-bit guests in do_trap_hypercall()
> > > > > > instead?
> > > > > >
> > > > > > The question is what to return if this is not the case. -EDOM?
> > > > >
> > > > >
> > > > > I can see where Julien is coming from: we have been trying to keep the
> > > > > arm32 and arm64 ABIs identical since the beginning of the project. So,
> > > > > like Julien, my preference would be to always return 32-bit on ARM,
> > > > > both
> > > > > aarch32 and aarch64. It would make things simple.
> > > > >
> > > > > The case of XENMEM_maximum_ram_page is interesting but it is not a
> > > > > problem in reality because the max physical address size is only
> > > > > 40-bit
> > > > > for aarch32 guests, so 32-bit are always enough to return the highest
> > > > > page in memory for 32-bit guests.
> > > >
> > > > You are aware that this isn't the guest's max page, but the host's?
> >
> > I can see now that you meant to say that, no matter what is the max
> > pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is
> > supposed to return the max memory page, which could go above the
> > addressibility limit of the VM.
> >
> > So XENMEM_maximum_ram_page should potentially be able to return (1<<44)
> > even when called by an aarch32 VM, with max IPA 40-bit.
>
> I am a bit confused with what you wrote. Yes, 32-bit VM can only address
> 40-bit, but this is only limiting its own (guest) physical address space. Such
> VM would still be able to map any host physical address (assuming GFN != MFN).
I meant to say the same thing that you wrote, sorry it wasn't clear
> > I would imagine it could be useful if dom0 is 32-bit but domUs are
> > 64-bit on a 64-bit hypervisor (which I think it would be a very rare
> > configuration on ARM.)
>
> Looking at the implementation, the hypercall is accessible by any domain. IOW
> a domU 32-bit could read a wrong value.
>
> That said, it is not clear to me why an Arm or HVM x86 guest would want to
> read the value.
Right, indeed. AFAICT it is currently unused on ARM.
Going through the code, the only caller that could potentially use it on
ARM is libxl_domain_core_dump and xc_maximum_ram_page is called on the
if ( !auto_translated_physmap ) code path.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
2021-12-15 7:03 ` Juergen Gross
2021-12-16 2:10 ` Stefano Stabellini
@ 2021-12-17 10:04 ` Julien Grall
1 sibling, 0 replies; 52+ messages in thread
From: Julien Grall @ 2021-12-17 10:04 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné,
Christopher Clark
Hi Juergen,
On 15/12/2021 07:03, Juergen Gross wrote:
> On 14.12.21 18:36, Julien Grall wrote:
>> Hi,
>>
>> On 08/12/2021 15:55, 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.
>>
>> So on Arm64, I don't think you can make use of the full 64-bit because
>> a 32-bit domain would not be able to see the top 32-bit.
>>
>> In fact, this could potentially cause us some trouble (see [1]) in Xen.
>> So it feels like the hypercalls should always return a 32-bit signed
>> value on Arm.
>
> This would break hypercalls like XENMEM_maximum_ram_page which are able
> to return larger values, right?
>
>> The other advantage is it would be clear that the top 32-bit are not
>> usuable. Stefano, what do you think?
>
> Wouldn't it make more sense to check the return value to be a sign
> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead?
>
> The question is what to return if this is not the case. -EDOM?
It looks like there was a lot of discussion afterwards. I will read
everything and answer on the last part of the thread :).
>
>>
>>>
>>> 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.
>>
>> TBH, I think this should have been split because the modifications are
>> done on lines that are untouched.
>>
>> The extra patch would have made the review easier (even if this patch
>> is still quite small).
>
> I can split the patch if you want.
I have already reviewed this patch. So it is not going to help me :).
However, I would appreciate if in the future the coding style changes
are separated at least when they are not meant to be untouched.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 03/13] xen: don't include asm/hypercall.h from C sources
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
2021-12-08 15:55 ` [PATCH v3 01/13] xen: move do_vcpu_op() to arch specific code Juergen Gross
2021-12-08 15:55 ` [PATCH v3 02/13] xen: harmonize return types of hypercall handlers Juergen Gross
@ 2021-12-08 15:55 ` Juergen Gross
2021-12-08 15:55 ` [PATCH v3 04/13] xen: include compat/platform.h from hypercall.h Juergen Gross
` (11 subsequent siblings)
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:55 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Andrew Cooper,
George Dunlap, 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- remove platform_op hunk (Jan Beulich)
- remove including of xen/hypervisor.h from arch/x86/x86_64/mm.c
(Jan Beulich)
- fix include order in common/compat/grant_table.c (Jan Beulich)
---
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 | 2 --
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 ++++
16 files changed, 20 insertions(+), 7 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 8efac7ee60..403cc84324 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 8ec4547bed..73768b09a7 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -22,6 +22,7 @@
#include <xen/param.h>
#include <xen/event.h>
#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/sched.h>
#include <asm/regs.h>
#include <asm/types.h>
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 4a9ac8d5e6..4a44af7199 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 284c2dfb9e..08e9ffa104 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 2ddcd95dce..f8bdc57433 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..3510a5affe 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -25,14 +25,12 @@ EMIT_FILE;
#include <xen/numa.h>
#include <xen/nodemask.h>
#include <xen/guest_access.h>
-#include <xen/hypercall.h>
#include <xen/mem_access.h>
#include <asm/current.h>
#include <asm/asm_defns.h>
#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/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index ff1d678f01..989fdef65b 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -3,6 +3,7 @@
*
*/
+#include <xen/hypercall.h>
#include <compat/grant_table.h>
#define xen_grant_entry_v1 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 0262f2c48a..4d8adf7faa 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] 52+ messages in thread* [PATCH v3 04/13] xen: include compat/platform.h from hypercall.h
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (2 preceding siblings ...)
2021-12-08 15:55 ` [PATCH v3 03/13] xen: don't include asm/hypercall.h from C sources Juergen Gross
@ 2021-12-08 15:55 ` Juergen Gross
2021-12-09 9:01 ` Jan Beulich
2021-12-08 15:55 ` [PATCH v3 05/13] xen: generate hypercall interface related code Juergen Gross
` (10 subsequent siblings)
14 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:55 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall, Stefano Stabellini
The definition of compat_platform_op_t is in compat/platform.h
already, so include that file from hypercall.h instead of repeating
the typedef.
This allows to remove the related include statement from
arch/x86/x86_64/platform_hypercall.c.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
xen/arch/x86/x86_64/platform_hypercall.c | 1 -
xen/include/xen/hypercall.h | 4 +++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c
index f84252bac6..7631058cce 100644
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -6,7 +6,6 @@ EMIT_FILE;
#include <xen/hypercall.h>
#include <xen/lib.h>
-#include <compat/platform.h>
#define xen_platform_op compat_platform_op
#define xen_platform_op_t compat_platform_op_t
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index 9266bc86e9..459172f6a5 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -15,6 +15,9 @@
#include <public/version.h>
#include <public/pmu.h>
#include <public/hvm/dm_op.h>
+#ifdef CONFIG_COMPAT
+#include <compat/platform.h>
+#endif
#include <asm/hypercall.h>
#include <xsm/xsm.h>
@@ -207,7 +210,6 @@ extern int compat_multicall(
int compat_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-typedef struct compat_platform_op compat_platform_op_t;
DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t);
int compat_platform_op(XEN_GUEST_HANDLE_PARAM(compat_platform_op_t) u_xenpf_op);
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v3 04/13] xen: include compat/platform.h from hypercall.h
2021-12-08 15:55 ` [PATCH v3 04/13] xen: include compat/platform.h from hypercall.h Juergen Gross
@ 2021-12-09 9:01 ` Jan Beulich
0 siblings, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-12-09 9:01 UTC (permalink / raw)
To: Juergen Gross
Cc: Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall, Stefano Stabellini,
xen-devel
On 08.12.2021 16:55, Juergen Gross wrote:
> The definition of compat_platform_op_t is in compat/platform.h
> already, so include that file from hypercall.h instead of repeating
> the typedef.
>
> This allows to remove the related include statement from
> arch/x86/x86_64/platform_hypercall.c.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
> V3:
> - new patch
I was actually puzzled by the cover letter stating two already applied
patches when my record said three, yet total patch count having gone
down by just two. Then I came to spot this new patch ...
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 05/13] xen: generate hypercall interface related code
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (3 preceding siblings ...)
2021-12-08 15:55 ` [PATCH v3 04/13] xen: include compat/platform.h from hypercall.h Juergen Gross
@ 2021-12-08 15:55 ` Juergen Gross
2021-12-14 8:56 ` Jan Beulich
2021-12-08 15:55 ` [PATCH v3 06/13] xen: use generated prototypes for hypercall handlers Juergen Gross
` (9 subsequent siblings)
14 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:55 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Andrew Cooper, George Dunlap, 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.
The priorities for the specific hypercalls are based on two benchamrks
performed in guests (PV and PVH):
- make -j 4 of the Xen hypervisor (resulting in cpu load with lots of
processes created)
- scp of a large file to the guest (network load)
With a small additional debug patch applied the number of the
different hypercalls in the guest and in dom0 (for looking at backend
activity related hypercalls) were counted while the benchmark in domU
was running:
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)
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- split platform_op for doe and compat prefixes (Jan Beulich)
- add "addline:" directive
- add priorities to handlers (Jan Beulich)
V2.1:
- add missing "delete" statement in awk script
- optimize case of 2 hypercalls with same priority
V2.2:
- avoid asort() function (Michal Orzel)
V3:
- drop "addline:" support, as no longer needed
- enclose call sequence macros in "({ ... })" (Jan Beulich)
- small style adjustment (Jan Beulich)
- move generating to xen/include/Makefile (Anthony PERARD)
Signed-off-by: Juergen Gross <jgross@suse.com>
---
.gitignore | 1 +
xen/include/Makefile | 13 ++
xen/include/hypercall-defs.c | 280 ++++++++++++++++++++++++++++++
xen/scripts/gen_hypercall.awk | 314 ++++++++++++++++++++++++++++++++++
4 files changed, 608 insertions(+)
create mode 100644 xen/include/hypercall-defs.c
create mode 100644 xen/scripts/gen_hypercall.awk
diff --git a/.gitignore b/.gitignore
index 9513506dd9..753a602e29 100644
--- a/.gitignore
+++ b/.gitignore
@@ -336,6 +336,7 @@ 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/include/Makefile b/xen/include/Makefile
index 95daa8a289..43509131b9 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -77,6 +77,18 @@ compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) config/auto.conf Makefile
cat $(filter %.h,$^) >$@.new
mv -f $@.new $@
+quiet_cmd_genhyp = GEN $@
+define cmd_genhyp
+ awk -f ../scripts/gen_hypercall.awk <$< >$@
+endef
+
+all: xen/hypercall-defs.h
+
+xen/hypercall-defs.h: hypercall-defs.i ../scripts/gen_hypercall.awk FORCE
+ $(call if_changed,genhyp)
+
+targets += xen/hypercall-defs.h
+
ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
all: headers.chk headers99.chk headers++.chk
@@ -133,4 +145,5 @@ endif
clean::
rm -rf compat config generated headers*.chk
+ rm -f xen/hypercall-defs.h hypercall-defs.i
rm -f $(BASEDIR)/include/xen/lib/x86/cpuid-autogen.h
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
new file mode 100644
index 0000000000..e08b0a343a
--- /dev/null
+++ b/xen/include/hypercall-defs.c
@@ -0,0 +1,280 @@
+/*
+ * 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 hypercall handler calling code 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 function prefix can be annotated with a priority by adding ":<prio>" to it
+ * ("1" being the highest priority, higher numbers mean lower priority, no
+ * priority specified is the lowest priority). The generated code will try to
+ * achieve better performance for calling high priority handlers.
+ * 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
+#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+defhandle: compat_platform_op_t
+#endif
+#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)
+#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
+#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+platform_op(compat_platform_op_t *u_xenpf_op)
+#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)
+platform_op(xen_platform_op_t *u_xenpf_op)
+#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
+#if defined(CONFIG_HVM) && defined(CONFIG_X86)
+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:1 do:1 - - -
+set_gdt compat do - - -
+stack_switch do:2 do:2 - - -
+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:2 do:2 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:1 do:1 - - -
+vcpu_op compat do compat:1 do:1 do
+set_segment_base do:2 do:2 - - -
+#ifdef CONFIG_PV
+mmuext_op compat:2 do:2 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:1 do:1 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..34840c514f
--- /dev/null
+++ b/xen/scripts/gen_hypercall.awk
@@ -0,0 +1,314 @@
+# 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 handler call
+function do_call(f, p, i) {
+ 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");
+}
+
+# Generate case statement for call
+function do_case(f, p) {
+ printf(" case __HYPERVISOR_%s: \\\n", fn[f]);
+ do_call(f, p);
+ printf(" break; \\\n");
+}
+
+# Generate switch statement for calling handlers
+function do_switch(ca, p, i) {
+ printf(" switch ( num ) \\\n");
+ printf(" { \\\n");
+ for (i = 1; i <= nc; i++)
+ if (call[i] == ca && call_prio[i] == p)
+ do_case(call_fn[i], call_p[i]);
+ printf(" default: \\\n");
+ printf(" ret = -ENOSYS; \\\n");
+ printf(" break; \\\n");
+ printf(" } \\\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;
+ pref = $(c + 1);
+ idx = index(pref, ":");
+ if (idx == 0)
+ prio = 100;
+ else {
+ prio = substr(pref, idx + 1) + 0;
+ pref = substr(pref, 1, idx - 1);
+ if (prio >= 100 || prio < 1)
+ do_err("Priority must be in the range 1..99");
+ }
+ fnd = 0;
+ for (i = 1; i <= n; i++) {
+ if (fn[i] != $1)
+ continue;
+ for (j = 1; j <= n_pre[i]; j++) {
+ if (pre[i, j] == pref) {
+ prios[col[c], prio]++;
+ if (prios[col[c], prio] == 1) {
+ n_prios[col[c]]++;
+ prio_list[col[c], n_prios[col[c]]] = prio;
+ prio_mask[col[c], prio] = "(1ULL << __HYPERVISOR_"$1")";
+ } else
+ prio_mask[col[c], prio] = prio_mask[col[c], prio] " | (1ULL << __HYPERVISOR_"$1")";
+ nc++;
+ call[nc] = col[c];
+ call_fn[nc] = i;
+ call_p[nc] = j;
+ call_prio[nc] = prio;
+ fnd = 1;
+ }
+ }
+ }
+ if (fnd == 0)
+ do_err("No prototype for prefix/hypercall combination");
+ }
+ 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;
+ need_mask = 0;
+ for (pl = 1; pl <= n_prios[ca]; pl++) {
+ for (pll = pl; pll > 1; pll--) {
+ if (prio_list[ca, pl] > p_list[pll - 1])
+ break;
+ else
+ p_list[pll] = p_list[pll - 1];
+ }
+ p_list[pll] = prio_list[ca, pl];
+ # If any prio but the default one has more than 1 entry we need "mask"
+ if (p_list[pll] != 100 && prios[ca, p_list[pll]] > 1)
+ need_mask = 1;
+ }
+ printf("\n");
+ printf("#define call_handlers_%s(num, ret, a1, a2, a3, a4, a5) \\\n", ca);
+ printf("({ \\\n");
+ if (need_mask)
+ printf(" uint64_t mask = 1ULL << num; \\\n");
+ printf(" ");
+ for (pl = 1; pl <= n_prios[ca]; pl++) {
+ if (prios[ca, p_list[pl]] > 1) {
+ if (pl < n_prios[ca]) {
+ printf(" if ( likely(mask & (%s)) ) \\\n", prio_mask[ca, p_list[pl]]);
+ printf(" { \\\n");
+ }
+ if (prios[ca, p_list[pl]] == 2) {
+ fnd = 0;
+ for (i = 1; i <= nc; i++)
+ if (call[i] == ca && call_prio[i] == p_list[pl]) {
+ fnd++;
+ if (fnd == 1)
+ printf(" if ( num == __HYPERVISOR_%s ) \\\n", fn[call_fn[i]]);
+ else
+ printf(" else \\\n");
+ do_call(call_fn[i], call_p[i]);
+ }
+ } else {
+ do_switch(ca, p_list[pl]);
+ }
+ if (pl < n_prios[ca])
+ printf(" } \\\n");
+ } else {
+ for (i = 1; i <= nc; i++)
+ if (call[i] == ca && call_prio[i] == p_list[pl]) {
+ printf("if ( likely(num == __HYPERVISOR_%s) ) \\\n", fn[call_fn[i]]);
+ do_call(call_fn[i], call_p[i]);
+ }
+ }
+ if (pl < n_prios[ca] || prios[ca, p_list[pl]] <= 2)
+ printf(" else \\\n");
+ }
+ if (prios[ca, p_list[n_prios[ca]]] <= 2) {
+ printf("\\\n");
+ printf(" ret = -ENOSYS; \\\n");
+ }
+ printf("})\n");
+ delete p_list;
+ 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] 52+ messages in thread* Re: [PATCH v3 05/13] xen: generate hypercall interface related code
2021-12-08 15:55 ` [PATCH v3 05/13] xen: generate hypercall interface related code Juergen Gross
@ 2021-12-14 8:56 ` Jan Beulich
0 siblings, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-12-14 8:56 UTC (permalink / raw)
To: Juergen Gross
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 08.12.2021 16:55, 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.
>
> The priorities for the specific hypercalls are based on two benchamrks
> performed in guests (PV and PVH):
>
> - make -j 4 of the Xen hypervisor (resulting in cpu load with lots of
> processes created)
> - scp of a large file to the guest (network load)
>
> With a small additional debug patch applied the number of the
> different hypercalls in the guest and in dom0 (for looking at backend
> activity related hypercalls) were counted while the benchmark in domU
> was running:
>
> 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)
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 06/13] xen: use generated prototypes for hypercall handlers
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (4 preceding siblings ...)
2021-12-08 15:55 ` [PATCH v3 05/13] xen: generate hypercall interface related code Juergen Gross
@ 2021-12-08 15:55 ` Juergen Gross
2021-12-08 15:56 ` [PATCH v3 07/13] x86/pv-shim: don't modify hypercall table Juergen Gross
` (8 subsequent siblings)
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:55 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, 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>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/hvm/hypercall.c | 6 +-
xen/include/asm-x86/hypercall.h | 133 ------------------------
xen/include/xen/hypercall.h | 178 +-------------------------------
3 files changed, 5 insertions(+), 312 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 459172f6a5..f307dfb597 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -21,33 +21,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(xen_platform_op_t) u_xenpf_op);
-
extern long
pci_physdev_op(
int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
@@ -65,171 +51,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);
-
-DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t);
-int compat_platform_op(XEN_GUEST_HANDLE_PARAM(compat_platform_op_t) 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] 52+ messages in thread* [PATCH v3 07/13] x86/pv-shim: don't modify hypercall table
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (5 preceding siblings ...)
2021-12-08 15:55 ` [PATCH v3 06/13] xen: use generated prototypes for hypercall handlers Juergen Gross
@ 2021-12-08 15:56 ` Juergen Gross
2021-12-08 15:56 ` [PATCH v3 08/13] xen/x86: don't use hypercall table for calling compat hypercalls Juergen Gross
` (7 subsequent siblings)
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:56 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- add test for pv_shim to wrappers (Jan Beulich)
- add grant_table_op to hypercall table if CONFIG_PV_SHIM (Jan Beulich)
V3:
- add [un]likely() to pv_shim tests (Jan Beulich)
---
xen/arch/x86/pv/hypercall.c | 2 +-
xen/arch/x86/pv/shim.c | 54 +++++++++++++++++------------------
xen/common/event_channel.c | 9 ++++++
xen/common/grant_table.c | 9 ++++++
xen/include/asm-x86/pv/shim.h | 3 ++
5 files changed, 49 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 7e99dbda34..abe4dd7839 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -62,7 +62,7 @@ const pv_hypercall_table_t pv_hypercall_table[] = {
COMPAT_CALL(xen_version),
HYPERCALL(console_io),
COMPAT_CALL(physdev_op_compat),
-#ifdef CONFIG_GRANT_TABLE
+#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
COMPAT_CALL(grant_table_op),
#endif
HYPERCALL(vm_assist),
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index d9704121a7..9a4a257d4c 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,29 @@ 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)
+{
+ if ( likely(!pv_shim) )
+ return -ENOSYS;
+
+ 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)
+{
+ if ( likely(!pv_shim) )
+ return -ENOSYS;
+
+ 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..0e4973f800 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 ( unlikely(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 4d8adf7faa..e5641608ad 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 {
/*
@@ -3543,6 +3547,11 @@ do_grant_table_op(
long rc;
unsigned int opaque_in = cmd & GNTTABOP_ARG_MASK, opaque_out = 0;
+#ifdef CONFIG_PV_SHIM
+ if ( unlikely(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] 52+ messages in thread* [PATCH v3 08/13] xen/x86: don't use hypercall table for calling compat hypercalls
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (6 preceding siblings ...)
2021-12-08 15:56 ` [PATCH v3 07/13] x86/pv-shim: don't modify hypercall table Juergen Gross
@ 2021-12-08 15:56 ` Juergen Gross
2021-12-08 15:56 ` [PATCH v3 09/13] xen/x86: call hypercall handlers via generated macro Juergen Gross
` (6 subsequent siblings)
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:56 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 abe4dd7839..c391d18130 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] 52+ messages in thread* [PATCH v3 09/13] xen/x86: call hypercall handlers via generated macro
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (7 preceding siblings ...)
2021-12-08 15:56 ` [PATCH v3 08/13] xen/x86: don't use hypercall table for calling compat hypercalls Juergen Gross
@ 2021-12-08 15:56 ` Juergen Gross
2021-12-08 15:56 ` [PATCH v3 10/13] xen/arm: " Juergen Gross
` (5 subsequent siblings)
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:56 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 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. With the additional generated
data the hard coded hypercall_args_table[] can be replaced by tables
using the generated number of parameters.
Note that this change modifies behavior of clobbering registers in a
minor way: in case a hypercall is returning -ENOSYS (or the unsigned
equivalent thereof) for any reason the parameter registers will no
longer be clobbered. This should be of no real concern, as those cases
ought to be extremely rare and reuse of the registers in those cases
seems rather far fetched.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- make hypercall_args_*[] const (Jan Beulich)
- make clobber_regs*() independent from NDEBUG (Jan Beulich)
- drop "L" suffix for 32-bit register clobber value (Jan Beulich)
V3:
- add array_access_nospec() (Jan Beulich)
- remove local variables in hvm_hypercall() (Andrew Cooper)
---
xen/arch/x86/hvm/hypercall.c | 166 +++-------------------------
xen/arch/x86/hypercall.c | 59 ----------
xen/arch/x86/pv/hypercall.c | 184 +++-----------------------------
xen/include/asm-x86/hypercall.h | 55 +++++++---
4 files changed, 76 insertions(+), 388 deletions(-)
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 85b7a33523..5952172476 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 const unsigned char hypercall_args_64[] = hypercall_args_hvm64;
+static const 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.
@@ -230,85 +167,27 @@ int hvm_hypercall(struct cpu_user_regs *regs)
if ( mode == 8 )
{
- unsigned long rdi = regs->rdi;
- unsigned long rsi = regs->rsi;
- unsigned long rdx = regs->rdx;
- unsigned long r10 = regs->r10;
- unsigned long r8 = regs->r8;
-
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
+ eax, regs->rdi, regs->rsi, regs->rdx, regs->r10, regs->r8);
- regs->rax = hvm_hypercall_table[eax].native(rdi, rsi, rdx, r10, r8);
+ call_handlers_hvm64(eax, regs->rax, regs->rdi, regs->rsi, regs->rdx,
+ regs->r10, regs->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;
- }
- }
-#endif
+ if ( !curr->hcall_preempted && regs->rax != -ENOSYS )
+ clobber_regs(regs, get_nargs(hypercall_args_64, eax));
}
else
{
- unsigned int ebx = regs->ebx;
- unsigned int ecx = regs->ecx;
- unsigned int edx = regs->edx;
- unsigned int esi = regs->esi;
- unsigned int edi = regs->edi;
-
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
+ regs->ebx, regs->ecx, regs->edx, regs->esi, regs->edi);
curr->hcall_compat = true;
- regs->eax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi);
+ call_handlers_hvm32(eax, regs->eax, regs->ebx, regs->ecx, regs->edx,
+ regs->esi, regs->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;
- }
- }
-#endif
+ if ( !curr->hcall_preempted && regs->eax != -ENOSYS )
+ clobber_regs32(regs, get_nargs(hypercall_args_32, eax));
}
hvmemul_cache_restore(curr, token);
@@ -327,31 +206,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 c391d18130..ef92266620 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 const 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),
-#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
- 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),
-#endif
-#ifdef CONFIG_KEXEC
- COMPAT_CALL(kexec_op),
+static const unsigned char hypercall_args_32[] = hypercall_args_pv32;
#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,22 +62,10 @@ _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;
- }
- }
-#endif
+ if ( !curr->hcall_preempted && regs->rax != -ENOSYS )
+ clobber_regs(regs, get_nargs(hypercall_args_64, eax));
}
#ifdef CONFIG_PV32
else
@@ -194,17 +76,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,23 +86,11 @@ _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;
- }
- }
-#endif
+ if ( !curr->hcall_preempted && regs->eax != -ENOSYS )
+ clobber_regs32(regs, get_nargs(hypercall_args_32, eax));
}
#endif /* CONFIG_PV32 */
@@ -256,13 +116,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 +125,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..2077e28d23 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,46 @@ compat_common_vcpu_op(
#endif /* CONFIG_COMPAT */
+#ifndef NDEBUG
+static inline unsigned int _get_nargs(const unsigned char *tbl, unsigned int c)
+{
+ return tbl[c];
+}
+#define get_nargs(t, c) _get_nargs(t, array_index_nospec(c, ARRAY_SIZE(t)))
+#else
+#define get_nargs(tbl, c) 0
+#endif
+
+static inline void clobber_regs(struct cpu_user_regs *regs,
+ unsigned int nargs)
+{
+#ifndef NDEBUG
+ /* 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;
+ }
+#endif
+}
+
+static inline void clobber_regs32(struct cpu_user_regs *regs,
+ unsigned int nargs)
+{
+#ifndef NDEBUG
+ /* Deliberately corrupt used parameter regs. */
+ switch ( nargs )
+ {
+ case 5: regs->edi = 0xdeadf00dU; fallthrough;
+ case 4: regs->esi = 0xdeadf00dU; fallthrough;
+ case 3: regs->edx = 0xdeadf00dU; fallthrough;
+ case 2: regs->ecx = 0xdeadf00dU; fallthrough;
+ case 1: regs->ebx = 0xdeadf00dU;
+ }
+#endif
+}
+
#endif /* __ASM_X86_HYPERCALL_H__ */
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v3 10/13] xen/arm: call hypercall handlers via generated macro
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (8 preceding siblings ...)
2021-12-08 15:56 ` [PATCH v3 09/13] xen/x86: call hypercall handlers via generated macro Juergen Gross
@ 2021-12-08 15:56 ` Juergen Gross
2021-12-08 15:56 ` [PATCH v3 11/13] xen/x86: add hypercall performance counters for hvm, correct pv Juergen Gross
` (4 subsequent siblings)
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:56 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel
Instead of using a function table use the generated 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>
Tested-by: Michal Orzel <michal.orzel@arm.com>
---
V2:
- make hypercall_args[] const (Jan Beulich)
---
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..00af76cf14 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 const 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] 52+ messages in thread* [PATCH v3 11/13] xen/x86: add hypercall performance counters for hvm, correct pv
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (9 preceding siblings ...)
2021-12-08 15:56 ` [PATCH v3 10/13] xen/arm: " Juergen Gross
@ 2021-12-08 15:56 ` Juergen Gross
2021-12-08 15:56 ` [PATCH v3 12/13] xen: drop calls_to_multicall performance counter Juergen Gross
` (3 subsequent siblings)
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:56 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 first hypercall entry (set_trap_table) for each
hypercall. Fix that.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@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 5952172476..37f60238d2 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -200,6 +200,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 ef92266620..bf64bb41bb 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -102,7 +102,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] 52+ messages in thread* [PATCH v3 12/13] xen: drop calls_to_multicall performance counter
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (10 preceding siblings ...)
2021-12-08 15:56 ` [PATCH v3 11/13] xen/x86: add hypercall performance counters for hvm, correct pv Juergen Gross
@ 2021-12-08 15:56 ` Juergen Gross
2021-12-08 15:56 ` [PATCH v3 13/13] tools/xenperf: update hypercall names Juergen Gross
` (2 subsequent siblings)
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:56 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
The calls_to_multicall performance counter is basically redundant to
the multicall hypercall counter. The only difference is the counting
of continuation calls, which isn't really that interesting.
Drop the calls_to_multicall performance counter.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- new patch
---
xen/common/multicall.c | 1 -
xen/include/xen/perfc_defn.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index 7b20717c88..2e66f9f0c1 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -114,7 +114,6 @@ do_multicall(
if ( unlikely(disp == mc_preempt) && i < nr_calls )
goto preempted;
- perfc_incr(calls_to_multicall);
perfc_add(calls_from_multicall, i);
mcs->flags = 0;
return rc;
diff --git a/xen/include/xen/perfc_defn.h b/xen/include/xen/perfc_defn.h
index 08b182ccd9..eb6152859e 100644
--- a/xen/include/xen/perfc_defn.h
+++ b/xen/include/xen/perfc_defn.h
@@ -6,7 +6,6 @@
PERFCOUNTER_ARRAY(hypercalls, "hypercalls", NR_hypercalls)
-PERFCOUNTER(calls_to_multicall, "calls to multicall")
PERFCOUNTER(calls_from_multicall, "calls from multicall")
PERFCOUNTER(irqs, "#interrupts")
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v3 13/13] tools/xenperf: update hypercall names
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (11 preceding siblings ...)
2021-12-08 15:56 ` [PATCH v3 12/13] xen: drop calls_to_multicall performance counter Juergen Gross
@ 2021-12-08 15:56 ` Juergen Gross
2021-12-09 9:05 ` [PATCH v3 00/13] xen: drop hypercall function tables Jan Beulich
2022-03-08 8:34 ` Jan Beulich
14 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-08 15:56 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Jan Beulich
The hypercall names need some update.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- new patch
---
tools/misc/xenperf.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tools/misc/xenperf.c b/tools/misc/xenperf.c
index a5fbdaa45f..2ad737e661 100644
--- a/tools/misc/xenperf.c
+++ b/tools/misc/xenperf.c
@@ -57,6 +57,11 @@ const char *hypercall_name_table[64] =
X(sysctl),
X(domctl),
X(kexec_op),
+ X(tmem_op),
+ X(argo_op),
+ X(xenpmu_op),
+ X(dm_op),
+ X(hypfs_op),
X(arch_0),
X(arch_1),
X(arch_2),
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (12 preceding siblings ...)
2021-12-08 15:56 ` [PATCH v3 13/13] tools/xenperf: update hypercall names Juergen Gross
@ 2021-12-09 9:05 ` Jan Beulich
2021-12-09 9:10 ` Juergen Gross
2022-03-08 8:34 ` Jan Beulich
14 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-12-09 9:05 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 08.12.2021 16:55, Juergen Gross wrote:
> 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 call macros.
>
> Another by-product of generating the call macros is the automatic
> generating of the hypercall handler prototypes from the same data base
> which is used to generate the macros.
>
> 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.
>
> Changes in V2:
> - new patches 6, 14, 15
> - patch 7: support hypercall priorities for faster code
> - comments addressed
>
> Changes in V3:
> - patches 1 and 4 removed as already applied
> - comments addressed
>
> Juergen Gross (13):
> xen: move do_vcpu_op() to arch specific code
> xen: harmonize return types of hypercall handlers
> xen: don't include asm/hypercall.h from C sources
> xen: include compat/platform.h from hypercall.h
> 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 generated macro
> xen/arm: call hypercall handlers via generated macro
> xen/x86: add hypercall performance counters for hvm, correct pv
> xen: drop calls_to_multicall performance counter
> tools/xenperf: update hypercall names
It's not easy to tell which, if any, of the later patches are fully
independent of earlier ones and could go in ahead of those awaiting
further acks. Do you have any suggestion there, or should we wait
until things can be applied in order?
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2021-12-09 9:05 ` [PATCH v3 00/13] xen: drop hypercall function tables Jan Beulich
@ 2021-12-09 9:10 ` Juergen Gross
2021-12-13 10:35 ` Jan Beulich
0 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2021-12-09 9:10 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2349 bytes --]
On 09.12.21 10:05, Jan Beulich wrote:
> On 08.12.2021 16:55, Juergen Gross wrote:
>> 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 call macros.
>>
>> Another by-product of generating the call macros is the automatic
>> generating of the hypercall handler prototypes from the same data base
>> which is used to generate the macros.
>>
>> 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.
>>
>> Changes in V2:
>> - new patches 6, 14, 15
>> - patch 7: support hypercall priorities for faster code
>> - comments addressed
>>
>> Changes in V3:
>> - patches 1 and 4 removed as already applied
>> - comments addressed
>>
>> Juergen Gross (13):
>> xen: move do_vcpu_op() to arch specific code
>> xen: harmonize return types of hypercall handlers
>> xen: don't include asm/hypercall.h from C sources
>> xen: include compat/platform.h from hypercall.h
>> 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 generated macro
>> xen/arm: call hypercall handlers via generated macro
>> xen/x86: add hypercall performance counters for hvm, correct pv
>> xen: drop calls_to_multicall performance counter
>> tools/xenperf: update hypercall names
>
> It's not easy to tell which, if any, of the later patches are fully
> independent of earlier ones and could go in ahead of those awaiting
> further acks. Do you have any suggestion there, or should we wait
> until things can be applied in order?
I think all but the last patch should be applied in order. The last one
obviously can be applied at any time.
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] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2021-12-09 9:10 ` Juergen Gross
@ 2021-12-13 10:35 ` Jan Beulich
2021-12-13 10:48 ` Juergen Gross
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-12-13 10:35 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 09.12.2021 10:10, Juergen Gross wrote:
> On 09.12.21 10:05, Jan Beulich wrote:
>> On 08.12.2021 16:55, Juergen Gross wrote:
>>> 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 call macros.
>>>
>>> Another by-product of generating the call macros is the automatic
>>> generating of the hypercall handler prototypes from the same data base
>>> which is used to generate the macros.
>>>
>>> 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.
>>>
>>> Changes in V2:
>>> - new patches 6, 14, 15
>>> - patch 7: support hypercall priorities for faster code
>>> - comments addressed
>>>
>>> Changes in V3:
>>> - patches 1 and 4 removed as already applied
>>> - comments addressed
>>>
>>> Juergen Gross (13):
>>> xen: move do_vcpu_op() to arch specific code
>>> xen: harmonize return types of hypercall handlers
>>> xen: don't include asm/hypercall.h from C sources
>>> xen: include compat/platform.h from hypercall.h
>>> 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 generated macro
>>> xen/arm: call hypercall handlers via generated macro
>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>> xen: drop calls_to_multicall performance counter
>>> tools/xenperf: update hypercall names
>>
>> It's not easy to tell which, if any, of the later patches are fully
>> independent of earlier ones and could go in ahead of those awaiting
>> further acks. Do you have any suggestion there, or should we wait
>> until things can be applied in order?
>
> I think all but the last patch should be applied in order. The last one
> obviously can be applied at any time.
Hmm, I think 11 and 12 are fine to go ahead as well; I actually need them
for some immediate purpose and hence I did pull them (but nothing else)
into my local tree, without observing issues.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2021-12-13 10:35 ` Jan Beulich
@ 2021-12-13 10:48 ` Juergen Gross
0 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2021-12-13 10:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2796 bytes --]
On 13.12.21 11:35, Jan Beulich wrote:
> On 09.12.2021 10:10, Juergen Gross wrote:
>> On 09.12.21 10:05, Jan Beulich wrote:
>>> On 08.12.2021 16:55, Juergen Gross wrote:
>>>> 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 call macros.
>>>>
>>>> Another by-product of generating the call macros is the automatic
>>>> generating of the hypercall handler prototypes from the same data base
>>>> which is used to generate the macros.
>>>>
>>>> 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.
>>>>
>>>> Changes in V2:
>>>> - new patches 6, 14, 15
>>>> - patch 7: support hypercall priorities for faster code
>>>> - comments addressed
>>>>
>>>> Changes in V3:
>>>> - patches 1 and 4 removed as already applied
>>>> - comments addressed
>>>>
>>>> Juergen Gross (13):
>>>> xen: move do_vcpu_op() to arch specific code
>>>> xen: harmonize return types of hypercall handlers
>>>> xen: don't include asm/hypercall.h from C sources
>>>> xen: include compat/platform.h from hypercall.h
>>>> 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 generated macro
>>>> xen/arm: call hypercall handlers via generated macro
>>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>>> xen: drop calls_to_multicall performance counter
>>>> tools/xenperf: update hypercall names
>>>
>>> It's not easy to tell which, if any, of the later patches are fully
>>> independent of earlier ones and could go in ahead of those awaiting
>>> further acks. Do you have any suggestion there, or should we wait
>>> until things can be applied in order?
>>
>> I think all but the last patch should be applied in order. The last one
>> obviously can be applied at any time.
>
> Hmm, I think 11 and 12 are fine to go ahead as well; I actually need them
> for some immediate purpose and hence I did pull them (but nothing else)
> into my local tree, without observing issues.
Yeah, those should be okay to take.
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] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2021-12-08 15:55 [PATCH v3 00/13] xen: drop hypercall function tables Juergen Gross
` (13 preceding siblings ...)
2021-12-09 9:05 ` [PATCH v3 00/13] xen: drop hypercall function tables Jan Beulich
@ 2022-03-08 8:34 ` Jan Beulich
2022-03-08 8:39 ` Juergen Gross
14 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2022-03-08 8:34 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 08.12.2021 16:55, Juergen Gross wrote:
> 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 call macros.
>
> Another by-product of generating the call macros is the automatic
> generating of the hypercall handler prototypes from the same data base
> which is used to generate the macros.
>
> 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.
>
> Changes in V2:
> - new patches 6, 14, 15
> - patch 7: support hypercall priorities for faster code
> - comments addressed
>
> Changes in V3:
> - patches 1 and 4 removed as already applied
> - comments addressed
>
> Juergen Gross (13):
> xen: move do_vcpu_op() to arch specific code
> xen: harmonize return types of hypercall handlers
> xen: don't include asm/hypercall.h from C sources
> xen: include compat/platform.h from hypercall.h
> 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 generated macro
> xen/arm: call hypercall handlers via generated macro
> xen/x86: add hypercall performance counters for hvm, correct pv
> xen: drop calls_to_multicall performance counter
> tools/xenperf: update hypercall names
As it's pretty certain now that parts of this which didn't go in yet will
need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
expecting a v4 instead.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2022-03-08 8:34 ` Jan Beulich
@ 2022-03-08 8:39 ` Juergen Gross
2022-03-08 8:50 ` Jan Beulich
2022-03-08 12:50 ` Jan Beulich
0 siblings, 2 replies; 52+ messages in thread
From: Juergen Gross @ 2022-03-08 8:39 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2418 bytes --]
On 08.03.22 09:34, Jan Beulich wrote:
> On 08.12.2021 16:55, Juergen Gross wrote:
>> 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 call macros.
>>
>> Another by-product of generating the call macros is the automatic
>> generating of the hypercall handler prototypes from the same data base
>> which is used to generate the macros.
>>
>> 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.
>>
>> Changes in V2:
>> - new patches 6, 14, 15
>> - patch 7: support hypercall priorities for faster code
>> - comments addressed
>>
>> Changes in V3:
>> - patches 1 and 4 removed as already applied
>> - comments addressed
>>
>> Juergen Gross (13):
>> xen: move do_vcpu_op() to arch specific code
>> xen: harmonize return types of hypercall handlers
>> xen: don't include asm/hypercall.h from C sources
>> xen: include compat/platform.h from hypercall.h
>> 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 generated macro
>> xen/arm: call hypercall handlers via generated macro
>> xen/x86: add hypercall performance counters for hvm, correct pv
>> xen: drop calls_to_multicall performance counter
>> tools/xenperf: update hypercall names
>
> As it's pretty certain now that parts of this which didn't go in yet will
> need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
> expecting a v4 instead.
Yes, I was planning to spin that up soon.
The main remaining question is whether we want to switch the return type
of all hypercalls (or at least the ones common to all archs) not
requiring to return 64-bit values to "int", as Julien requested.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2022-03-08 8:39 ` Juergen Gross
@ 2022-03-08 8:50 ` Jan Beulich
2022-03-08 8:53 ` Juergen Gross
2022-03-08 12:50 ` Jan Beulich
1 sibling, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2022-03-08 8:50 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 08.03.2022 09:39, Juergen Gross wrote:
> On 08.03.22 09:34, Jan Beulich wrote:
>> On 08.12.2021 16:55, Juergen Gross wrote:
>>> 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 call macros.
>>>
>>> Another by-product of generating the call macros is the automatic
>>> generating of the hypercall handler prototypes from the same data base
>>> which is used to generate the macros.
>>>
>>> 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.
>>>
>>> Changes in V2:
>>> - new patches 6, 14, 15
>>> - patch 7: support hypercall priorities for faster code
>>> - comments addressed
>>>
>>> Changes in V3:
>>> - patches 1 and 4 removed as already applied
>>> - comments addressed
>>>
>>> Juergen Gross (13):
>>> xen: move do_vcpu_op() to arch specific code
>>> xen: harmonize return types of hypercall handlers
>>> xen: don't include asm/hypercall.h from C sources
>>> xen: include compat/platform.h from hypercall.h
>>> 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 generated macro
>>> xen/arm: call hypercall handlers via generated macro
>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>> xen: drop calls_to_multicall performance counter
>>> tools/xenperf: update hypercall names
>>
>> As it's pretty certain now that parts of this which didn't go in yet will
>> need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
>> expecting a v4 instead.
>
> Yes, I was planning to spin that up soon.
>
> The main remaining question is whether we want to switch the return type
> of all hypercalls (or at least the ones common to all archs) not
> requiring to return 64-bit values to "int", as Julien requested.
Could you remind me of the (sub)thread this was in, to read through the
justification again? Without recalling any details I guess I'd prefer
to stick to long for non-compat flavors.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2022-03-08 8:50 ` Jan Beulich
@ 2022-03-08 8:53 ` Juergen Gross
0 siblings, 0 replies; 52+ messages in thread
From: Juergen Gross @ 2022-03-08 8:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2932 bytes --]
On 08.03.22 09:50, Jan Beulich wrote:
> On 08.03.2022 09:39, Juergen Gross wrote:
>> On 08.03.22 09:34, Jan Beulich wrote:
>>> On 08.12.2021 16:55, Juergen Gross wrote:
>>>> 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 call macros.
>>>>
>>>> Another by-product of generating the call macros is the automatic
>>>> generating of the hypercall handler prototypes from the same data base
>>>> which is used to generate the macros.
>>>>
>>>> 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.
>>>>
>>>> Changes in V2:
>>>> - new patches 6, 14, 15
>>>> - patch 7: support hypercall priorities for faster code
>>>> - comments addressed
>>>>
>>>> Changes in V3:
>>>> - patches 1 and 4 removed as already applied
>>>> - comments addressed
>>>>
>>>> Juergen Gross (13):
>>>> xen: move do_vcpu_op() to arch specific code
>>>> xen: harmonize return types of hypercall handlers
>>>> xen: don't include asm/hypercall.h from C sources
>>>> xen: include compat/platform.h from hypercall.h
>>>> 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 generated macro
>>>> xen/arm: call hypercall handlers via generated macro
>>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>>> xen: drop calls_to_multicall performance counter
>>>> tools/xenperf: update hypercall names
>>>
>>> As it's pretty certain now that parts of this which didn't go in yet will
>>> need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
>>> expecting a v4 instead.
>>
>> Yes, I was planning to spin that up soon.
>>
>> The main remaining question is whether we want to switch the return type
>> of all hypercalls (or at least the ones common to all archs) not
>> requiring to return 64-bit values to "int", as Julien requested.
>
> Could you remind me of the (sub)thread this was in, to read through the
> justification again? Without recalling any details I guess I'd prefer
> to stick to long for non-compat flavors.
This discussion started with:
https://lists.xen.org/archives/html/xen-devel/2021-12/threads.html#01293
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2022-03-08 8:39 ` Juergen Gross
2022-03-08 8:50 ` Jan Beulich
@ 2022-03-08 12:50 ` Jan Beulich
2022-03-08 12:56 ` Juergen Gross
1 sibling, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2022-03-08 12:50 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 08.03.2022 09:39, Juergen Gross wrote:
> On 08.03.22 09:34, Jan Beulich wrote:
>> On 08.12.2021 16:55, Juergen Gross wrote:
>>> 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 call macros.
>>>
>>> Another by-product of generating the call macros is the automatic
>>> generating of the hypercall handler prototypes from the same data base
>>> which is used to generate the macros.
>>>
>>> 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.
>>>
>>> Changes in V2:
>>> - new patches 6, 14, 15
>>> - patch 7: support hypercall priorities for faster code
>>> - comments addressed
>>>
>>> Changes in V3:
>>> - patches 1 and 4 removed as already applied
>>> - comments addressed
>>>
>>> Juergen Gross (13):
>>> xen: move do_vcpu_op() to arch specific code
>>> xen: harmonize return types of hypercall handlers
>>> xen: don't include asm/hypercall.h from C sources
>>> xen: include compat/platform.h from hypercall.h
>>> 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 generated macro
>>> xen/arm: call hypercall handlers via generated macro
>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>> xen: drop calls_to_multicall performance counter
>>> tools/xenperf: update hypercall names
>>
>> As it's pretty certain now that parts of this which didn't go in yet will
>> need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
>> expecting a v4 instead.
>
> Yes, I was planning to spin that up soon.
>
> The main remaining question is whether we want to switch the return type
> of all hypercalls (or at least the ones common to all archs) not
> requiring to return 64-bit values to "int", as Julien requested.
After walking through the earlier discussion (Jürgen - thanks for the link)
I'm inclined to say that if Arm wants their return values limited to 32 bits
(with exceptions where needed), so be it. But on x86 I'd rather not see us
change this aspect. Of course I'd much prefer if architectures didn't
diverge in this regard, yet then again Arm has already diverged in avoiding
the compat layer (in this case I view the divergence as helpful, though, as
it avoids unnecessary headache).
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2022-03-08 12:50 ` Jan Beulich
@ 2022-03-08 12:56 ` Juergen Gross
2022-03-08 13:42 ` Jan Beulich
0 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2022-03-08 12:56 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 3425 bytes --]
On 08.03.22 13:50, Jan Beulich wrote:
> On 08.03.2022 09:39, Juergen Gross wrote:
>> On 08.03.22 09:34, Jan Beulich wrote:
>>> On 08.12.2021 16:55, Juergen Gross wrote:
>>>> 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 call macros.
>>>>
>>>> Another by-product of generating the call macros is the automatic
>>>> generating of the hypercall handler prototypes from the same data base
>>>> which is used to generate the macros.
>>>>
>>>> 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.
>>>>
>>>> Changes in V2:
>>>> - new patches 6, 14, 15
>>>> - patch 7: support hypercall priorities for faster code
>>>> - comments addressed
>>>>
>>>> Changes in V3:
>>>> - patches 1 and 4 removed as already applied
>>>> - comments addressed
>>>>
>>>> Juergen Gross (13):
>>>> xen: move do_vcpu_op() to arch specific code
>>>> xen: harmonize return types of hypercall handlers
>>>> xen: don't include asm/hypercall.h from C sources
>>>> xen: include compat/platform.h from hypercall.h
>>>> 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 generated macro
>>>> xen/arm: call hypercall handlers via generated macro
>>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>>> xen: drop calls_to_multicall performance counter
>>>> tools/xenperf: update hypercall names
>>>
>>> As it's pretty certain now that parts of this which didn't go in yet will
>>> need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
>>> expecting a v4 instead.
>>
>> Yes, I was planning to spin that up soon.
>>
>> The main remaining question is whether we want to switch the return type
>> of all hypercalls (or at least the ones common to all archs) not
>> requiring to return 64-bit values to "int", as Julien requested.
>
> After walking through the earlier discussion (Jürgen - thanks for the link)
> I'm inclined to say that if Arm wants their return values limited to 32 bits
> (with exceptions where needed), so be it. But on x86 I'd rather not see us
> change this aspect. Of course I'd much prefer if architectures didn't
> diverge in this regard, yet then again Arm has already diverged in avoiding
> the compat layer (in this case I view the divergence as helpful, though, as
> it avoids unnecessary headache).
How to handle this in common code then? Have a hypercall_ret_t type
(exact naming TBD) which is defined as long on x86 and int on Arm?
Or use long in the handlers and check the value on Arm side to be a
valid 32-bit signed int (this would be cumbersome for the exceptions,
though)?
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2022-03-08 12:56 ` Juergen Gross
@ 2022-03-08 13:42 ` Jan Beulich
2022-03-08 13:44 ` Juergen Gross
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2022-03-08 13:42 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 08.03.2022 13:56, Juergen Gross wrote:
> On 08.03.22 13:50, Jan Beulich wrote:
>> On 08.03.2022 09:39, Juergen Gross wrote:
>>> On 08.03.22 09:34, Jan Beulich wrote:
>>>> On 08.12.2021 16:55, Juergen Gross wrote:
>>>>> 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 call macros.
>>>>>
>>>>> Another by-product of generating the call macros is the automatic
>>>>> generating of the hypercall handler prototypes from the same data base
>>>>> which is used to generate the macros.
>>>>>
>>>>> 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.
>>>>>
>>>>> Changes in V2:
>>>>> - new patches 6, 14, 15
>>>>> - patch 7: support hypercall priorities for faster code
>>>>> - comments addressed
>>>>>
>>>>> Changes in V3:
>>>>> - patches 1 and 4 removed as already applied
>>>>> - comments addressed
>>>>>
>>>>> Juergen Gross (13):
>>>>> xen: move do_vcpu_op() to arch specific code
>>>>> xen: harmonize return types of hypercall handlers
>>>>> xen: don't include asm/hypercall.h from C sources
>>>>> xen: include compat/platform.h from hypercall.h
>>>>> 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 generated macro
>>>>> xen/arm: call hypercall handlers via generated macro
>>>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>>>> xen: drop calls_to_multicall performance counter
>>>>> tools/xenperf: update hypercall names
>>>>
>>>> As it's pretty certain now that parts of this which didn't go in yet will
>>>> need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
>>>> expecting a v4 instead.
>>>
>>> Yes, I was planning to spin that up soon.
>>>
>>> The main remaining question is whether we want to switch the return type
>>> of all hypercalls (or at least the ones common to all archs) not
>>> requiring to return 64-bit values to "int", as Julien requested.
>>
>> After walking through the earlier discussion (Jürgen - thanks for the link)
>> I'm inclined to say that if Arm wants their return values limited to 32 bits
>> (with exceptions where needed), so be it. But on x86 I'd rather not see us
>> change this aspect. Of course I'd much prefer if architectures didn't
>> diverge in this regard, yet then again Arm has already diverged in avoiding
>> the compat layer (in this case I view the divergence as helpful, though, as
>> it avoids unnecessary headache).
>
> How to handle this in common code then? Have a hypercall_ret_t type
> (exact naming TBD) which is defined as long on x86 and int on Arm?
> Or use long in the handlers and check the value on Arm side to be a
> valid 32-bit signed int (this would be cumbersome for the exceptions,
> though)?
I was thinking along the lines of hypercall_ret_t, yes, but the
compiler wouldn't be helping with spotting truncation issues (we can't
reasonably enable the respective warnings, as they would trigger all
over the place). If we were to go that route, we'd rely on an initial
audit and subsequent patch review to spot issues. Therefore,
cumbersome or not, the checking approach may be the more viable one.
Then again Julien may have a better plan in mind; I'd anyway expect
him to supply details on how he thinks such a transition could be done
safely, as he was the one to request limiting to 32 bits.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2022-03-08 13:42 ` Jan Beulich
@ 2022-03-08 13:44 ` Juergen Gross
2022-03-08 13:52 ` Jan Beulich
0 siblings, 1 reply; 52+ messages in thread
From: Juergen Gross @ 2022-03-08 13:44 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 4469 bytes --]
On 08.03.22 14:42, Jan Beulich wrote:
> On 08.03.2022 13:56, Juergen Gross wrote:
>> On 08.03.22 13:50, Jan Beulich wrote:
>>> On 08.03.2022 09:39, Juergen Gross wrote:
>>>> On 08.03.22 09:34, Jan Beulich wrote:
>>>>> On 08.12.2021 16:55, Juergen Gross wrote:
>>>>>> 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 call macros.
>>>>>>
>>>>>> Another by-product of generating the call macros is the automatic
>>>>>> generating of the hypercall handler prototypes from the same data base
>>>>>> which is used to generate the macros.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Changes in V2:
>>>>>> - new patches 6, 14, 15
>>>>>> - patch 7: support hypercall priorities for faster code
>>>>>> - comments addressed
>>>>>>
>>>>>> Changes in V3:
>>>>>> - patches 1 and 4 removed as already applied
>>>>>> - comments addressed
>>>>>>
>>>>>> Juergen Gross (13):
>>>>>> xen: move do_vcpu_op() to arch specific code
>>>>>> xen: harmonize return types of hypercall handlers
>>>>>> xen: don't include asm/hypercall.h from C sources
>>>>>> xen: include compat/platform.h from hypercall.h
>>>>>> 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 generated macro
>>>>>> xen/arm: call hypercall handlers via generated macro
>>>>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>>>>> xen: drop calls_to_multicall performance counter
>>>>>> tools/xenperf: update hypercall names
>>>>>
>>>>> As it's pretty certain now that parts of this which didn't go in yet will
>>>>> need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
>>>>> expecting a v4 instead.
>>>>
>>>> Yes, I was planning to spin that up soon.
>>>>
>>>> The main remaining question is whether we want to switch the return type
>>>> of all hypercalls (or at least the ones common to all archs) not
>>>> requiring to return 64-bit values to "int", as Julien requested.
>>>
>>> After walking through the earlier discussion (Jürgen - thanks for the link)
>>> I'm inclined to say that if Arm wants their return values limited to 32 bits
>>> (with exceptions where needed), so be it. But on x86 I'd rather not see us
>>> change this aspect. Of course I'd much prefer if architectures didn't
>>> diverge in this regard, yet then again Arm has already diverged in avoiding
>>> the compat layer (in this case I view the divergence as helpful, though, as
>>> it avoids unnecessary headache).
>>
>> How to handle this in common code then? Have a hypercall_ret_t type
>> (exact naming TBD) which is defined as long on x86 and int on Arm?
>> Or use long in the handlers and check the value on Arm side to be a
>> valid 32-bit signed int (this would be cumbersome for the exceptions,
>> though)?
>
> I was thinking along the lines of hypercall_ret_t, yes, but the
> compiler wouldn't be helping with spotting truncation issues (we can't
> reasonably enable the respective warnings, as they would trigger all
> over the place). If we were to go that route, we'd rely on an initial
> audit and subsequent patch review to spot issues. Therefore,
> cumbersome or not, the checking approach may be the more viable one.
>
> Then again Julien may have a better plan in mind; I'd anyway expect
> him to supply details on how he thinks such a transition could be done
> safely, as he was the one to request limiting to 32 bits.
In order to have some progress I could just leave the Arm side alone
in my series. It could be added later if a solution has been agreed
on.
What do you think?
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 00/13] xen: drop hypercall function tables
2022-03-08 13:44 ` Juergen Gross
@ 2022-03-08 13:52 ` Jan Beulich
0 siblings, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2022-03-08 13:52 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Bertrand Marquis, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
Christopher Clark, xen-devel
On 08.03.2022 14:44, Juergen Gross wrote:
> On 08.03.22 14:42, Jan Beulich wrote:
>> On 08.03.2022 13:56, Juergen Gross wrote:
>>> On 08.03.22 13:50, Jan Beulich wrote:
>>>> On 08.03.2022 09:39, Juergen Gross wrote:
>>>>> On 08.03.22 09:34, Jan Beulich wrote:
>>>>>> On 08.12.2021 16:55, Juergen Gross wrote:
>>>>>>> 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 call macros.
>>>>>>>
>>>>>>> Another by-product of generating the call macros is the automatic
>>>>>>> generating of the hypercall handler prototypes from the same data base
>>>>>>> which is used to generate the macros.
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> Changes in V2:
>>>>>>> - new patches 6, 14, 15
>>>>>>> - patch 7: support hypercall priorities for faster code
>>>>>>> - comments addressed
>>>>>>>
>>>>>>> Changes in V3:
>>>>>>> - patches 1 and 4 removed as already applied
>>>>>>> - comments addressed
>>>>>>>
>>>>>>> Juergen Gross (13):
>>>>>>> xen: move do_vcpu_op() to arch specific code
>>>>>>> xen: harmonize return types of hypercall handlers
>>>>>>> xen: don't include asm/hypercall.h from C sources
>>>>>>> xen: include compat/platform.h from hypercall.h
>>>>>>> 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 generated macro
>>>>>>> xen/arm: call hypercall handlers via generated macro
>>>>>>> xen/x86: add hypercall performance counters for hvm, correct pv
>>>>>>> xen: drop calls_to_multicall performance counter
>>>>>>> tools/xenperf: update hypercall names
>>>>>>
>>>>>> As it's pretty certain now that parts of this which didn't go in yet will
>>>>>> need re-basing, I'm going to drop this from my waiting-to-be-acked folder,
>>>>>> expecting a v4 instead.
>>>>>
>>>>> Yes, I was planning to spin that up soon.
>>>>>
>>>>> The main remaining question is whether we want to switch the return type
>>>>> of all hypercalls (or at least the ones common to all archs) not
>>>>> requiring to return 64-bit values to "int", as Julien requested.
>>>>
>>>> After walking through the earlier discussion (Jürgen - thanks for the link)
>>>> I'm inclined to say that if Arm wants their return values limited to 32 bits
>>>> (with exceptions where needed), so be it. But on x86 I'd rather not see us
>>>> change this aspect. Of course I'd much prefer if architectures didn't
>>>> diverge in this regard, yet then again Arm has already diverged in avoiding
>>>> the compat layer (in this case I view the divergence as helpful, though, as
>>>> it avoids unnecessary headache).
>>>
>>> How to handle this in common code then? Have a hypercall_ret_t type
>>> (exact naming TBD) which is defined as long on x86 and int on Arm?
>>> Or use long in the handlers and check the value on Arm side to be a
>>> valid 32-bit signed int (this would be cumbersome for the exceptions,
>>> though)?
>>
>> I was thinking along the lines of hypercall_ret_t, yes, but the
>> compiler wouldn't be helping with spotting truncation issues (we can't
>> reasonably enable the respective warnings, as they would trigger all
>> over the place). If we were to go that route, we'd rely on an initial
>> audit and subsequent patch review to spot issues. Therefore,
>> cumbersome or not, the checking approach may be the more viable one.
>>
>> Then again Julien may have a better plan in mind; I'd anyway expect
>> him to supply details on how he thinks such a transition could be done
>> safely, as he was the one to request limiting to 32 bits.
>
> In order to have some progress I could just leave the Arm side alone
> in my series. It could be added later if a solution has been agreed
> on.
>
> What do you think?
I see no issue with this if there's no other dependency on Arm following
suit.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread