Re: [PATCH v2 22/23] x86/pv: System call handling in FRED mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2025 17:04, Andrew Cooper wrote:
> Under FRED, entry_from_pv() handles everything, even system calls.  This means
> more of our logic is written in C now, rather than assembly.
> 
> In order to facilitate this, introduce pv_inject_callback(), which reuses
> struct trap_bounce infrastructure to inject the syscall/sysenter callbacks.
> This in turns requires some !PV compatibility for pv_inject_callback() and
> pv_hypercall() which can both be ASSERT_UNREACHABLE().
> 
> For each of INT $N, SYSCALL and SYSENTER, FRED gives us interrupted context
> which was previously lost.  As the guest can't see FRED, Xen has to lose state
> in the same way to maintain the prior behaviour.

In principle we could expose a new capability to the guest allowing it to
request that we preserve state. Question of course is whether that would
be of any practical use.

> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -712,11 +712,16 @@ int arch_set_info_hvm_guest(struct vcpu *v, const 
> struct vcpu_hvm_context *ctx);
>  
>  #ifdef CONFIG_PV
>  void pv_inject_event(const struct x86_event *event);
> +void pv_inject_callback(unsigned int type);
>  #else
>  static inline void pv_inject_event(const struct x86_event *event)
>  {
>      ASSERT_UNREACHABLE();
>  }
> +static inline void pv_inject_callback(unsigned int type)
> +{
> +    ASSERT_UNREACHABLE();
> +}
>  #endif

We don't really need this, nor ...

> --- a/xen/arch/x86/include/asm/hypercall.h
> +++ b/xen/arch/x86/include/asm/hypercall.h
> @@ -20,6 +20,11 @@
>  
>  #ifdef CONFIG_PV
>  void pv_hypercall(struct cpu_user_regs *regs);
> +#else
> +static inline void pv_hypercall(struct cpu_user_regs *regs)
> +{
> +    ASSERT_UNREACHABLE();
> +}
>  #endif

... this, do we? If you expose the decls outside of the #ifdef, I can't help
the impression that all call sites will simply be DCE-ed (thanks to the
!IS_ENABLED(CONFIG_PV) check at the top of entry_from_pv()).

> --- a/xen/arch/x86/pv/traps.c
> +++ b/xen/arch/x86/pv/traps.c
> @@ -19,6 +19,8 @@
>  #include <asm/shared.h>
>  #include <asm/traps.h>
>  
> +#include <public/callback.h>
> +
>  void pv_inject_event(const struct x86_event *event)
>  {
>      struct vcpu *curr = current;
> @@ -95,6 +97,37 @@ void pv_inject_event(const struct x86_event *event)
>      }
>  }
>  
> +void pv_inject_callback(unsigned int type)
> +{
> +    struct vcpu *curr = current;
> +    struct trap_bounce *tb = &curr->arch.pv.trap_bounce;
> +    unsigned long rip = 0;
> +    bool irq = false;

Move the latter two initializers into a default: case, after an
ASSERT_UNREACHABLE()?

> +    ASSERT(is_pv_64bit_vcpu(curr));

I was first wondering why you check this here, but yes, PV32 is disabled when
FRED is enabled. IOW if a new use for this function turned up, this could
validly be relaxed.

> @@ -2305,6 +2309,27 @@ void asmlinkage entry_from_pv(struct cpu_user_regs 
> *regs)
>  
>      switch ( type )
>      {
> +    case X86_ET_SW_INT:
> +        /*
> +         * INT $3/4 are indistinguishable from INT3/INTO under IDT, and are

Didn't we discuss this the other day? They are distinguishable, as long as you
set their gates' DPL to 0. Just that we use DPL 3. Hence I think this wants
wording a little differently, to make clear it's our (possibly wrong) choice.

> +         * permitted by Xen without the guest kernel having a choice.  Let

Doesn't the guest have a choice by using TI_SET_DPL() suitably?

> +         * them fall through into X86_ET_HW_EXC, as #BP in particular needs
> +         * handling by do_int3() in case an external debugger is attached.
> +         */

I don't understand this, though. An external debugger would better not place
breakpoints using CD 03, so I think we'd better wire such the normal INT nn
way. And for #OF I also don't think we need to make an exception.

Jan