| Message ID | 20210108144123.58546-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/hypercall: fix gnttab hypercall args conditional build on pvshim | expand |
On 08.01.2021 15:41, Roger Pau Monne wrote: > --- a/xen/arch/x86/hypercall.c > +++ b/xen/arch/x86/hypercall.c > @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] = > ARGS(xen_version, 2), > ARGS(console_io, 3), > ARGS(physdev_op_compat, 1), > -#ifdef CONFIG_GRANT_TABLE > +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM) > ARGS(grant_table_op, 3), > #endif > ARGS(vm_assist, 2), This is correct when a shim-enabled build runs as shim, but not when it runs as normal hypervisor. Just like the hypercall handler gets patched into the hypercall table (in pv_shim_setup_dom()), the argument count will also want updating there, I think. Jan
On Fri, Jan 08, 2021 at 04:01:52PM +0100, Jan Beulich wrote: > On 08.01.2021 15:41, Roger Pau Monne wrote: > > --- a/xen/arch/x86/hypercall.c > > +++ b/xen/arch/x86/hypercall.c > > @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] = > > ARGS(xen_version, 2), > > ARGS(console_io, 3), > > ARGS(physdev_op_compat, 1), > > -#ifdef CONFIG_GRANT_TABLE > > +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM) > > ARGS(grant_table_op, 3), > > #endif > > ARGS(vm_assist, 2), > > This is correct when a shim-enabled build runs as shim, but > not when it runs as normal hypervisor. Just like the hypercall > handler gets patched into the hypercall table (in > pv_shim_setup_dom()), the argument count will also want > updating there, I think. Having the argument count set when the hypercall handler is NULL is fine, as Xen won't get into processing hypercall_args_table if the handler is NULL. While it's true that can be fixed at run time in the same way that we patch the handler it seems more cumbersome, that's why I went for this IMO simpler fix. Thanks, Roger.
On 08.01.2021 16:11, Roger Pau Monné wrote: > On Fri, Jan 08, 2021 at 04:01:52PM +0100, Jan Beulich wrote: >> On 08.01.2021 15:41, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/hypercall.c >>> +++ b/xen/arch/x86/hypercall.c >>> @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] = >>> ARGS(xen_version, 2), >>> ARGS(console_io, 3), >>> ARGS(physdev_op_compat, 1), >>> -#ifdef CONFIG_GRANT_TABLE >>> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM) >>> ARGS(grant_table_op, 3), >>> #endif >>> ARGS(vm_assist, 2), >> >> This is correct when a shim-enabled build runs as shim, but >> not when it runs as normal hypervisor. Just like the hypercall >> handler gets patched into the hypercall table (in >> pv_shim_setup_dom()), the argument count will also want >> updating there, I think. > > Having the argument count set when the hypercall handler is NULL is > fine, as Xen won't get into processing hypercall_args_table if the > handler is NULL. Oh, good point. > While it's true that can be fixed at run time in the > same way that we patch the handler it seems more cumbersome, that's > why I went for this IMO simpler fix. Agreed: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 08.01.2021 16:24, Jan Beulich wrote: > On 08.01.2021 16:11, Roger Pau Monné wrote: >> On Fri, Jan 08, 2021 at 04:01:52PM +0100, Jan Beulich wrote: >>> On 08.01.2021 15:41, Roger Pau Monne wrote: >>>> --- a/xen/arch/x86/hypercall.c >>>> +++ b/xen/arch/x86/hypercall.c >>>> @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] = >>>> ARGS(xen_version, 2), >>>> ARGS(console_io, 3), >>>> ARGS(physdev_op_compat, 1), >>>> -#ifdef CONFIG_GRANT_TABLE >>>> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM) >>>> ARGS(grant_table_op, 3), >>>> #endif >>>> ARGS(vm_assist, 2), >>> >>> This is correct when a shim-enabled build runs as shim, but >>> not when it runs as normal hypervisor. Just like the hypercall >>> handler gets patched into the hypercall table (in >>> pv_shim_setup_dom()), the argument count will also want >>> updating there, I think. >> >> Having the argument count set when the hypercall handler is NULL is >> fine, as Xen won't get into processing hypercall_args_table if the >> handler is NULL. > > Oh, good point. Albeit then - why not drop the #ifdef altogether? Jan
On Fri, Jan 08, 2021 at 04:24:01PM +0100, Jan Beulich wrote: > On 08.01.2021 16:11, Roger Pau Monné wrote: > > On Fri, Jan 08, 2021 at 04:01:52PM +0100, Jan Beulich wrote: > >> On 08.01.2021 15:41, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/hypercall.c > >>> +++ b/xen/arch/x86/hypercall.c > >>> @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] = > >>> ARGS(xen_version, 2), > >>> ARGS(console_io, 3), > >>> ARGS(physdev_op_compat, 1), > >>> -#ifdef CONFIG_GRANT_TABLE > >>> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM) > >>> ARGS(grant_table_op, 3), > >>> #endif > >>> ARGS(vm_assist, 2), > >> > >> This is correct when a shim-enabled build runs as shim, but > >> not when it runs as normal hypervisor. Just like the hypercall > >> handler gets patched into the hypercall table (in > >> pv_shim_setup_dom()), the argument count will also want > >> updating there, I think. > > > > Having the argument count set when the hypercall handler is NULL is > > fine, as Xen won't get into processing hypercall_args_table if the > > handler is NULL. > > Oh, good point. > > > While it's true that can be fixed at run time in the > > same way that we patch the handler it seems more cumbersome, that's > > why I went for this IMO simpler fix. > > Agreed: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. In fact I'm not sure it's helpful to have any build time conditionals in hypercall_args_table, but let's leave that for another fix. Roger.
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c index dd00983005..8d18ef80cc 100644 --- a/xen/arch/x86/hypercall.c +++ b/xen/arch/x86/hypercall.c @@ -47,7 +47,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] = ARGS(xen_version, 2), ARGS(console_io, 3), ARGS(physdev_op_compat, 1), -#ifdef CONFIG_GRANT_TABLE +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM) ARGS(grant_table_op, 3), #endif ARGS(vm_assist, 2),
A pvshim build doesn't require the grant table functionality built in, but it does require knowing the number of arguments the hypercall has so the hypercall parameter clobbering works properly. Note this hasn't been detected by osstest because the tools pvshim build is done without debug enabled, so the hypercall parameter clobbering doesn't happen. Fixes: d2151152dd2 ('xen: make grant table support configurable') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hypercall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)