| Message ID | 1243971470-31676-2-git-send-email-glommer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Glauber Costa wrote: > This is not kvm specific, and should do fine in plain qemu This is fine with plain qemu already. The problem, IIUC, is that in-kernel kvm irqchip does not have a chance to remove the halted state again. Did you test the effect of this patch on that scenario? What makes it safe to be removed now? > > Signed-off-by: Glauber Costa <glommer@redhat.com> > --- > hw/apic.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/hw/apic.c b/hw/apic.c > index 86aa6b6..2eddba0 100644 > --- a/hw/apic.c > +++ b/hw/apic.c > @@ -467,8 +467,7 @@ static void apic_init_ipi(APICState *s) > > cpu_reset(s->cpu_env); > > - if (!(s->apicbase & MSR_IA32_APICBASE_BSP) && > - (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel())) > + if (!(s->apicbase & MSR_IA32_APICBASE_BSP)) > s->cpu_env->halted = 1; > > if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel()) Jan
On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote: > Glauber Costa wrote: > > This is not kvm specific, and should do fine in plain qemu > > This is fine with plain qemu already. The problem, IIUC, is that > in-kernel kvm irqchip does not have a chance to remove the halted state > again. Did you test the effect of this patch on that scenario? What > makes it safe to be removed now? IIRC, the in kernel irqchip sets halted = 0 in the very beginning of the vcpu initialization. It is tested here with in-kernel irqchip and works, so probably not a problem, unless you can spot something. -- 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
Glauber Costa wrote: > On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote: >> Glauber Costa wrote: >>> This is not kvm specific, and should do fine in plain qemu >> This is fine with plain qemu already. The problem, IIUC, is that >> in-kernel kvm irqchip does not have a chance to remove the halted state >> again. Did you test the effect of this patch on that scenario? What >> makes it safe to be removed now? > IIRC, the in kernel irqchip sets halted = 0 in the very beginning of > the vcpu initialization. > > It is tested here with in-kernel irqchip and works, so probably not > a problem, unless you can spot something. At least your patch applied alone breaks -smp >1 here. But the whole management of env->halted for the in-kernel irqchip in qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be nice to always see a consistent halted in user space, specifically for debugging purposes. Jan
On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote: > Glauber Costa wrote: > > On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote: > >> Glauber Costa wrote: > >>> This is not kvm specific, and should do fine in plain qemu > >> This is fine with plain qemu already. The problem, IIUC, is that > >> in-kernel kvm irqchip does not have a chance to remove the halted state > >> again. Did you test the effect of this patch on that scenario? What > >> makes it safe to be removed now? > > IIRC, the in kernel irqchip sets halted = 0 in the very beginning of > > the vcpu initialization. > > > > It is tested here with in-kernel irqchip and works, so probably not > > a problem, unless you can spot something. > > At least your patch applied alone breaks -smp >1 here. > > But the whole management of env->halted for the in-kernel irqchip in > qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be > nice to always see a consistent halted in user space, specifically for > debugging purposes. out of curiosity: did you apply the whole series? please report with it. I suspect there is a change later on that might make it work. Of course, this is no excuse, as I'm a huge fan of bisectability. If this is the case, I'll rework the series in a way that it always work. -- 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
Glauber Costa wrote: > On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote: >> Glauber Costa wrote: >>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote: >>>> Glauber Costa wrote: >>>>> This is not kvm specific, and should do fine in plain qemu >>>> This is fine with plain qemu already. The problem, IIUC, is that >>>> in-kernel kvm irqchip does not have a chance to remove the halted state >>>> again. Did you test the effect of this patch on that scenario? What >>>> makes it safe to be removed now? >>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of >>> the vcpu initialization. >>> >>> It is tested here with in-kernel irqchip and works, so probably not >>> a problem, unless you can spot something. >> At least your patch applied alone breaks -smp >1 here. >> >> But the whole management of env->halted for the in-kernel irqchip in >> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be >> nice to always see a consistent halted in user space, specifically for >> debugging purposes. > out of curiosity: did you apply the whole series? Meanwhile I did, but it makes no difference. > > please report with it. I suspect there is a change later on that might > make it work. Of course, this is no excuse, as I'm a huge fan of bisectability. > If this is the case, I'll rework the series in a way that it always work. I still suspect that dealing with halted for the in-kernel case is a bit more tricky. Jan
On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote: > Glauber Costa wrote: > > On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote: > >> Glauber Costa wrote: > >>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote: > >>>> Glauber Costa wrote: > >>>>> This is not kvm specific, and should do fine in plain qemu > >>>> This is fine with plain qemu already. The problem, IIUC, is that > >>>> in-kernel kvm irqchip does not have a chance to remove the halted state > >>>> again. Did you test the effect of this patch on that scenario? What > >>>> makes it safe to be removed now? > >>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of > >>> the vcpu initialization. > >>> > >>> It is tested here with in-kernel irqchip and works, so probably not > >>> a problem, unless you can spot something. > >> At least your patch applied alone breaks -smp >1 here. > >> > >> But the whole management of env->halted for the in-kernel irqchip in > >> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be > >> nice to always see a consistent halted in user space, specifically for > >> debugging purposes. > > out of curiosity: did you apply the whole series? > > Meanwhile I did, but it makes no difference. ok, thanks for testing. I'll take a look at that tomorrow. -- 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
On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote: > Glauber Costa wrote: > > On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote: > >> Glauber Costa wrote: > >>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote: > >>>> Glauber Costa wrote: > >>>>> This is not kvm specific, and should do fine in plain qemu > >>>> This is fine with plain qemu already. The problem, IIUC, is that > >>>> in-kernel kvm irqchip does not have a chance to remove the halted state > >>>> again. Did you test the effect of this patch on that scenario? What > >>>> makes it safe to be removed now? > >>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of > >>> the vcpu initialization. > >>> > >>> It is tested here with in-kernel irqchip and works, so probably not > >>> a problem, unless you can spot something. > >> At least your patch applied alone breaks -smp >1 here. > >> > >> But the whole management of env->halted for the in-kernel irqchip in > >> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be > >> nice to always see a consistent halted in user space, specifically for > >> debugging purposes. > > out of curiosity: did you apply the whole series? > > Meanwhile I did, but it makes no difference. Jan, can you be more specific on why it breaks? I'm double trying it here, and it works for me just fine. -- 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
On Tue, Jun 02, 2009 at 03:37:47PM -0400, Glauber Costa wrote: > This is not kvm specific, and should do fine in plain qemu > > Signed-off-by: Glauber Costa <glommer@redhat.com> > --- > hw/apic.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/hw/apic.c b/hw/apic.c > index 86aa6b6..2eddba0 100644 > --- a/hw/apic.c > +++ b/hw/apic.c > @@ -467,8 +467,7 @@ static void apic_init_ipi(APICState *s) > > cpu_reset(s->cpu_env); > > - if (!(s->apicbase & MSR_IA32_APICBASE_BSP) && > - (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel())) > + if (!(s->apicbase & MSR_IA32_APICBASE_BSP)) > s->cpu_env->halted = 1; > > if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel()) Once upon a time there was a good reason to add this: 358bdca5beb733c983bd5e7406c7893e19e9600b What have changed? -- Gleb. -- 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
Glauber Costa wrote: > On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote: >> Glauber Costa wrote: >>> On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote: >>>> Glauber Costa wrote: >>>>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote: >>>>>> Glauber Costa wrote: >>>>>>> This is not kvm specific, and should do fine in plain qemu >>>>>> This is fine with plain qemu already. The problem, IIUC, is that >>>>>> in-kernel kvm irqchip does not have a chance to remove the halted state >>>>>> again. Did you test the effect of this patch on that scenario? What >>>>>> makes it safe to be removed now? >>>>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of >>>>> the vcpu initialization. >>>>> >>>>> It is tested here with in-kernel irqchip and works, so probably not >>>>> a problem, unless you can spot something. >>>> At least your patch applied alone breaks -smp >1 here. >>>> >>>> But the whole management of env->halted for the in-kernel irqchip in >>>> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be >>>> nice to always see a consistent halted in user space, specifically for >>>> debugging purposes. >>> out of curiosity: did you apply the whole series? >> Meanwhile I did, but it makes no difference. > Jan, can you be more specific on why it breaks? I'm double trying it > here, and it works for me just fine. > It locked up in the BIOS after reset from a booted SMP guest, somewhere before selecting the boot device. Jan
diff --git a/hw/apic.c b/hw/apic.c index 86aa6b6..2eddba0 100644 --- a/hw/apic.c +++ b/hw/apic.c @@ -467,8 +467,7 @@ static void apic_init_ipi(APICState *s) cpu_reset(s->cpu_env); - if (!(s->apicbase & MSR_IA32_APICBASE_BSP) && - (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel())) + if (!(s->apicbase & MSR_IA32_APICBASE_BSP)) s->cpu_env->halted = 1; if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel())
This is not kvm specific, and should do fine in plain qemu Signed-off-by: Glauber Costa <glommer@redhat.com> --- hw/apic.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)