svm: save_svm_cpu_user_regs vs. svm_store_cpu_guest_regs

svm: save_svm_cpu_user_regs vs. svm_store_cpu_guest_regs

[Jan Beulich]

What is the reason for save_svm_cpu_user_regs() explicitly saving rax?
Without this, the function could be eliminated, with its single use replaced by
a call to svm_store_cpu_guest_regs().

Thanks, Jan

Re: svm: save_svm_cpu_user_regs vs. svm_store_cpu_guest_regs

[Keir Fraser]

On 9/5/07 08:25, "Jan Beulich" <jbeulich@novell.com> wrote:

> What is the reason for save_svm_cpu_user_regs() explicitly saving rax?
> Without this, the function could be eliminated, with its single use replaced
> by a call to svm_store_cpu_guest_regs().

It would be great if we can get away with just moving save/restore of rAX
into svm_{load,store}_cpu_guest_regs(), kill save_svm_cpu_user_regs()
completely, and get rid of the call at the top of the vmexit handler.
There's no equivalent call at the top of the VMX vmexit handler: all the
common HVM code will explicitly svm_store_cpu_guest_regs() before depending
on GPR state.

Or, if the loads/stores to/from the VMCB are really cheap we could simplify
things by always restoring from 'struct reg' on vmentry, always loading to
'struct reg' on vmexit (as the SVM code already does right now), and then
svm_{load,save}_cpu_guest_regs() become no-ops.

We should pick one or the other - lazy or eager - and stick with it.

 -- Keir

Re: svm: save_svm_cpu_user_regs vs. svm_store_cpu_guest_regs

[Jan Beulich]

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 09.05.07 09:51 >>>
>On 9/5/07 08:25, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>> What is the reason for save_svm_cpu_user_regs() explicitly saving rax?
>> Without this, the function could be eliminated, with its single use replaced
>> by a call to svm_store_cpu_guest_regs().
>
>It would be great if we can get away with just moving save/restore of rAX
>into svm_{load,store}_cpu_guest_regs(), kill save_svm_cpu_user_regs()
>completely, and get rid of the call at the top of the vmexit handler.
>There's no equivalent call at the top of the VMX vmexit handler: all the
>common HVM code will explicitly svm_store_cpu_guest_regs() before depending
>on GPR state.

It's not really GPR state that matters here (GPRs are saved in the respective
exit.S files), which is why I wondered about the need for saving RAX. The point
here is that CS:RIP, RFLAGS, and SS:RSP may need to be stored, and the fact
that VMX doesn't do so doesn't mean it can be freely removed from SVM code:
If I'm seeing things right (I didn't check this on hardware, yet), hvm hypercalls
are currently having a security hole on VMX in that ring 3 code can possibly
invoke them and/or prevent ring 0 from invoking them - VMX code doesn't seem
to save the CS selector anywhere, but the ring_3() test depends on that (so
generally appears to operate on stale data, i.e. whatever was saved on the
stack the last time vmx_store_cpu_guest_regs() was executed. If I wasn't
buried in hunting bugs for SLE10 SP1, I would have confirmed this already and
sent a patch if necessary (along with quite a few more ones, more or less all
addressing 32on64/hvm weakness)...

Jan

Re: svm: save_svm_cpu_user_regs vs. svm_store_cpu_guest_regs

[Keir Fraser]

On 9/5/07 09:11, "Jan Beulich" <jbeulich@novell.com> wrote:

> It's not really GPR state that matters here (GPRs are saved in the respective
> exit.S files), which is why I wondered about the need for saving RAX.

The rAX that is saved on the stack in exits.S is not the guest rAX. It's a
bogus value which gets overwritten by save_svm_cpu_user_regs().

The fact that the rAX value then gets stuffed back into VMCB in exits.S on
vmentry is pretty disgusting. We should move the rAX loading on vmexit into
exits.S as well for symmetry, and kill save_svm_cpu_user_regs().

> The point
> here is that CS:RIP, RFLAGS, and SS:RSP may need to be stored, and the fact
> that VMX doesn't do so doesn't mean it can be freely removed from SVM code:

Okay then, more precisely: modulo bugs it can be removed. Or we load/save
all VMCB state into the regs structure in exits.S, increasing latency
slightly for all vmexits but avoiding any risk of inconsistent 'struct regs'
state.

 -- Keir

> If I'm seeing things right (I didn't check this on hardware, yet), hvm
> hypercalls
> are currently having a security hole on VMX in that ring 3 code can possibly
> invoke them and/or prevent ring 0 from invoking them - VMX code doesn't seem
> to save the CS selector anywhere, but the ring_3() test depends on that (so
> generally appears to operate on stale data, i.e. whatever was saved on the
> stack the last time vmx_store_cpu_guest_regs() was executed. If I wasn't
> buried in hunting bugs for SLE10 SP1, I would have confirmed this already and
> sent a patch if necessary (along with quite a few more ones, more or less all
> addressing 32on64/hvm weakness)...

RE: svm: save_svm_cpu_user_regs vs. svm_store_cpu_guest_regs

[Petersson, Mats]

 

> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com 
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of 
> Keir Fraser
> Sent: 09 May 2007 09:35
> To: Jan Beulich
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] svm: save_svm_cpu_user_regs vs. 
> svm_store_cpu_guest_regs
> 
> On 9/5/07 09:11, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
> > It's not really GPR state that matters here (GPRs are saved 
> in the respective
> > exit.S files), which is why I wondered about the need for 
> saving RAX.
> 
> The rAX that is saved on the stack in exits.S is not the 
> guest rAX. It's a
> bogus value which gets overwritten by save_svm_cpu_user_regs().
> 
> The fact that the rAX value then gets stuffed back into VMCB 
> in exits.S on
> vmentry is pretty disgusting. We should move the rAX loading 
> on vmexit into
> exits.S as well for symmetry, and kill save_svm_cpu_user_regs().

Yes, that's my fault. I got tired of having a million checks of the type
"if (reg == RAX) ... vmcb->rax ... ; else ... regs->something ...;" in
the svm-code to deal with the fact that the RAX register isn't part of
the normally saved on the stack registers. 

I agree that is should be symmetrical tho'. 

> 
> > The point
> > here is that CS:RIP, RFLAGS, and SS:RSP may need to be 
> stored, and the fact
> > that VMX doesn't do so doesn't mean it can be freely 
> removed from SVM code:
> 
> Okay then, more precisely: modulo bugs it can be removed. Or 
> we load/save
> all VMCB state into the regs structure in exits.S, increasing latency
> slightly for all vmexits but avoiding any risk of 
> inconsistent 'struct regs'
> state.

Agreed. 

--
Mats
> 
>  -- Keir
> 
> > If I'm seeing things right (I didn't check this on 
> hardware, yet), hvm
> > hypercalls
> > are currently having a security hole on VMX in that ring 3 
> code can possibly
> > invoke them and/or prevent ring 0 from invoking them - VMX 
> code doesn't seem
> > to save the CS selector anywhere, but the ring_3() test 
> depends on that (so
> > generally appears to operate on stale data, i.e. whatever 
> was saved on the
> > stack the last time vmx_store_cpu_guest_regs() was 
> executed. If I wasn't
> > buried in hunting bugs for SLE10 SP1, I would have 
> confirmed this already and
> > sent a patch if necessary (along with quite a few more 
> ones, more or less all
> > addressing 32on64/hvm weakness)...
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 
>