VMX and save/restore guest in virtual-8086 mode

PATCH Marcelo Tosatti 3 participants 10 messages Apr 7, 2010 – Apr 8, 2010 KVM

lore

Share & tools

lore

Marcelo Tosatti

author

2010-04-07 20:24 UTC #0 lore raw

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?

qemu-kvm-x86.c +1 -0

index 748ff69..9821653 100644
@@ -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;

Avi Kivity

2010-04-07 20:46 UTC #1 lore raw

Quoted 11 lines · 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);

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...

Quoted 12 lines

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

Quoted 5 lines

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?

Quoted 2 lines

- Should be conditional on VMX !unrestricted guest.

Userspace should know nothing of this mess.

Quoted 2 lines

- Its a fugly workaround.

True.

Signature · 2 lines

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

Jan Kiszka

2010-04-08 07:23 UTC #2 lore raw

Quoted 14 lines · Avi Kivity

Quoted 11 lines · 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);

Quoted 1 line

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.

Quoted 21 lines

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...
Quoted 12 lines
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

Quoted 7 lines

Quoted 5 lines
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?

Quoted 11 lines

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

Still likely the way to go for old kernels.

Jan

Signature · 2 lines

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

Avi Kivity

2010-04-08 07:29 UTC #3 lore raw

Quoted 17 lines · Jan Kiszka

Quoted 15 lines · Avi Kivity
Quoted 12 lines · 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);

Quoted 6 lines

Quoted 2 lines
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.

Quoted 12 lines

Quoted 8 lines
Quoted 6 lines
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.

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
- 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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2010-04-08 07:54 UTC #4 lore raw

Quoted 24 lines · Avi Kivity

Quoted 16 lines · Jan Kiszka
Quoted 14 lines · Avi Kivity
Quoted 12 lines · 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);
Quoted 6 lines
Quoted 2 lines
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.

Quoted 4 lines

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.

Quoted 14 lines

Quoted 13 lines
Quoted 9 lines
Quoted 7 lines
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?

Quoted 4 lines

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.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
- Its a fugly workaround.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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

Signature · 2 lines

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

Avi Kivity

2010-04-08 08:05 UTC #5 lore raw

Quoted 10 lines · Jan Kiszka

Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
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.

Quoted 8 lines

Quoted 5 lines
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.

Quoted 2 lines

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

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

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines
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).

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

author

2010-04-08 14:41 UTC #6 lore raw

Quoted 12 lines · Avi Kivity

Quoted 11 lines · Jan Kiszka
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
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.

Quoted 5 lines

Quoted 3 lines
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.

Quoted 14 lines

Quoted 2 lines
enter_rmode/pmode should
just trigger a set_rflags to update things.
Not what I had in mind, but a valid implementation.
Quoted 2 lines
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.
Quoted 3 lines
Quoted 1 line
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.

Quoted 6 lines

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).
Signature · 1 line
error compiling committee.c: too many arguments to function

Avi Kivity

2010-04-08 14:47 UTC #7 lore raw

Quoted 18 lines · Marcelo Tosatti

Quoted 16 lines · Avi Kivity
Quoted 14 lines · Jan Kiszka
Quoted 14 lines
Quoted 9 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2010-04-08 14:57 UTC #8 lore raw

In reply to Marcelo Tosatti

Quoted 12 lines · Marcelo Tosatti

Quoted 11 lines · Avi Kivity
Quoted 10 lines · Jan Kiszka
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
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.

Quoted 6 lines

Quoted 4 lines
Quoted 3 lines
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.

Quoted 10 lines

Quoted 10 lines
Quoted 3 lines
enter_rmode/pmode should
just trigger a set_rflags to update things.
Not what I had in mind, but a valid implementation.
Quoted 2 lines
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.

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
Quoted 1 line
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

Signature · 2 lines

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

Marcelo Tosatti

author

2010-04-08 14:50 UTC #9 lore raw

In reply to Jan Kiszka

Quoted 13 lines · Jan Kiszka

Quoted 13 lines
Quoted 12 lines
Quoted 8 lines
Quoted 6 lines
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?

Quoted 6 lines

Quoted 4 lines
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.