Multicall compat translation and hypercall continuation handling can
also be shrunk to the processing of just (up to) 5 arguments.
Take the opportunity to
- make exceeding the limit noisy in hypercall_create_continuation(),
- use speculation-safe array access in hypercall_create_continuation(),
- avoid a Misra C:2012 Rule 19.1 violation in xlat_multicall_entry(),
- further tidy xlat_multicall_entry() and __trace_multicall_call()
style-wise.
Amends: 2f531c122e95 ("x86: limit number of hypercall parameters to 5")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
hypercall_xlat_continuation() uses BUG() when encountering too large an
argument count in release builds, but I think that's too harsh. Hence in
hypercall_create_continuation() I'm re-using the existing error path.
Interestingly the multicall part of hypercall_xlat_continuation() has no
check at all which would cover release builds.
With gcc14 code size grows according to my observation, due to the loops
in xlat_multicall_entry() and __trace_multicall_call() both being
unrolled now.
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -392,7 +392,11 @@ unsigned long hypercall_create_continuat
if ( mcs->flags & MCSF_in_multicall )
{
for ( i = 0; *p != '\0'; i++ )
- mcs->call.args[i] = NEXT_ARG(p, args);
+ {
+ if ( i >= ARRAY_SIZE(mcs->call.args) )
+ goto bad_fmt;
+ array_access_nospec(mcs->call.args, i) = NEXT_ARG(p, args);
+ }
/* Return value gets written back to mcs->call.result */
rc = mcs->call.result;
@@ -417,7 +421,7 @@ unsigned long hypercall_create_continuat
case 2: regs->x2 = arg; break;
case 3: regs->x3 = arg; break;
case 4: regs->x4 = arg; break;
- case 5: regs->x5 = arg; break;
+ default: goto bad_fmt;
}
}
@@ -440,7 +444,7 @@ unsigned long hypercall_create_continuat
case 2: regs->r2 = arg; break;
case 3: regs->r3 = arg; break;
case 4: regs->r4 = arg; break;
- case 5: regs->r5 = arg; break;
+ default: goto bad_fmt;
}
}
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -41,7 +41,11 @@ unsigned long hypercall_create_continuat
if ( mcs->flags & MCSF_in_multicall )
{
for ( i = 0; *p != '\0'; i++ )
- mcs->call.args[i] = NEXT_ARG(p, args);
+ {
+ if ( i >= ARRAY_SIZE(mcs->call.args) )
+ goto bad_fmt;
+ array_access_nospec(mcs->call.args, i) = NEXT_ARG(p, args);
+ }
}
else
{
@@ -65,7 +69,7 @@ unsigned long hypercall_create_continuat
case 2: regs->rdx = arg; break;
case 3: regs->r10 = arg; break;
case 4: regs->r8 = arg; break;
- case 5: regs->r9 = arg; break;
+ default: goto bad_fmt;
}
}
}
@@ -81,7 +85,7 @@ unsigned long hypercall_create_continuat
case 2: regs->rdx = arg; break;
case 3: regs->rsi = arg; break;
case 4: regs->rdi = arg; break;
- case 5: regs->rbp = arg; break;
+ default: goto bad_fmt;
}
}
}
@@ -177,7 +181,6 @@ int hypercall_xlat_continuation(unsigned
case 2: reg = ®s->rdx; break;
case 3: reg = ®s->rsi; break;
case 4: reg = ®s->rdi; break;
- case 5: reg = ®s->rbp; break;
default: BUG(); reg = NULL; break;
}
if ( (mask & 1) )
--- a/xen/common/compat/multicall.c
+++ b/xen/common/compat/multicall.c
@@ -14,9 +14,13 @@ typedef int ret_t;
static inline void xlat_multicall_entry(struct mc_state *mcs)
{
- int i;
- for (i=0; i<6; i++)
- mcs->compat_call.args[i] = mcs->call.args[i];
+ unsigned int i;
+ typeof(mcs->compat_call.args[0]) args[ARRAY_SIZE(mcs->call.args)];
+
+ for ( i = 0; i < ARRAY_SIZE(args); i++ )
+ args[i] = mcs->call.args[i];
+
+ memcpy(mcs->compat_call.args, args, sizeof(args));
}
#define multicall_entry compat_multicall_entry
@@ -29,8 +33,8 @@ static inline void xlat_multicall_entry(
static void __trace_multicall_call(multicall_entry_t *call)
{
- xen_ulong_t args[6];
- int i;
+ xen_ulong_t args[ARRAY_SIZE(call->args)];
+ unsigned int i;
for ( i = 0; i < ARRAY_SIZE(args); i++ )
args[i] = call->args[i];
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -640,7 +640,11 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
*/
struct multicall_entry {
xen_ulong_t op, result;
+#ifndef __XEN__
xen_ulong_t args[6];
+#else /* Only 5 arguments are supported in reality. */
+ xen_ulong_t args[5], unused;
+#endif
};
typedef struct multicall_entry multicall_entry_t;
DEFINE_XEN_GUEST_HANDLE(multicall_entry_t);On 29/04/2025 9:16 am, Jan Beulich wrote:
> Multicall compat translation and hypercall continuation handling can
> also be shrunk to the processing of just (up to) 5 arguments.
>
> Take the opportunity to
> - make exceeding the limit noisy in hypercall_create_continuation(),
> - use speculation-safe array access in hypercall_create_continuation(),
> - avoid a Misra C:2012 Rule 19.1 violation in xlat_multicall_entry(),
> - further tidy xlat_multicall_entry() and __trace_multicall_call()
> style-wise.
>
> Amends: 2f531c122e95 ("x86: limit number of hypercall parameters to 5")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> hypercall_xlat_continuation() uses BUG() when encountering too large an
> argument count in release builds, but I think that's too harsh. Hence in
> hypercall_create_continuation() I'm re-using the existing error path.
> Interestingly the multicall part of hypercall_xlat_continuation() has no
> check at all which would cover release builds.
This has never been the best of code. All we can do is improve it
moving forward.
>
> With gcc14 code size grows according to my observation, due to the loops
> in xlat_multicall_entry() and __trace_multicall_call() both being
> unrolled now.
With GCC-12, it gets marginally smaller. The loops are unrolled and the
instruction scheduling is rather odd.
mov 0x208(%r15),%rax
mov 0x228(%r15),%rdx
mov %eax,0x1c(%rsp)
mov 0x210(%r15),%rax
mov %edx,0x2c(%rsp)
mov %eax,0x20(%rsp)
mov 0x218(%r15),%rax
mov 0x1c(%rsp),%rsi
mov %edx,0x210(%r15)
mov %eax,0x24(%rsp)
mov 0x220(%r15),%rax
mov %rsi,0x200(%r15)
mov %eax,0x28(%rsp)
mov 0x24(%rsp),%rsi
mov %rsi,0x208(%r15)
This is the interleaved copy onto the stack, and back off.
Weirdly, we're doing 8 byte loads and 4 byte stores to copy on to the
stack. It's not wrong per say, but it's also not necessary.
Then, for the copy-off, there is an overlapping store:
mov %edx,0x210(%r15)
and
mov %rsi,0x208(%r15)
not to mention this:
mov %eax,0x28(%rsp)
mov 0x24(%rsp),%rsi
using an overlapping store and wider load for the copy-off.
Anyway - it looks to function correctly, but I don't exactly feel as if
the optimiser has done a great job.
~Andrew
On Tue, 28 Apr 2025, Andrew Cooper wrote:
> On 29/04/2025 9:16 am, Jan Beulich wrote:
> > Multicall compat translation and hypercall continuation handling can
> > also be shrunk to the processing of just (up to) 5 arguments.
> >
> > Take the opportunity to
> > - make exceeding the limit noisy in hypercall_create_continuation(),
> > - use speculation-safe array access in hypercall_create_continuation(),
> > - avoid a Misra C:2012 Rule 19.1 violation in xlat_multicall_entry(),
> > - further tidy xlat_multicall_entry() and __trace_multicall_call()
> > style-wise.
> >
> > Amends: 2f531c122e95 ("x86: limit number of hypercall parameters to 5")
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Arm side:
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
© 2016 - 2025 Red Hat, Inc.