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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 13.11.25 14:30, Jan Beulich wrote:
> On 11.11.2025 18:54, Grygorii Strashko wrote:
> > @@ -3991,7 +3995,7 @@ static void hvm_latch_shinfo_size(struct domain *d)
> >        */
> >       if ( current->domain == d )
> >       {
> > -        d->arch.has_32bit_shinfo =
> > +        d->arch.has_32bit_shinfo = IS_ENABLED(CONFIG_COMPAT) &&
> >               hvm_guest_x86_mode(current) != X86_MODE_64BIT;
> I think this could need commenting on if we really want to put it in this shape.
ok.

> But why would we retain the has_32bit_shinfo field in the first place when
> COMPAT=n?
Here I'm not sure. May be other can comment.

> > @@ -4965,6 +4969,7 @@ static int do_altp2m_op(
> >   #endif /* CONFIG_ALTP2M */
> >   }
> >   
> > +#ifdef CONFIG_COMPAT
> >   DEFINE_XEN_GUEST_HANDLE(compat_hvm_altp2m_op_t);
> >   
> >   /*
> > @@ -5064,6 +5069,12 @@ static int compat_altp2m_op(
> >   
> >       return rc;
> >   }
> > +#else
> > +static int compat_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
> > +{
> > +    return -EOPNOTSUPP;
> > +}
> > +#endif /* CONFIG_COMPAT */
> I'm not in favor of repeating the function "header". Imo such #ifdef-s better
> go inside respective functions' bodies.
ok.

> > --- 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.

> > @@ -171,6 +177,7 @@ int hvm_hypercall(struct cpu_user_regs *regs)
> >           HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%x, %x, %x, %x, %x)", eax,
> >                       regs->ebx, regs->ecx, regs->edx, regs->esi, regs->edi);
> >   
> > +#ifdef CONFIG_COMPAT
> >           curr->hcall_compat = true;
> >           call_handlers_hvm32(eax, regs->eax, regs->ebx, regs->ecx, regs->edx,
> >                               regs->esi, regs->edi);
> > @@ -178,6 +185,9 @@ int hvm_hypercall(struct cpu_user_regs *regs)
> >   
> >           if ( !curr->hcall_preempted && regs->eax != -ENOSYS )
> >               clobber_regs(regs, eax, hvm, 32);
> > +#else
> > +        regs->eax = -EOPNOTSUPP;
> > +#endif
> I'm generally against most attempts to use ENOSYS, but here it should be used:
> The top-level hypercalls are (effectively) unimplemented in this mode.
ok.

> > @@ -208,10 +218,19 @@ enum mc_disposition hvm_do_multicall_call(struct mc_state 
> > *state)
> >       }
> >       else
> >       {
> > +#ifdef CONFIG_COMPAT
> >           struct compat_multicall_entry *call = &state->compat_call;
> >   
> >           call_handlers_hvm32(call->op, call->result, call->args[0], call->args[1],
> >                               call->args[2], call->args[3], call->args[4]);
> > +#else
> > +        /*
> > +         * code should never reach here in case !CONFIG_COMPAT as any
> > +         * 32-bit hypercall should bail out earlier from hvm_hypercall()
> > +         * with -EOPNOTSUPP
> > +         */
> > +        unreachable();
> I.e. you rather mean ASSERT_UNREACHABLE()?
ok.

--
Best regards,
-grygorii