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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.
But why would we retain the has_32bit_shinfo field in the first place when
COMPAT=n?

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

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

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

> @@ -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()?

Jan