| Message ID | 4A678A56.7050901@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote: > Release and re-acquire preemption and IRQ lock in the same order as > vcpu_enter_guest does. This should happen in vcpu_enter_guest, before it decides to disable preemption/irqs (so you consolidate the control there). Maybe add a new member to x86_ops? > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > > arch/x86/kvm/vmx.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index d75c271..4f914c3 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3324,8 +3324,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu, > struct vcpu_vmx *vmx = to_vmx(vcpu); > enum emulation_result err = EMULATE_DONE; > > - preempt_enable(); > local_irq_enable(); > + preempt_enable(); > > while (!guest_state_valid(vcpu)) { > err = emulate_instruction(vcpu, kvm_run, 0, 0, 0); > @@ -3344,8 +3344,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu, > schedule(); > } > > - local_irq_disable(); > preempt_disable(); > + local_irq_disable(); > > vmx->invalid_state_emulation_result = err; > } > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti wrote: > On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote: >> Release and re-acquire preemption and IRQ lock in the same order as >> vcpu_enter_guest does. > > This should happen in vcpu_enter_guest, before it decides to disable > preemption/irqs (so you consolidate the control there). Maybe, maybe not. handle_invalid_guest_state is an alternative way of "executing" guest code, and it currently shares the setup and tear-down with vmx_vcpu_run. If it has to share parts that actually require preemption and IRQ lock, then moving makes not much sense. Can anyone comment on what the requirements for handle_invalid_guest_state are? I would suggest to merge this fix first and then decide about and potentially merge a refactoring patch. Jan > > Maybe add a new member to x86_ops? > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> >> arch/x86/kvm/vmx.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index d75c271..4f914c3 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -3324,8 +3324,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu, >> struct vcpu_vmx *vmx = to_vmx(vcpu); >> enum emulation_result err = EMULATE_DONE; >> >> - preempt_enable(); >> local_irq_enable(); >> + preempt_enable(); >> >> while (!guest_state_valid(vcpu)) { >> err = emulate_instruction(vcpu, kvm_run, 0, 0, 0); >> @@ -3344,8 +3344,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu, >> schedule(); >> } >> >> - local_irq_disable(); >> preempt_disable(); >> + local_irq_disable(); >> >> vmx->invalid_state_emulation_result = err; >> } >> > >
On 07/24/2009 10:00 AM, Jan Kiszka wrote: > Marcelo Tosatti wrote: > >> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote: >> >>> Release and re-acquire preemption and IRQ lock in the same order as >>> vcpu_enter_guest does. >>> >> This should happen in vcpu_enter_guest, before it decides to disable >> preemption/irqs (so you consolidate the control there). >> > > Maybe, maybe not. handle_invalid_guest_state is an alternative way of > "executing" guest code, and it currently shares the setup and tear-down > with vmx_vcpu_run. If it has to share parts that actually require > preemption and IRQ lock, then moving makes not much sense. Can anyone > comment on what the requirements for handle_invalid_guest_state are? > Like you said, it's an alternative to vmx entry/exit, so it shares the same requirements. It must run with interrupts and preemption enabled, but any code that normally runs in the entry critical section (like interrupt injection) must continue to run in a critical section. > I would suggest to merge this fix first and then decide about and > potentially merge a refactoring patch. > btw, what does it fix? a debug warning?
Avi Kivity wrote: > On 07/24/2009 10:00 AM, Jan Kiszka wrote: >> Marcelo Tosatti wrote: >> >>> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote: >>> >>>> Release and re-acquire preemption and IRQ lock in the same order as >>>> vcpu_enter_guest does. >>>> >>> This should happen in vcpu_enter_guest, before it decides to disable >>> preemption/irqs (so you consolidate the control there). >>> >> >> Maybe, maybe not. handle_invalid_guest_state is an alternative way of >> "executing" guest code, and it currently shares the setup and tear-down >> with vmx_vcpu_run. If it has to share parts that actually require >> preemption and IRQ lock, then moving makes not much sense. Can anyone >> comment on what the requirements for handle_invalid_guest_state are? >> > > Like you said, it's an alternative to vmx entry/exit, so it shares the > same requirements. It must run with interrupts and preemption enabled, > but any code that normally runs in the entry critical section (like > interrupt injection) must continue to run in a critical section. > > >> I would suggest to merge this fix first and then decide about and >> potentially merge a refactoring patch. >> > > btw, what does it fix? a debug warning? > I haven't seen anything in the wild, and I don't think it would raise a warning. All it should cause is a potential delay of some pending reschedule as preempt_enable will not fire under local_irq_disable. Jan
On 07/26/2009 05:23 PM, Jan Kiszka wrote: >> btw, what does it fix? a debug warning? >> >> > > I haven't seen anything in the wild, and I don't think it would raise a > warning. All it should cause is a potential delay of some pending > reschedule as preempt_enable will not fire under local_irq_disable. > Ah, okay, then it is a real fix. Preempt-correctness is important. (but won't local_irq_enable() reschedule?)
Avi Kivity wrote: > On 07/26/2009 05:23 PM, Jan Kiszka wrote: >>> btw, what does it fix? a debug warning? >>> >>> >> >> I haven't seen anything in the wild, and I don't think it would raise a >> warning. All it should cause is a potential delay of some pending >> reschedule as preempt_enable will not fire under local_irq_disable. >> > > Ah, okay, then it is a real fix. Preempt-correctness is important. > > (but won't local_irq_enable() reschedule?) The last time I checked it was essentially a plain 'sti'. Jan
On 07/26/2009 05:38 PM, Jan Kiszka wrote: > Avi Kivity wrote: > >> On 07/26/2009 05:23 PM, Jan Kiszka wrote: >> >>>> btw, what does it fix? a debug warning? >>>> >>>> >>>> >>> I haven't seen anything in the wild, and I don't think it would raise a >>> warning. All it should cause is a potential delay of some pending >>> reschedule as preempt_enable will not fire under local_irq_disable. >>> >>> >> Ah, okay, then it is a real fix. Preempt-correctness is important. >> >> (but won't local_irq_enable() reschedule?) >> > > The last time I checked it was essentially a plain 'sti'. > > Presumably there's a reschedule interrupt queued; I think if you set the reschedule bit you have to IPI the cpu running the task.
Avi Kivity wrote: > On 07/26/2009 05:38 PM, Jan Kiszka wrote: >> Avi Kivity wrote: >> >>> On 07/26/2009 05:23 PM, Jan Kiszka wrote: >>> >>>>> btw, what does it fix? a debug warning? >>>>> >>>>> >>>>> >>>> I haven't seen anything in the wild, and I don't think it would raise a >>>> warning. All it should cause is a potential delay of some pending >>>> reschedule as preempt_enable will not fire under local_irq_disable. >>>> >>>> >>> Ah, okay, then it is a real fix. Preempt-correctness is important. >>> >>> (but won't local_irq_enable() reschedule?) >>> >> >> The last time I checked it was essentially a plain 'sti'. >> >> > > Presumably there's a reschedule interrupt queued; I think if you set the > reschedule bit you have to IPI the cpu running the task. > Yeah. But as we preempt_disable first, that one might have been processed already. Jan
On 07/26/2009 05:55 PM, Jan Kiszka wrote: >> Presumably there's a reschedule interrupt queued; I think if you set the >> reschedule bit you have to IPI the cpu running the task. >> > > Yeah. But as we preempt_disable first, that one might have been > processed already. > Ah, yes. Thanks.
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index d75c271..4f914c3 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3324,8 +3324,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu, struct vcpu_vmx *vmx = to_vmx(vcpu); enum emulation_result err = EMULATE_DONE; - preempt_enable(); local_irq_enable(); + preempt_enable(); while (!guest_state_valid(vcpu)) { err = emulate_instruction(vcpu, kvm_run, 0, 0, 0); @@ -3344,8 +3344,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu, schedule(); } - local_irq_disable(); preempt_disable(); + local_irq_disable(); vmx->invalid_state_emulation_result = err; }
Release and re-acquire preemption and IRQ lock in the same order as vcpu_enter_guest does. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- arch/x86/kvm/vmx.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)