Re: [RFC v1 6/7] x86/svm: Use the emulator path for VMEXIT_HLT

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Hello,

Here is my feedback from a SEV-ES implementation perspective, as it has 
specific needs related to some VMEXIT handling.
Le 18/05/2026 à 15:14, Ross Lagerwall a écrit :
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> ---
>   xen/arch/x86/hvm/emulate.c     |  5 +++++
>   xen/arch/x86/hvm/svm/emulate.c |  2 +-
>   xen/arch/x86/hvm/svm/svm.c     | 24 +++++++++++-------------
>   xen/arch/x86/hvm/svm/svm.h     |  1 +
>   4 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index c9553cd28238..471c032c1e9c 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2800,6 +2800,11 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
> *hvmemul_ctxt,
>   
>           switch ( hvmemul_ctxt->insn )
>           {
> +        case INSTR_HLT:
> +            hvmemul_ctxt->ctxt.retire.hlt = true;
> +            rc = X86EMUL_OKAY;
> +            break;
> +
>           default:
>               ASSERT_UNREACHABLE();
>               rc = X86EMUL_UNHANDLEABLE;
> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
> index 1dd830a31bd7..31f3cd88a858 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -16,7 +16,7 @@
>   #include "svm.h"
>   #include "vmcb.h"
>   
> -static unsigned long svm_nextrip_insn_length(struct vcpu *v)
> +unsigned long svm_nextrip_insn_length(struct vcpu *v)
>   {
>       struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>   
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index f49d2ebbfdd5..2d6022d6238c 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2051,18 +2051,6 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
>           hvm_inject_hw_exception(X86_EXC_GP, 0);
>   }
>   
> -static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb,
> -                              struct cpu_user_regs *regs)
> -{
> -    unsigned int inst_len;
> -
> -    if ( (inst_len = svm_get_insn_len(current, INSTR_HLT)) == 0 )
> -        return;
> -    __update_guest_eip(regs, inst_len);
> -
> -    hvm_hlt(regs->eflags);
> -}
> -
>   static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
>   {
>       struct vcpu *curr = current;
> @@ -2363,6 +2351,13 @@ static void cf_check svm_set_reg(struct vcpu *v, 
> unsigned int reg, uint64_t val)
>       }
>   }
>   
> +static void svm_emulate_one(struct hvm_emulate_ctxt *ctxt)
> +{
> +    ctxt->insn_len = svm_nextrip_insn_length(current);
> +    if ( !hvm_emulate_one_ctxt(ctxt) )
> +        hvm_inject_hw_exception(X86_EXC_GP, 0);
> +}
> +
>   static struct hvm_function_table __initdata_cf_clobber svm_function_table = {
>       .name                 = "SVM",
>       .cpu_up_prepare       = svm_cpu_up_prepare,
> @@ -2496,6 +2491,7 @@ void asmlinkage svm_vmexit_handler(void)
>       vintr_t intr;
>       bool vcpu_guestmode = false;
>       struct vlapic *vlapic = vcpu_vlapic(v);
> +    struct hvm_emulate_ctxt ctxt;
>   
>       regs->rax = vmcb->rax;
>       regs->rip = vmcb->rip;
> @@ -2840,7 +2836,9 @@ void asmlinkage svm_vmexit_handler(void)
>       }
>   
>       case VMEXIT_HLT:
> -        svm_vmexit_do_hlt(vmcb, regs);
> +        hvm_emulate_init_once(&ctxt, NULL, NULL, regs);
> +        ctxt.insn = INSTR_HLT;
> +        svm_emulate_one(&ctxt);
>           break;

In this case, most of the logic is hidden behind svm_emulate_one(); 
however, SEV-ES changes some aspects of the VMEXIT_HLT behavior (this is 
the same for e.g VMEXIT_PAUSE).
With SEV-ES, we can't access the CPU registers anymore but hlt can still 
be intercepted (it's a "Automatic Exit (AE)"), in this case, the CPU 
increases RIP itself (this is documented in SEV-ES section of the APM), 
and we just have to emulate the HLT behavior. How would that specific 
behavior fit in this new design ?
We can skip increasing rip in this specific case, but it's now common code.

>   
>       case VMEXIT_IOIO:
> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
> index f75bca7c5f66..9422dbd38a78 100644
> --- a/xen/arch/x86/hvm/svm/svm.h
> +++ b/xen/arch/x86/hvm/svm/svm.h
> @@ -36,6 +36,7 @@ static inline void svm_invlpga(unsigned long linear, uint32_t 
> asid)
>       asm volatile ( "invlpga" :: "a" (linear), "c" (asid) );
>   }
>   
> +unsigned long svm_nextrip_insn_length(struct vcpu *v);
>   unsigned int svm_get_insn_len(struct vcpu *v, unsigned int instr_enc);
>   unsigned int svm_get_task_switch_insn_len(void);
>   
Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature