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
