Re: [PATCH 4/4] x86/svm: Drop emulation of Intel's SYSENTER behaviour

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Thu Jan 22, 2026 at 6:52 PM CET, Teddy Astie wrote:
> Le 22/01/2026 à 17:51, Alejandro Vallejo a écrit :
>> With cross-vendor support gone, it's no longer needed.
>> 
>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
>> ---
>>   xen/arch/x86/hvm/svm/svm.c               | 42 +++++++++++-------------
>>   xen/arch/x86/hvm/svm/vmcb.c              |  3 ++
>>   xen/arch/x86/include/asm/hvm/svm-types.h | 10 ------
>>   3 files changed, 22 insertions(+), 33 deletions(-)
>> 
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index 0658ca990f..e8f19dec04 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -401,10 +401,6 @@ static int svm_vmcb_save(struct vcpu *v, struct 
>> hvm_hw_cpu *c)
>>   {
>>       struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>>   
>> -    c->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs;
>> -    c->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp;
>> -    c->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip;
>> -
>>       if ( vmcb->event_inj.v &&
>>            hvm_event_needs_reinjection(vmcb->event_inj.type,
>>                                        vmcb->event_inj.vector) )
>> @@ -468,11 +464,6 @@ static int svm_vmcb_restore(struct vcpu *v, struct 
>> hvm_hw_cpu *c)
>>       svm_update_guest_cr(v, 0, 0);
>>       svm_update_guest_cr(v, 4, 0);
>>   
>> -    /* Load sysenter MSRs into both VMCB save area and VCPU fields. */
>> -    vmcb->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs = c->sysenter_cs;
>> -    vmcb->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp = 
>> c->sysenter_esp;
>> -    vmcb->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip = 
>> c->sysenter_eip;
>> -
>>       if ( paging_mode_hap(v->domain) )
>>       {
>>           vmcb_set_np(vmcb, true);
>> @@ -501,6 +492,9 @@ static void svm_save_cpu_state(struct vcpu *v, struct 
>> hvm_hw_cpu *data)
>>   {
>>       struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>>   
>> +    data->sysenter_cs      = vmcb->sysenter_cs;
>> +    data->sysenter_esp     = vmcb->sysenter_esp;
>> +    data->sysenter_eip     = vmcb->sysenter_eip;
>>       data->shadow_gs        = vmcb->kerngsbase;
>>       data->msr_lstar        = vmcb->lstar;
>>       data->msr_star         = vmcb->star;
>> @@ -512,11 +506,14 @@ static void svm_load_cpu_state(struct vcpu *v, struct 
>> hvm_hw_cpu *data)
>>   {
>>       struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>>   
>> -    vmcb->kerngsbase = data->shadow_gs;
>> -    vmcb->lstar      = data->msr_lstar;
>> -    vmcb->star       = data->msr_star;
>> -    vmcb->cstar      = data->msr_cstar;
>> -    vmcb->sfmask     = data->msr_syscall_mask;
>> +    vmcb->sysenter_cs  = data->sysenter_cs;
>> +    vmcb->sysenter_esp = data->sysenter_esp;
>> +    vmcb->sysenter_eip = data->sysenter_eip;
>> +    vmcb->kerngsbase   = data->shadow_gs;
>> +    vmcb->lstar        = data->msr_lstar;
>> +    vmcb->star         = data->msr_star;
>> +    vmcb->cstar        = data->msr_cstar;
>> +    vmcb->sfmask       = data->msr_syscall_mask;
>>       v->arch.hvm.guest_efer = data->msr_efer;
>>       svm_update_guest_efer(v);
>>   }
>
> I think we should merge svm_save_cpu_state/svm_vmcb_save into 
> svm_save_vmcb_ctxt and similarly for svm_load_cpu_state/svm_vmcb_restore 
> into svm_load_vmcb_ctxt to avoid having multiple functions as a part of 
> the same logic.
>
> That could be done in a separate patch (or series).

Maybe. I (subjectively) think it makes sense to keep separate the fields coming
straight from the VMCB from those that have authoritative values elsewhere.

Nothing precludes having that visual separation within a single function though
it's not like either is huge.

I may append it as a patch at the tail.

>
>> @@ -1720,12 +1717,9 @@ static int cf_check svm_msr_read_intercept(
>>   
>>       switch ( msr )
>>       {
>> -        /*
>> -         * Sync not needed while the cross-vendor logic is in unilateral 
>> effect.
>>       case MSR_IA32_SYSENTER_CS:
>>       case MSR_IA32_SYSENTER_ESP:
>>       case MSR_IA32_SYSENTER_EIP:
>> -         */
>>       case MSR_STAR:
>>       case MSR_LSTAR:
>>       case MSR_CSTAR:
>> @@ -1740,13 +1734,15 @@ static int cf_check svm_msr_read_intercept(
>>       switch ( msr )
>>       {
>>       case MSR_IA32_SYSENTER_CS:
>> -        *msr_content = v->arch.hvm.svm.guest_sysenter_cs;
>> +        *msr_content = vmcb->sysenter_cs;
>>           break;
>> +
>>       case MSR_IA32_SYSENTER_ESP:
>> -        *msr_content = v->arch.hvm.svm.guest_sysenter_esp;
>> +        *msr_content = vmcb->sysenter_esp;
>>           break;
>> +
>>       case MSR_IA32_SYSENTER_EIP:
>> -        *msr_content = v->arch.hvm.svm.guest_sysenter_eip;
>> +        *msr_content = vmcb->sysenter_eip;
>>           break;
>>   
>>       case MSR_STAR:
>> @@ -1940,11 +1936,11 @@ static int cf_check svm_msr_write_intercept(
>>           switch ( msr )
>>           {
>>           case MSR_IA32_SYSENTER_ESP:
>> -            vmcb->sysenter_esp = v->arch.hvm.svm.guest_sysenter_esp = 
>> msr_content;
>> +            vmcb->sysenter_esp = msr_content;
>>               break;
>>   
>>           case MSR_IA32_SYSENTER_EIP:
>> -            vmcb->sysenter_eip = v->arch.hvm.svm.guest_sysenter_eip = 
>> msr_content;
>> +            vmcb->sysenter_eip = msr_content;
>>               break;
>
>>   
>>           case MSR_LSTAR:
>> @@ -1970,7 +1966,7 @@ static int cf_check svm_msr_write_intercept(
>>           break;
>>   
>>       case MSR_IA32_SYSENTER_CS:
>> -        vmcb->sysenter_cs = v->arch.hvm.svm.guest_sysenter_cs = msr_content;
>> +        vmcb->sysenter_cs = msr_content;
>>           break;
>>   
>>       case MSR_STAR:
>> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
>> index e583ef8548..76fcaf15c2 100644
>> --- a/xen/arch/x86/hvm/svm/vmcb.c
>> +++ b/xen/arch/x86/hvm/svm/vmcb.c
>> @@ -97,6 +97,9 @@ static int construct_vmcb(struct vcpu *v)
>>       svm_disable_intercept_for_msr(v, MSR_LSTAR);
>>       svm_disable_intercept_for_msr(v, MSR_STAR);
>>       svm_disable_intercept_for_msr(v, MSR_SYSCALL_MASK);
>> +    svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_CS);
>> +    svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_EIP);
>> +    svm_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_ESP);
>>   
>>       vmcb->_msrpm_base_pa = virt_to_maddr(svm->msrpm);
>>       vmcb->_iopm_base_pa = __pa(v->domain->arch.hvm.io_bitmap);
>> diff --git a/xen/arch/x86/include/asm/hvm/svm-types.h 
>> b/xen/arch/x86/include/asm/hvm/svm-types.h
>> index 051b235d8f..aaee91b4b6 100644
>> --- a/xen/arch/x86/include/asm/hvm/svm-types.h
>> +++ b/xen/arch/x86/include/asm/hvm/svm-types.h
>> @@ -27,16 +27,6 @@ struct svm_vcpu {
>>   
>>       /* VMCB has a cached instruction from #PF/#NPF Decode Assist? */
>>       uint8_t cached_insn_len; /* Zero if no cached instruction. */
>> -
>> -    /*
>> -     * Upper four bytes are undefined in the VMCB, therefore we can't use 
>> the
>> -     * fields in the VMCB. Write a 64bit value and then read a 64bit value 
>> is
>> -     * fine unless there's a VMRUN/VMEXIT in between which clears the upper
>> -     * four bytes.
>> -     */
>> -    uint64_t guest_sysenter_cs;
>> -    uint64_t guest_sysenter_esp;
>> -    uint64_t guest_sysenter_eip;
>>   };
>>   
>>   struct nestedsvm {
>
> Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

Cheers,
Alejandro