Re: [XEN][PATCH 3/5] x86: hvm: factor out compat code under ifdefs

[Jürgen Groß <jgross@xxxxxxxx>]

On 13.11.25 16:32, Grygorii Strashko wrote:
> Hi Jürgen,
>
> On 13.11.25 16:46, Juergen Gross wrote:
> > On 13.11.25 15:39, Jürgen Groß wrote:
> > > On 13.11.25 14:23, Jan Beulich wrote:
> > > > On 13.11.2025 14:18, Grygorii Strashko wrote:
> > > > > On 13.11.25 14:30, Jan Beulich wrote:
> > > > > > On 11.11.2025 18:54, Grygorii Strashko wrote:
> > > > > > > --- a/xen/arch/x86/hvm/hypercall.c
> > > > > > > +++ b/xen/arch/x86/hvm/hypercall.c
> > > > > > > @@ -29,10 +29,12 @@ long hvm_memory_op(unsigned long cmd, 
> > > > > > > XEN_GUEST_HANDLE_PARAM(void) arg)
> > > > > > >            return -ENOSYS;
> > > > > > >        }
> > > > > > > -    if ( !vcpu_is_hcall_compat(current) )
> > > > > > > -        rc = do_memory_op(cmd, arg);
> > > > > > > -    else
> > > > > > > +#ifdef CONFIG_COMPAT
> > > > > > > +    if ( vcpu_is_hcall_compat(current) )
> > > > > > >            rc = compat_memory_op(cmd, arg);
> > > > > > > +    else
> > > > > > > +#endif
> > > > > > > +        rc = do_memory_op(cmd, arg);
> > > > > > Why would this be needed when vcpu_is_hcall_compat() already honors 
> > > > > > CONFIG_COMPAT?
> > > > > > (Same question then applies elsewhere, of course.)
> > > > > This I do not like by myself, but I was not able to find other options :(
> > > > >
> > > > > hypercall-defs.h is autogenerated and it's the only one place where functions
> > > > > declarations like do_x_op() are appeared or disappeared.
> > > > > So build is failing without ifdefs as compiler can't find compat_memory_op()
> > > > > declaration.
> > > > Oh, I see; I hadn't thought of that aspect. I wonder if we wouldn't better take
> > > > care of that in the machinery there. Cc-ing Jürgen, who did introduce this
> > > > originally. Maybe he has concrete arguments against us doing so.
> > > No arguments against it.
> > >
> > > You probably will need a new Prefix defined (e.g. compat_always) and set it via
> > >
> > > #define PREFIX_compat_always compat
> > >
> > > unconditionally. Then it should be possible to have
> > >
> > > Prefix: compat_always
> > > memory_op(...)
> > >
> > > outside of #ifdefs and drop the memory_op() in the #ifdef CONFIG_COMPAT section.
> > Oh, this might be wrong, as this will break the PV32 memory_op() hypercall.
> >
> > You need to keep the current memory_op() in the #ifdef CONFIG_COMPAT section
> > and move the compat_always stuff into an #else part of the CONFIG_COMPAT.
> >
> > > This should result in the compat_memory_op() prototype to be always available.
> > > Having no related function should be no problem due to DCO in case CONFIG_COMPAT
> > > isn't defined.
> Smth like this, right?
>
> diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
> index 338d7afe3048..e85943320bd2 100644
> --- a/xen/include/hypercall-defs.c
> +++ b/xen/include/hypercall-defs.c
> @@ -80,6 +80,8 @@ rettype: compat int
>   #define PREFIX_compat
>   #endif
>
> +#define PREFIX_compat_always compat
> +
>   #ifdef CONFIG_ARM
>   #define PREFIX_dep dep
>   #define PREFIX_do_arm do_arm
> @@ -156,6 +158,9 @@ platform_op(compat_platform_op_t *u_xenpf_op)
>   #ifdef CONFIG_KEXEC
>   kexec_op(unsigned int op, void *uarg)
>   #endif
> +#else
> +prefix: PREFIX_compat_always
This should be:

+prefix: compat_always

> +memory_op(unsigned int cmd, void *arg)
>   #endif /* CONFIG_COMPAT */

Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature