Re: [PATCH 07/11] KVM: x86: Add mode-aware versions of kvm_<reg>_{read,write}() helpers

[Huang, Kai]

On Tue, 2026-04-14 at 08:42 -0700, Sean Christopherson wrote:
> On Tue, Apr 14, 2026, Kai Huang wrote:
> > 
> > > --- a/arch/x86/kvm/svm/nested.c
> > > +++ b/arch/x86/kvm/svm/nested.c
> > > @@ -757,7 +757,7 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm)
> > >  
> > >  	svm->vcpu.arch.cr2 = save->cr2;
> > >  
> > > -	kvm_rax_write(vcpu, save->rax);
> > > +	kvm_rax_write_raw(vcpu, save->rax);
> > >  	kvm_rsp_write(vcpu, save->rsp);
> > >  	kvm_rip_write(vcpu, save->rip);
> > >  
> > > @@ -1238,7 +1238,7 @@ static int nested_svm_vmexit_update_vmcb12(struct kvm_vcpu *vcpu)
> > >  	vmcb12->save.rflags = kvm_get_rflags(vcpu);
> > >  	vmcb12->save.rip    = kvm_rip_read(vcpu);
> > >  	vmcb12->save.rsp    = kvm_rsp_read(vcpu);
> > > -	vmcb12->save.rax    = kvm_rax_read(vcpu);
> > > +	vmcb12->save.rax    = kvm_rax_read_raw(vcpu);
> > 
> > Not sure whether it matters, I think there's an inconsistency here:
> > 
> > The "rax" one has "raw" postfix, but "rsp" doesn't, despite in practice it
> > is also a "raw" operation.  Ditto for "rip", although it will be moved out
> > of the "regs[]" GPR array.
> 
> Oh, there's very much an inconsistency.  RIP probably "fine", as it should be
> impossible to get a 64-bit RIP into the CPU when it's not in 64-bit mode.  RSP
> is likely not "fine", i.e. should probably use a "raw" version.
> 
> But most importantly, for this patch, I want to avoid introducing functional
> changes, which means using the "raw" variant to read RAX.

Yeah make sense.

> 
> > But maybe they are different?
> > 
> > [...]
> > 
> > >  	case EXIT_REASON_MSR_WRITE:
> > > -		kvm_rcx_write(vcpu, tdx->vp_enter_args.r12);
> > > -		kvm_rax_write(vcpu, tdx->vp_enter_args.r13 & -1u);
> > > -		kvm_rdx_write(vcpu, tdx->vp_enter_args.r13 >> 32);
> > > +		kvm_ecx_write(vcpu, tdx->vp_enter_args.r12);
> > > +		kvm_eax_write(vcpu, tdx->vp_enter_args.r13 & -1u);
> > 
> > Nit:  the "& -1u" isn't needed anymore with using kvm_eax_write(), but maybe
> > we should just focus on replacing the functions in this patch but leave
> > cleanup in the future.
> 
> Gah, good eyeballs.  I intended to drop it here.

If you want to drop it in this patch, then there's another one in
__kvm_emulate_rdmsr():

@@ -2140,8 +2140,8 @@ static int __kvm_emulate_rdmsr(struct kvm_vcpu *vcpu,
u32 msr, int reg,
 		trace_kvm_msr_read(msr, data);
 
 		if (reg < 0) {
-			kvm_rax_write(vcpu, data & -1u);
-			kvm_rdx_write(vcpu, (data >> 32) & -1u);
+			kvm_eax_write(vcpu, data & -1u);
+			kvm_edx_write(vcpu, (data >> 32) & -1u);
 		} else {
 			kvm_register_write(vcpu, reg, data);
 		}
> 
> > [...]
> > 
> > 
> > > @@ -12184,23 +12185,23 @@ static void __set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> > >  	vcpu->arch.emulate_regs_need_sync_from_vcpu = true;
> > >  	vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
> > >  
> > > -	kvm_rax_write(vcpu, regs->rax);
> > > -	kvm_rbx_write(vcpu, regs->rbx);
> > > -	kvm_rcx_write(vcpu, regs->rcx);
> > > -	kvm_rdx_write(vcpu, regs->rdx);
> > > -	kvm_rsi_write(vcpu, regs->rsi);
> > > -	kvm_rdi_write(vcpu, regs->rdi);
> > > +	kvm_rax_write_raw(vcpu, regs->rax);
> > > +	kvm_rbx_write_raw(vcpu, regs->rbx);
> > > +	kvm_rcx_write_raw(vcpu, regs->rcx);
> > > +	kvm_rdx_write_raw(vcpu, regs->rdx);
> > > +	kvm_rsi_write_raw(vcpu, regs->rsi);
> > > +	kvm_rdi_write_raw(vcpu, regs->rdi);
> > >  	kvm_rsp_write(vcpu, regs->rsp);
> > > -	kvm_rbp_write(vcpu, regs->rbp);
> > > +	kvm_rbp_write_raw(vcpu, regs->rbp);
> > > 
> > 
> > Ditto, the "rsp" one stands out. :-)
> 
> Yeah, same thing as above.  I don't think the currently code is 100% correct, but
> in practice it probably doesn't matter.
> 
> If we want to clean up RSP handling, it should definitely be done in a separate
> patch (or patches, plural).  But I'm hesitant to even try, especially for this
> path since it's very much part of KVM's ABI.  I.e. if ain't broke, don't fix it.

Right.

I was thinking maybe just rename the RSP version to have the "raw" postfix
as well, just for consistency, but no other functional changes.