VMX and save/restore guest in virtual-8086 mode

VMX and save/restore guest in virtual-8086 mode

[Marcelo Tosatti]

During initialization, WinXP.32 switches to virtual-8086 mode, with
paging enabled, to use VGABIOS functions.

Since enter_pmode unconditionally clears IOPL and VM bits in RFLAGS

        flags = vmcs_readl(GUEST_RFLAGS);
        flags &= ~(X86_EFLAGS_IOPL | X86_EFLAGS_VM);
        flags |= (vmx->rmode.save_iopl << IOPL_SHIFT);
        vmcs_writel(GUEST_RFLAGS, flags);

And the order of loading state is set_regs (rflags) followed by
set_sregs (cr0), these bits are lost across save/restore:

savevm 1
kvm_arch_save_regs EIP=7a04 cr0=8001003b eflags=33286
system_reset
loadvm 1
kvm_arch_save_regs EIP=7a04 cr0=8001003b eflags=10286
cont
kvm: unhandled exit 80000021
kvm_run returned -22

The following patch fixes it, but it has some drawbacks:

- cpu_synchronize_state+writeback is noticeably slow with tpr patching,
  this makes it slower.
- Should be conditional on VMX !unrestricted guest.
- Its a fugly workaround.

Any better ideas?

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 748ff69..9821653 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -956,6 +956,7 @@ void kvm_arch_load_regs(CPUState *env, int level)
     sregs.efer = env->efer;
 
     kvm_set_sregs(env, &sregs);
+    kvm_set_regs(env, &regs);
 
     /* msrs */
     n = 0;

Re: VMX and save/restore guest in virtual-8086 mode

[Avi Kivity]

On 04/07/2010 11:24 PM, Marcelo Tosatti wrote:
> During initialization, WinXP.32 switches to virtual-8086 mode, with
> paging enabled, to use VGABIOS functions.
>
> Since enter_pmode unconditionally clears IOPL and VM bits in RFLAGS
>
>          flags = vmcs_readl(GUEST_RFLAGS);
>          flags&= ~(X86_EFLAGS_IOPL | X86_EFLAGS_VM);
>          flags |= (vmx->rmode.save_iopl<<  IOPL_SHIFT);
>          vmcs_writel(GUEST_RFLAGS, flags);
>
>    


Looks like KVM_SET_REGS should write rmode.save_iopl (and a new save_vm)?

I think we have a small related bug in realmode emulation - we run the 
guest with iopl=3.  This means the guest can use pushfl and see the host 
iopl instead of the guest iopl.  We should run with iopl=0, which causes 
pushfl/popfl to #GP, where we can emulate the flags correctly (by 
updating rmode.save_iopl and rmode.save_vm).  That has lots of 
implications however...


> And the order of loading state is set_regs (rflags) followed by
> set_sregs (cr0), these bits are lost across save/restore:
>
> savevm 1
> kvm_arch_save_regs EIP=7a04 cr0=8001003b eflags=33286
> system_reset
> loadvm 1
> kvm_arch_save_regs EIP=7a04 cr0=8001003b eflags=10286
> cont
> kvm: unhandled exit 80000021
> kvm_run returned -22
>
> The following patch fixes it, but it has some drawbacks:
>
> - cpu_synchronize_state+writeback is noticeably slow with tpr patching,
>    this makes it slower.
>    

Isn't it a very rare event?

> - Should be conditional on VMX !unrestricted guest.
>    

Userspace should know nothing of this mess.

> - Its a fugly workaround.
>    

True.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: VMX and save/restore guest in virtual-8086 mode

[Jan Kiszka]

Avi Kivity wrote:
> On 04/07/2010 11:24 PM, Marcelo Tosatti wrote:
>> During initialization, WinXP.32 switches to virtual-8086 mode, with
>> paging enabled, to use VGABIOS functions.
>>
>> Since enter_pmode unconditionally clears IOPL and VM bits in RFLAGS
>>
>>          flags = vmcs_readl(GUEST_RFLAGS);
>>          flags&= ~(X86_EFLAGS_IOPL | X86_EFLAGS_VM);
>>          flags |= (vmx->rmode.save_iopl<<  IOPL_SHIFT);
>>          vmcs_writel(GUEST_RFLAGS, flags);
>>
>>    
> 
> 
> Looks like KVM_SET_REGS should write rmode.save_iopl (and a new save_vm)?

Just like we manipulate the flags for guest debugging in the
set/get_rflags vendor handlers, the same should happen for IOPL and VM.
This is no business of enter_pmode/rmode.

> 
> I think we have a small related bug in realmode emulation - we run the 
> guest with iopl=3.  This means the guest can use pushfl and see the host 
> iopl instead of the guest iopl.  We should run with iopl=0, which causes 
> pushfl/popfl to #GP, where we can emulate the flags correctly (by 
> updating rmode.save_iopl and rmode.save_vm).  That has lots of 
> implications however...
> 
> 
>> And the order of loading state is set_regs (rflags) followed by
>> set_sregs (cr0), these bits are lost across save/restore:
>>
>> savevm 1
>> kvm_arch_save_regs EIP=7a04 cr0=8001003b eflags=33286
>> system_reset
>> loadvm 1
>> kvm_arch_save_regs EIP=7a04 cr0=8001003b eflags=10286
>> cont
>> kvm: unhandled exit 80000021
>> kvm_run returned -22
>>
>> The following patch fixes it, but it has some drawbacks:
>>
>> - cpu_synchronize_state+writeback is noticeably slow with tpr patching,
>>    this makes it slower.
>>    
> 
> Isn't it a very rare event?

It has to be - otherwise the decision to go for full sync and individual
get/set IOCTL would have been wrong. What happens during tpr patching?

> 
>> - Should be conditional on VMX !unrestricted guest.
>>    
> 
> Userspace should know nothing of this mess.
> 
>> - Its a fugly workaround.
>>    
> 
> True.
> 

Still likely the way to go for old kernels.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: VMX and save/restore guest in virtual-8086 mode

[Avi Kivity]

On 04/08/2010 10:22 AM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 04/07/2010 11:24 PM, Marcelo Tosatti wrote:
>>      
>>> During initialization, WinXP.32 switches to virtual-8086 mode, with
>>> paging enabled, to use VGABIOS functions.
>>>
>>> Since enter_pmode unconditionally clears IOPL and VM bits in RFLAGS
>>>
>>>           flags = vmcs_readl(GUEST_RFLAGS);
>>>           flags&= ~(X86_EFLAGS_IOPL | X86_EFLAGS_VM);
>>>           flags |= (vmx->rmode.save_iopl<<   IOPL_SHIFT);
>>>           vmcs_writel(GUEST_RFLAGS, flags);
>>>
>>>
>>>        
>>
>> Looks like KVM_SET_REGS should write rmode.save_iopl (and a new save_vm)?
>>      
> Just like we manipulate the flags for guest debugging in the
> set/get_rflags vendor handlers, the same should happen for IOPL and VM.
> This is no business of enter_pmode/rmode.
>    

This is vendor specific code, and it isn't manipulating guest values, 
only host values (->set_rflags() is called when the guest value changes, 
which isn't happening here).  Of course some refactoring will be helpful 
here.

>>> The following patch fixes it, but it has some drawbacks:
>>>
>>> - cpu_synchronize_state+writeback is noticeably slow with tpr patching,
>>>     this makes it slower.
>>>
>>>        
>> Isn't it a very rare event?
>>      
> It has to be - otherwise the decision to go for full sync and individual
> get/set IOCTL would have been wrong. What happens during tpr patching?
>
>    

tpr patching listens for instructions which access the tpr and patches 
them to a call instruction (targeting some hacky code in the bios).  
Since there are a limited number of such instructions (20-30 IIRC) you 
expect tpr patching to happen very rarely.

>>> - Its a fugly workaround.
>>>
>>>        
>> True.
>>
>>      
> Still likely the way to go for old kernels.
>
>    

It's a bugfix that can go into -stable and supported distribution kernels.

-- 
error compiling committee.c: too many arguments to function

Re: VMX and save/restore guest in virtual-8086 mode

[Jan Kiszka]

Avi Kivity wrote:
> On 04/08/2010 10:22 AM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>    
>>> On 04/07/2010 11:24 PM, Marcelo Tosatti wrote:
>>>      
>>>> During initialization, WinXP.32 switches to virtual-8086 mode, with
>>>> paging enabled, to use VGABIOS functions.
>>>>
>>>> Since enter_pmode unconditionally clears IOPL and VM bits in RFLAGS
>>>>
>>>>           flags = vmcs_readl(GUEST_RFLAGS);
>>>>           flags&= ~(X86_EFLAGS_IOPL | X86_EFLAGS_VM);
>>>>           flags |= (vmx->rmode.save_iopl<<   IOPL_SHIFT);
>>>>           vmcs_writel(GUEST_RFLAGS, flags);
>>>>
>>>>
>>>>        
>>> Looks like KVM_SET_REGS should write rmode.save_iopl (and a new save_vm)?
>>>      
>> Just like we manipulate the flags for guest debugging in the
>> set/get_rflags vendor handlers, the same should happen for IOPL and VM.
>> This is no business of enter_pmode/rmode.
>>    
> 
> This is vendor specific code, and it isn't manipulating guest values, 
> only host values (->set_rflags() is called when the guest value changes, 
> which isn't happening here).  Of course some refactoring will be helpful 
> here.

Actually, the bug is that enter_pmode/rmode update save_iopl (and that
no one saves the VM bit). That should happen in vmx_set_rflags to also
keep track of changes _while_ we are in rmode. enter_rmode/pmode should
just trigger a set_rflags to update things. And vmx_get_rflags must
properly inject the saved flags instead of masking them out.

> 
>>>> The following patch fixes it, but it has some drawbacks:
>>>>
>>>> - cpu_synchronize_state+writeback is noticeably slow with tpr patching,
>>>>     this makes it slower.
>>>>
>>>>        
>>> Isn't it a very rare event?
>>>      
>> It has to be - otherwise the decision to go for full sync and individual
>> get/set IOCTL would have been wrong. What happens during tpr patching?
>>
>>    
> 
> tpr patching listens for instructions which access the tpr and patches 
> them to a call instruction (targeting some hacky code in the bios).  
> Since there are a limited number of such instructions (20-30 IIRC) you 
> expect tpr patching to happen very rarely.

Then I wonder why it is noticeable.

> 
>>>> - Its a fugly workaround.
>>>>
>>>>        
>>> True.
>>>
>>>      
>> Still likely the way to go for old kernels.
>>
>>    
> 
> It's a bugfix that can go into -stable and supported distribution kernels.

Well, would be happy to throw out tones of workaround based on this
approach. :)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: VMX and save/restore guest in virtual-8086 mode

[Avi Kivity]

On 04/08/2010 10:54 AM, Jan Kiszka wrote:
>
>    
>>>> Looks like KVM_SET_REGS should write rmode.save_iopl (and a new save_vm)?
>>>>
>>>>          
>>> Just like we manipulate the flags for guest debugging in the
>>> set/get_rflags vendor handlers, the same should happen for IOPL and VM.
>>> This is no business of enter_pmode/rmode.
>>>
>>>        
>> This is vendor specific code, and it isn't manipulating guest values,
>> only host values (->set_rflags() is called when the guest value changes,
>> which isn't happening here).  Of course some refactoring will be helpful
>> here.
>>      
> Actually, the bug is that enter_pmode/rmode update save_iopl (and that
> no one saves the VM bit). That should happen in vmx_set_rflags to also
> keep track of changes _while_ we are in rmode.

Exactly - that's what I suggested above.

> enter_rmode/pmode should
> just trigger a set_rflags to update things.

Not what I had in mind, but a valid implementation.

> And vmx_get_rflags must
> properly inject the saved flags instead of masking them out.
>    

Yes.  No one ever bothers to play with iopl in real mode, so we never 
noticed this.  We do this for cr0 for example.

>> It's a bugfix that can go into -stable and supported distribution kernels.
>>      
> Well, would be happy to throw out tones of workaround based on this
> approach. :)
>    

And I'll be happy to apply such patches.  Just ensure that 2.6.32.y and 
above have the fixes so we don't introduce regressions (I think most 
workarounds are a lot older).

-- 
error compiling committee.c: too many arguments to function

Re: VMX and save/restore guest in virtual-8086 mode

[Marcelo Tosatti]

On Thu, Apr 08, 2010 at 11:05:56AM +0300, Avi Kivity wrote:
> On 04/08/2010 10:54 AM, Jan Kiszka wrote:
> >
> >>>>Looks like KVM_SET_REGS should write rmode.save_iopl (and a new save_vm)?
> >>>>
> >>>Just like we manipulate the flags for guest debugging in the
> >>>set/get_rflags vendor handlers, the same should happen for IOPL and VM.
> >>>This is no business of enter_pmode/rmode.
> >>>
> >>This is vendor specific code, and it isn't manipulating guest values,
> >>only host values (->set_rflags() is called when the guest value changes,
> >>which isn't happening here).  Of course some refactoring will be helpful
> >>here.
> >Actually, the bug is that enter_pmode/rmode update save_iopl (and that
> >no one saves the VM bit). That should happen in vmx_set_rflags to also
> >keep track of changes _while_ we are in rmode.
> 
> Exactly - that's what I suggested above.

And new ioctl to save/restore save_iopl/save_vm.

> >enter_rmode/pmode should
> >just trigger a set_rflags to update things.
> 
> Not what I had in mind, but a valid implementation.
> 
> >And vmx_get_rflags must
> >properly inject the saved flags instead of masking them out.
> 
> Yes.  No one ever bothers to play with iopl in real mode, so we
> never noticed this.  We do this for cr0 for example.
> 
> >>It's a bugfix that can go into -stable and supported distribution kernels.
> >Well, would be happy to throw out tones of workaround based on this
> >approach. :)

Do you mean you'd be interested in writing the patch? Sure, go ahead,
let me know otherwise.

> And I'll be happy to apply such patches.  Just ensure that 2.6.32.y
> and above have the fixes so we don't introduce regressions (I think
> most workarounds are a lot older).
> 
> -- 
> error compiling committee.c: too many arguments to function

Re: VMX and save/restore guest in virtual-8086 mode

[Avi Kivity]

On 04/08/2010 05:16 PM, Marcelo Tosatti wrote:
> On Thu, Apr 08, 2010 at 11:05:56AM +0300, Avi Kivity wrote:
>    
>> On 04/08/2010 10:54 AM, Jan Kiszka wrote:
>>      
>>>        
>>>>>> Looks like KVM_SET_REGS should write rmode.save_iopl (and a new save_vm)?
>>>>>>
>>>>>>              
>>>>> Just like we manipulate the flags for guest debugging in the
>>>>> set/get_rflags vendor handlers, the same should happen for IOPL and VM.
>>>>> This is no business of enter_pmode/rmode.
>>>>>
>>>>>            
>>>> This is vendor specific code, and it isn't manipulating guest values,
>>>> only host values (->set_rflags() is called when the guest value changes,
>>>> which isn't happening here).  Of course some refactoring will be helpful
>>>> here.
>>>>          
>>> Actually, the bug is that enter_pmode/rmode update save_iopl (and that
>>> no one saves the VM bit). That should happen in vmx_set_rflags to also
>>> keep track of changes _while_ we are in rmode.
>>>        
>> Exactly - that's what I suggested above.
>>      
> And new ioctl to save/restore save_iopl/save_vm.
>    

That ioctl already exists, KVM_{GET,SET}_REGS.

We're writing via KVM_SET_SREGS eflags.vm=1 and eflags.iopl=3 while 
cr0.pe=0.  vmx_set_rflags() notices this and sets rmode.save_vm=1 and 
rmode.save_iopl=3.  Next we write via KVM_SET_SREGS cr0.pe=1.  So we 
call enter_pmode(), and recover eflags.vm and eflags.iopl from rmode.vm 
and rmode.iopl.  Win!

It's similar to how we handle cr0.ts, sometimes the host owns it so we 
keep it in a shadow register, sometimes the guest owns it so we keep it 
in cr0.

>>>> It's a bugfix that can go into -stable and supported distribution kernels.
>>>>          
>>> Well, would be happy to throw out tones of workaround based on this
>>> approach. :)
>>>        
> Do you mean you'd be interested in writing the patch? Sure, go ahead,
> let me know otherwise.
>    

I took it to mean he wants to kill the other qemu workarounds for kernel 
bugs.

-- 
error compiling committee.c: too many arguments to function

Re: VMX and save/restore guest in virtual-8086 mode

[Marcelo Tosatti]

On Thu, Apr 08, 2010 at 09:54:35AM +0200, Jan Kiszka wrote:
> >>>> The following patch fixes it, but it has some drawbacks:
> >>>>
> >>>> - cpu_synchronize_state+writeback is noticeably slow with tpr patching,
> >>>>     this makes it slower.
> >>>>
> >>>>        
> >>> Isn't it a very rare event?
> >>>      
> >> It has to be - otherwise the decision to go for full sync and individual
> >> get/set IOCTL would have been wrong. What happens during tpr patching?
> >>
> >>    
> > 
> > tpr patching listens for instructions which access the tpr and patches 
> > them to a call instruction (targeting some hacky code in the bios).  
> > Since there are a limited number of such instructions (20-30 IIRC) you 
> > expect tpr patching to happen very rarely.
> 
> Then I wonder why it is noticeable.

While switching kvm-tpr-opt.c from explicit {get,put}_{s}regs
to cpu_synchronize_state+writeback i noticed WinXP.32 boot
became visually slower. For some reason, the delay introduced by
cpu_synchronize_state+writeback forbids patching certain instructions
for longer periods, or somehow allows Windows to use unpatched
instructions more often </guess>. End result was 4x more patching (from
700 to 4000, roughly). Confirmed it was a timing issue by introducing
delays to original {get,put}_{s}regs version.

The particular tpr case is no big deal since as mentioned its a short
lived period, but for things like Kemari this might be an issue. But 
this is another discussion.

Re: VMX and save/restore guest in virtual-8086 mode

[Jan Kiszka]

Marcelo Tosatti wrote:
> On Thu, Apr 08, 2010 at 11:05:56AM +0300, Avi Kivity wrote:
>> On 04/08/2010 10:54 AM, Jan Kiszka wrote:
>>>>>> Looks like KVM_SET_REGS should write rmode.save_iopl (and a new save_vm)?
>>>>>>
>>>>> Just like we manipulate the flags for guest debugging in the
>>>>> set/get_rflags vendor handlers, the same should happen for IOPL and VM.
>>>>> This is no business of enter_pmode/rmode.
>>>>>
>>>> This is vendor specific code, and it isn't manipulating guest values,
>>>> only host values (->set_rflags() is called when the guest value changes,
>>>> which isn't happening here).  Of course some refactoring will be helpful
>>>> here.
>>> Actually, the bug is that enter_pmode/rmode update save_iopl (and that
>>> no one saves the VM bit). That should happen in vmx_set_rflags to also
>>> keep track of changes _while_ we are in rmode.
>> Exactly - that's what I suggested above.
> 
> And new ioctl to save/restore save_iopl/save_vm.

Not need. The information will all be contained in eflags and cr0 as
returned to userspace. The bug is that the wrong information is
currently returned, thus saved/restored.

> 
>>> enter_rmode/pmode should
>>> just trigger a set_rflags to update things.
>> Not what I had in mind, but a valid implementation.
>>
>>> And vmx_get_rflags must
>>> properly inject the saved flags instead of masking them out.
>> Yes.  No one ever bothers to play with iopl in real mode, so we
>> never noticed this.  We do this for cr0 for example.
>>
>>>> It's a bugfix that can go into -stable and supported distribution kernels.
>>> Well, would be happy to throw out tones of workaround based on this
>>> approach. :)
> 
> Do you mean you'd be interested in writing the patch? Sure, go ahead,
> let me know otherwise.

ATM, I fighting against too many customer bugs. And you have the test
case for this particular issue, I assume. So don't wait for me here.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux