| Message ID | 20230103200943.5801-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix truncation of various XENVER_* subops | expand |
On 03.01.2023 21:09, Andrew Cooper wrote: > kernel.c is mostly in an #ifndef COMPAT guard, because compat/kernel.c > reincludes kernel.c to recompile xen_version() in a compat form. > > However, the xen_version hypercall is almost guest-ABI-agnostic; only > XENVER_platform_parameters has a compat split. Handle this locally, and do > away with the reinclude entirely. And we henceforth mean to not introduce any interface structures here which would require translation (or we're willing to accept the clutter of handling those "locally" as well). Fine with me, just wanting to mention it. > --- a/xen/common/compat/kernel.c > +++ /dev/null > @@ -1,53 +0,0 @@ > -/****************************************************************************** > - * kernel.c > - */ > - > -EMIT_FILE; > - > -#include <xen/init.h> > -#include <xen/lib.h> > -#include <xen/errno.h> > -#include <xen/version.h> > -#include <xen/sched.h> > -#include <xen/guest_access.h> > -#include <asm/current.h> > -#include <compat/xen.h> > -#include <compat/version.h> > - > -extern xen_commandline_t saved_cmdline; > - > -#define xen_extraversion compat_extraversion > -#define xen_extraversion_t compat_extraversion_t > - > -#define xen_compile_info compat_compile_info > -#define xen_compile_info_t compat_compile_info_t > - > -CHECK_TYPE(capabilities_info); This and ... > -#define xen_platform_parameters compat_platform_parameters > -#define xen_platform_parameters_t compat_platform_parameters_t > -#undef HYPERVISOR_VIRT_START > -#define HYPERVISOR_VIRT_START HYPERVISOR_COMPAT_VIRT_START(current->domain) > - > -#define xen_changeset_info compat_changeset_info > -#define xen_changeset_info_t compat_changeset_info_t > - > -#define xen_feature_info compat_feature_info > -#define xen_feature_info_t compat_feature_info_t > - > -CHECK_TYPE(domain_handle); ... this go away without any replacement. Considering they're both char[], that's probably fine, but could do with mentioning in the description. > @@ -520,12 +518,27 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > case XENVER_platform_parameters: > { > - xen_platform_parameters_t params = { > - .virt_start = HYPERVISOR_VIRT_START > - }; With this gone the oddly (but intentionally) placed braces can then also go away. Preferably with these minor adjustments Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 03.01.2023 21:09, Andrew Cooper wrote: > --- a/xen/include/xlat.lst > +++ b/xen/include/xlat.lst > @@ -169,6 +169,9 @@ > ! vcpu_runstate_info vcpu.h > ? vcpu_set_periodic_timer vcpu.h > ! vcpu_set_singleshot_timer vcpu.h > +? compile_info version.h > +? feature_info version.h > +? build_id version.h Oh, btw, another minor request: Can you please sort these by name (as secondary criteria after the name of the containing header)? Thanks, Jan
On 04/01/2023 4:29 pm, Jan Beulich wrote: > On 03.01.2023 21:09, Andrew Cooper wrote: >> kernel.c is mostly in an #ifndef COMPAT guard, because compat/kernel.c >> reincludes kernel.c to recompile xen_version() in a compat form. >> >> However, the xen_version hypercall is almost guest-ABI-agnostic; only >> XENVER_platform_parameters has a compat split. Handle this locally, and do >> away with the reinclude entirely. > And we henceforth mean to not introduce any interface structures here > which would require translation (or we're willing to accept the clutter > of handling those "locally" as well). Fine with me, just wanting to > mention it. Sure - I'll put a note in the commit message. In general, we don't want guest-variant interfaces. > >> --- a/xen/common/compat/kernel.c >> +++ /dev/null >> @@ -1,53 +0,0 @@ >> -/****************************************************************************** >> - * kernel.c >> - */ >> - >> -EMIT_FILE; >> - >> -#include <xen/init.h> >> -#include <xen/lib.h> >> -#include <xen/errno.h> >> -#include <xen/version.h> >> -#include <xen/sched.h> >> -#include <xen/guest_access.h> >> -#include <asm/current.h> >> -#include <compat/xen.h> >> -#include <compat/version.h> >> - >> -extern xen_commandline_t saved_cmdline; >> - >> -#define xen_extraversion compat_extraversion >> -#define xen_extraversion_t compat_extraversion_t >> - >> -#define xen_compile_info compat_compile_info >> -#define xen_compile_info_t compat_compile_info_t >> - >> -CHECK_TYPE(capabilities_info); > This and ... > >> -#define xen_platform_parameters compat_platform_parameters >> -#define xen_platform_parameters_t compat_platform_parameters_t >> -#undef HYPERVISOR_VIRT_START >> -#define HYPERVISOR_VIRT_START HYPERVISOR_COMPAT_VIRT_START(current->domain) >> - >> -#define xen_changeset_info compat_changeset_info >> -#define xen_changeset_info_t compat_changeset_info_t >> - >> -#define xen_feature_info compat_feature_info >> -#define xen_feature_info_t compat_feature_info_t >> - >> -CHECK_TYPE(domain_handle); > ... this go away without any replacement. Considering they're both > char[], that's probably fine, but could do with mentioning in the > description. I did actually mean to ask about these two, because they're incomplete already. Why do we CHECK_TYPE(capabilities_info) but define identity aliases for compat_extraversion (amongst others) ? Is there even a point for having a compat alias of a char array? I'm tempted to just drop them. I don't think the check does anything useful for us. >> @@ -520,12 +518,27 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> >> case XENVER_platform_parameters: >> { >> - xen_platform_parameters_t params = { >> - .virt_start = HYPERVISOR_VIRT_START >> - }; > With this gone the oddly (but intentionally) placed braces can then > also go away. In light of how patch 3 ended up, I was considering pulling curr out into a variable. > Preferably with these minor adjustments > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, ~Andrew
On 04.01.2023 20:15, Andrew Cooper wrote: > On 04/01/2023 4:29 pm, Jan Beulich wrote: >> On 03.01.2023 21:09, Andrew Cooper wrote: >>> --- a/xen/common/compat/kernel.c >>> +++ /dev/null >>> @@ -1,53 +0,0 @@ >>> -/****************************************************************************** >>> - * kernel.c >>> - */ >>> - >>> -EMIT_FILE; >>> - >>> -#include <xen/init.h> >>> -#include <xen/lib.h> >>> -#include <xen/errno.h> >>> -#include <xen/version.h> >>> -#include <xen/sched.h> >>> -#include <xen/guest_access.h> >>> -#include <asm/current.h> >>> -#include <compat/xen.h> >>> -#include <compat/version.h> >>> - >>> -extern xen_commandline_t saved_cmdline; >>> - >>> -#define xen_extraversion compat_extraversion >>> -#define xen_extraversion_t compat_extraversion_t >>> - >>> -#define xen_compile_info compat_compile_info >>> -#define xen_compile_info_t compat_compile_info_t >>> - >>> -CHECK_TYPE(capabilities_info); >> This and ... >> >>> -#define xen_platform_parameters compat_platform_parameters >>> -#define xen_platform_parameters_t compat_platform_parameters_t >>> -#undef HYPERVISOR_VIRT_START >>> -#define HYPERVISOR_VIRT_START HYPERVISOR_COMPAT_VIRT_START(current->domain) >>> - >>> -#define xen_changeset_info compat_changeset_info >>> -#define xen_changeset_info_t compat_changeset_info_t >>> - >>> -#define xen_feature_info compat_feature_info >>> -#define xen_feature_info_t compat_feature_info_t >>> - >>> -CHECK_TYPE(domain_handle); >> ... this go away without any replacement. Considering they're both >> char[], that's probably fine, but could do with mentioning in the >> description. > > I did actually mean to ask about these two, because they're incomplete > already. > > Why do we CHECK_TYPE(capabilities_info) but define identity aliases for > compat_extraversion (amongst others) ? > > Is there even a point for having a compat alias of a char array? To be quite honest, for capabilities_info I don't recall why it wasn't aliased. For domain_handle I think the reason was that it's declared outside of this header, and it could conceivably be a more UUID-like struct. > I'm tempted to just drop them. I don't think the check does anything > useful for us. As said, I'm pretty much okay with this, but would like you to mention in the description that this is an intentional change. Jan
diff --git a/xen/common/Makefile b/xen/common/Makefile index 9a3a12b12dea..bbd75b4be6d3 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -59,7 +59,7 @@ obj-y += xmalloc_tlsf.o obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd earlycpio,$(n).init.o) -obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o xlat.o) +obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o) ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) obj-y += domctl.o diff --git a/xen/common/compat/kernel.c b/xen/common/compat/kernel.c deleted file mode 100644 index 804b919bdc72..000000000000 --- a/xen/common/compat/kernel.c +++ /dev/null @@ -1,53 +0,0 @@ -/****************************************************************************** - * kernel.c - */ - -EMIT_FILE; - -#include <xen/init.h> -#include <xen/lib.h> -#include <xen/errno.h> -#include <xen/version.h> -#include <xen/sched.h> -#include <xen/guest_access.h> -#include <asm/current.h> -#include <compat/xen.h> -#include <compat/version.h> - -extern xen_commandline_t saved_cmdline; - -#define xen_extraversion compat_extraversion -#define xen_extraversion_t compat_extraversion_t - -#define xen_compile_info compat_compile_info -#define xen_compile_info_t compat_compile_info_t - -CHECK_TYPE(capabilities_info); - -#define xen_platform_parameters compat_platform_parameters -#define xen_platform_parameters_t compat_platform_parameters_t -#undef HYPERVISOR_VIRT_START -#define HYPERVISOR_VIRT_START HYPERVISOR_COMPAT_VIRT_START(current->domain) - -#define xen_changeset_info compat_changeset_info -#define xen_changeset_info_t compat_changeset_info_t - -#define xen_feature_info compat_feature_info -#define xen_feature_info_t compat_feature_info_t - -CHECK_TYPE(domain_handle); - -#define DO(fn) int compat_##fn -#define COMPAT - -#include "../kernel.c" - -/* - * Local variables: - * mode: C - * c-file-style: "BSD" - * c-basic-offset: 4 - * tab-width: 4 - * indent-tabs-mode: nil - * End: - */ diff --git a/xen/common/kernel.c b/xen/common/kernel.c index f8134d3e7a9d..ccee178ff17a 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -18,7 +18,13 @@ #include <asm/current.h> #include <public/version.h> -#ifndef COMPAT +#ifdef CONFIG_COMPAT +#include <compat/version.h> + +CHECK_compile_info; +CHECK_feature_info; +CHECK_build_id; +#endif enum system_state system_state = SYS_STATE_early_boot; @@ -463,15 +469,7 @@ static int __init cf_check param_init(void) __initcall(param_init); #endif -# define DO(fn) long do_##fn - -#endif - -/* - * Simple hypercalls. - */ - -DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) +long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { bool_t deny = !!xsm_xen_version(XSM_OTHER, cmd); @@ -520,12 +518,27 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case XENVER_platform_parameters: { - xen_platform_parameters_t params = { - .virt_start = HYPERVISOR_VIRT_START - }; +#ifdef CONFIG_COMPAT + if ( current->hcall_compat ) + { + compat_platform_parameters_t params = { + .virt_start = HYPERVISOR_COMPAT_VIRT_START(current->domain), + }; + + if ( copy_to_guest(arg, ¶ms, 1) ) + return -EFAULT; + } + else +#endif + { + xen_platform_parameters_t params = { + .virt_start = HYPERVISOR_VIRT_START, + }; + + if ( copy_to_guest(arg, ¶ms, 1) ) + return -EFAULT; + } - if ( copy_to_guest(arg, ¶ms, 1) ) - return -EFAULT; return 0; } diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c index 41e1af01f6b2..6d361ddfce1b 100644 --- a/xen/include/hypercall-defs.c +++ b/xen/include/hypercall-defs.c @@ -245,7 +245,7 @@ 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 +xen_version do do do do do console_io do do do do do physdev_op_compat compat do - - dep #if defined(CONFIG_GRANT_TABLE) diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst index 65f7fe3811c7..f2bae220a6df 100644 --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -169,6 +169,9 @@ ! vcpu_runstate_info vcpu.h ? vcpu_set_periodic_timer vcpu.h ! vcpu_set_singleshot_timer vcpu.h +? compile_info version.h +? feature_info version.h +? build_id version.h ? xenoprof_init xenoprof.h ? xenoprof_passive xenoprof.h ? flask_access xsm/flask_op.h
kernel.c is mostly in an #ifndef COMPAT guard, because compat/kernel.c reincludes kernel.c to recompile xen_version() in a compat form. However, the xen_version hypercall is almost guest-ABI-agnostic; only XENVER_platform_parameters has a compat split. Handle this locally, and do away with the reinclude entirely. In particular, this removed the final instances of obfucation via the DO() macro. No functional change. Also saves 2k of of .text in the x86 build. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> --- xen/common/Makefile | 2 +- xen/common/compat/kernel.c | 53 -------------------------------------------- xen/common/kernel.c | 43 ++++++++++++++++++++++------------- xen/include/hypercall-defs.c | 2 +- xen/include/xlat.lst | 3 +++ 5 files changed, 33 insertions(+), 70 deletions(-) delete mode 100644 xen/common/compat/kernel.c