Patchwork Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

login
register
mail settings
Submitter Marcelo Tosatti
Date March 3, 2010, 2:29 a.m.
Message ID <20100303022910.GA21054@amt.cnet>
Download mbox | patch
Permalink /patch/46754/
State New
Headers show

Comments

Marcelo Tosatti - March 3, 2010, 2:29 a.m.
On Tue, Mar 02, 2010 at 05:31:09PM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> >> Marcelo Tosatti wrote:
> >>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> >>>> This grand cleanup drops all reset and vmsave/load related
> >>>> synchronization points in favor of four(!) generic hooks:
> >>>>
> >>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
> >>>>   (initial sync from kernel before vmsave)
> >>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
> >>>>   (writeback after vmload)
> >>>> - cpu_synchronize_all_post_init in main after machine init
> >>>> - cpu_synchronize_all_post_reset in qemu_system_reset
> >>>>   (writeback after system reset)
> >>>>
> >>>> These writeback points + the existing one of VCPU exec after
> >>>> cpu_synchronize_state map on three levels of writeback:
> >>>>
> >>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> >>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> >>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> >>>>
> >>>> This level is passed to the arch-specific VCPU state writing function
> >>>> that will decide which concrete substates need to be written. That way,
> >>>> no writer of load, save or reset functions that interact with in-kernel
> >>>> KVM states will ever have to worry about synchronization again. That
> >>>> also means that a lot of reasons for races, segfaults and deadlocks are
> >>>> eliminated.
> >>>>
> >>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> >>>> continue to need it before reading or writing of VCPU states that are
> >>>> also tracked by in-kernel KVM subsystems.
> >>>>
> >>>> Consequently, this patch removes many cpu_synchronize_state calls that
> >>>> are now redundant, just like remaining explicit register syncs.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>> Jan,
> >>>
> >>> This patch breaks system reset of WinXP.32 install (more easily
> >>> reproducible without iothread enabled).
> >>>
> >>> Screenshot attached.
> >>>
> >> Strange - no issues with qemu-kvm? Any special command line switch? /me
> >> goes scrounging for some installation XP32 CD in the meantime...
> > 
> > No issues with qemu-kvm. Could not spot anything obvious.
> > 
> 
> And, of course, my WinXP installation did not trigger any reset issue,
> even in non-iothreaded mode. :(

Try kvm-autotest. I could not reproduce easily in hand run either.

This is not it, but still looks wrong:

In real mode emulation, kvm_get_sregs returns TR segment values of VMX
fields, while KVM caches them in vmx_vcpu->rmode.
Marcelo Tosatti - March 4, 2010, 4:21 a.m.
On Tue, Mar 02, 2010 at 11:29:10PM -0300, Marcelo Tosatti wrote:
> On Tue, Mar 02, 2010 at 05:31:09PM +0100, Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> > > On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> > >> Marcelo Tosatti wrote:
> > >>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> > >>>> This grand cleanup drops all reset and vmsave/load related
> > >>>> synchronization points in favor of four(!) generic hooks:
> > >>>>
> > >>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
> > >>>>   (initial sync from kernel before vmsave)
> > >>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
> > >>>>   (writeback after vmload)
> > >>>> - cpu_synchronize_all_post_init in main after machine init
> > >>>> - cpu_synchronize_all_post_reset in qemu_system_reset
> > >>>>   (writeback after system reset)
> > >>>>
> > >>>> These writeback points + the existing one of VCPU exec after
> > >>>> cpu_synchronize_state map on three levels of writeback:
> > >>>>
> > >>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> > >>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> > >>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> > >>>>
> > >>>> This level is passed to the arch-specific VCPU state writing function
> > >>>> that will decide which concrete substates need to be written. That way,
> > >>>> no writer of load, save or reset functions that interact with in-kernel
> > >>>> KVM states will ever have to worry about synchronization again. That
> > >>>> also means that a lot of reasons for races, segfaults and deadlocks are
> > >>>> eliminated.
> > >>>>
> > >>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> > >>>> continue to need it before reading or writing of VCPU states that are
> > >>>> also tracked by in-kernel KVM subsystems.
> > >>>>
> > >>>> Consequently, this patch removes many cpu_synchronize_state calls that
> > >>>> are now redundant, just like remaining explicit register syncs.
> > >>>>
> > >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > >>> Jan,
> > >>>
> > >>> This patch breaks system reset of WinXP.32 install (more easily
> > >>> reproducible without iothread enabled).
> > >>>
> > >>> Screenshot attached.
> > >>>
> > >> Strange - no issues with qemu-kvm? Any special command line switch? /me
> > >> goes scrounging for some installation XP32 CD in the meantime...
> > > 
> > > No issues with qemu-kvm. Could not spot anything obvious.
> > > 
> > 
> > And, of course, my WinXP installation did not trigger any reset issue,
> > even in non-iothreaded mode. :(

The regression seems to be caused by seabios commit d7e998f. Kevin, the
failure can be seen on the attached screenshot, which happens on the
first reboot of WinXP 32 installation (after copying files etc).
Kevin O'Connor - March 4, 2010, 5:58 a.m.
On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> The regression seems to be caused by seabios commit d7e998f. Kevin, the
> failure can be seen on the attached screenshot, which happens on the
> first reboot of WinXP 32 installation (after copying files etc).

Sorry - I also noticed a bug in that commit recently.  I pushed the
fix I had in my local tree.

-Kevin
Marcelo Tosatti - March 4, 2010, 6:35 p.m.
On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > failure can be seen on the attached screenshot, which happens on the
> > first reboot of WinXP 32 installation (after copying files etc).
> 
> Sorry - I also noticed a bug in that commit recently.  I pushed the
> fix I had in my local tree.

Thanks, it does fix the issue here. Anthony can you please update
seabios?

TIA
Kevin O'Connor - March 6, 2010, 2:37 a.m.
On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
> On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> > On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > > failure can be seen on the attached screenshot, which happens on the
> > > first reboot of WinXP 32 installation (after copying files etc).
> > 
> > Sorry - I also noticed a bug in that commit recently.  I pushed the
> > fix I had in my local tree.
> 
> Thanks, it does fix the issue here. Anthony can you please update
> seabios?

Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
branch.  Is qemu ready to pull in bigger changes now?

-Kevin
Marcelo Tosatti - March 8, 2010, 8:33 p.m.
On Fri, Mar 05, 2010 at 09:37:26PM -0500, Kevin O'Connor wrote:
> On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
> > On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> > > On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > > > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > > > failure can be seen on the attached screenshot, which happens on the
> > > > first reboot of WinXP 32 installation (after copying files etc).
> > > 
> > > Sorry - I also noticed a bug in that commit recently.  I pushed the
> > > fix I had in my local tree.
> > 
> > Thanks, it does fix the issue here. Anthony can you please update
> > seabios?
> 
> Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
> branch.  Is qemu ready to pull in bigger changes now?

Anthony pulls in seabios master into qemu.git master periodically.
Anthony Liguori - March 9, 2010, 1:56 p.m.
On 03/08/2010 02:33 PM, Marcelo Tosatti wrote:
> On Fri, Mar 05, 2010 at 09:37:26PM -0500, Kevin O'Connor wrote:
>    
>> On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
>>      
>>> On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
>>>        
>>>> On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
>>>>          
>>>>> The regression seems to be caused by seabios commit d7e998f. Kevin, the
>>>>> failure can be seen on the attached screenshot, which happens on the
>>>>> first reboot of WinXP 32 installation (after copying files etc).
>>>>>            
>>>> Sorry - I also noticed a bug in that commit recently.  I pushed the
>>>> fix I had in my local tree.
>>>>          
>>> Thanks, it does fix the issue here. Anthony can you please update
>>> seabios?
>>>        
>> Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
>> branch.  Is qemu ready to pull in bigger changes now?
>>      
> Anthony pulls in seabios master into qemu.git master periodically.
>    

We should be up to date now FWIW.

Regards,

Anthony Liguori

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 14873b9..898173a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1822,13 +1822,23 @@  static u64 vmx_get_segment_base(struct kvm_vcpu *vcpu, int seg)
 static void vmx_get_segment(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg)
 {
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
 	u32 ar;
 
+	if (vmx->rmode.vm86_active && seg == VCPU_SREG_TR) {
+		var->base = vmx->rmode.tr.base;
+		var->limit = vmx->rmode.tr.limit;
+		var->selector = vmx->rmode.tr.selector;
+		ar = vmx->rmode.tr.ar;
+		goto ar;
+	}
+
 	var->base = vmcs_readl(sf->base);
 	var->limit = vmcs_read32(sf->limit);
 	var->selector = vmcs_read16(sf->selector);
 	ar = vmcs_read32(sf->ar_bytes);
+ar:
 	if ((ar & AR_UNUSABLE_MASK) && !emulate_invalid_guest_state)
 		ar = 0;
 	var->type = ar & 15;