Re: [PATCH 2/4] xen/version: Drop compat/kernel.c

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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@xxxxxxxx>

Thanks,

~Andrew