[BUG] No irqchip created after commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")

[BUG] No irqchip created after commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")

[Vitaly Kuznetsov]

I apologize if this was already reported,

I just noticed that with the latest updates QEMU doesn't start with the
following configuration:

qemu-system-x86_64 -name guest=win10 -machine pc,accel=kvm -cpu host,hv_vpindex,hv_synic ...

qemu-system-x86_64: failed to turn on HyperV SynIC in KVM: Invalid argument
qemu-system-x86_64: kvm_init_vcpu failed: Invalid argument

If I add 'kernel-irqchip=split' or ',kernel-irqchip=on' it starts as
usual. I bisected this to the following commit:

commit 11bc4a13d1f4b07dafbd1dda4d4bf0fdd7ad65f2 (HEAD, refs/bisect/bad)
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Nov 13 10:56:53 2019 +0100

    kvm: convert "-machine kernel_irqchip" to an accelerator property
    
so aparently we now default to 'kernel_irqchip=off'. Is this the desired
behavior?

-- 
Vitaly

Re: [BUG] No irqchip created after commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

No, absolutely not. I was sure I had tested it, but I will take a look.

Paolo

Il ven 20 dic 2019, 15:11 Vitaly Kuznetsov <vkuznets@redhat.com> ha scritto:

> I apologize if this was already reported,
>
> I just noticed that with the latest updates QEMU doesn't start with the
> following configuration:
>
> qemu-system-x86_64 -name guest=win10 -machine pc,accel=kvm -cpu
> host,hv_vpindex,hv_synic ...
>
> qemu-system-x86_64: failed to turn on HyperV SynIC in KVM: Invalid argument
> qemu-system-x86_64: kvm_init_vcpu failed: Invalid argument
>
> If I add 'kernel-irqchip=split' or ',kernel-irqchip=on' it starts as
> usual. I bisected this to the following commit:
>
> commit 11bc4a13d1f4b07dafbd1dda4d4bf0fdd7ad65f2 (HEAD, refs/bisect/bad)
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Wed Nov 13 10:56:53 2019 +0100
>
>     kvm: convert "-machine kernel_irqchip" to an accelerator property
>
> so aparently we now default to 'kernel_irqchip=off'. Is this the desired
> behavior?
>
> --
> Vitaly
>
>

[-- Attachment #2: Type: text/html, Size: 1590 bytes --]

[PATCH] accel/kvm: Make "kernel_irqchip" default on

[Xiaoyao Li]

Commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
accelerator property") moves kernel_irqchip property from "-machine" to
"-accel kvm", but it forgets to set the default value of
kernel_irqchip_allowed and kernel_irqchip_split.

Also cleaning up the three useless members (kernel_irqchip_allowed,
kernel_irqchip_required, kernel_irqchip_split) in struct MachineState.

Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 accel/kvm/kvm-all.c | 3 +++
 include/hw/boards.h | 3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b2f1a5bcb5ef..40f74094f8d3 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3044,8 +3044,11 @@ bool kvm_kernel_irqchip_split(void)
 static void kvm_accel_instance_init(Object *obj)
 {
     KVMState *s = KVM_STATE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(current_machine);
 
     s->kvm_shadow_mem = -1;
+    s->kernel_irqchip_allowed = true;
+    s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
 }
 
 static void kvm_accel_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 61f8bb8e5a42..fb1b43d5b972 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -271,9 +271,6 @@ struct MachineState {
 
     /*< public >*/
 
-    bool kernel_irqchip_allowed;
-    bool kernel_irqchip_required;
-    bool kernel_irqchip_split;
     char *dtb;
     char *dumpdtb;
     int phandle_start;
-- 
2.19.1

Re: [PATCH] accel/kvm: Make "kernel_irqchip" default on

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

Il sab 28 dic 2019, 09:48 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:

> Commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> accelerator property") moves kernel_irqchip property from "-machine" to
> "-accel kvm", but it forgets to set the default value of
> kernel_irqchip_allowed and kernel_irqchip_split.
>
> Also cleaning up the three useless members (kernel_irqchip_allowed,
> kernel_irqchip_required, kernel_irqchip_split) in struct MachineState.
>
> Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> accelerator property")
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>

Please also add a Reported-by line for Vitaly Kuznetsov.

---
>  accel/kvm/kvm-all.c | 3 +++
>  include/hw/boards.h | 3 ---
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b2f1a5bcb5ef..40f74094f8d3 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3044,8 +3044,11 @@ bool kvm_kernel_irqchip_split(void)
>  static void kvm_accel_instance_init(Object *obj)
>  {
>      KVMState *s = KVM_STATE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(current_machine);
>
>      s->kvm_shadow_mem = -1;
> +    s->kernel_irqchip_allowed = true;
> +    s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
>

Can you initialize this from the init_machine method instead of assuming
that current_machine has been initialized earlier?

Thanks for the quick fix!

Paolo

>  }
>
>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 61f8bb8e5a42..fb1b43d5b972 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -271,9 +271,6 @@ struct MachineState {
>
>      /*< public >*/
>
> -    bool kernel_irqchip_allowed;
> -    bool kernel_irqchip_required;
> -    bool kernel_irqchip_split;
>      char *dtb;
>      char *dumpdtb;
>      int phandle_start;
> --
> 2.19.1
>
>

[-- Attachment #2: Type: text/html, Size: 3128 bytes --]

Re: [PATCH] accel/kvm: Make "kernel_irqchip" default on

[Xiaoyao Li]

On Sat, 2019-12-28 at 10:02 +0000, Paolo Bonzini wrote:
> 
> 
> Il sab 28 dic 2019, 09:48 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:
> > Commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> > accelerator property") moves kernel_irqchip property from "-machine" to
> > "-accel kvm", but it forgets to set the default value of
> > kernel_irqchip_allowed and kernel_irqchip_split.
> > 
> > Also cleaning up the three useless members (kernel_irqchip_allowed,
> > kernel_irqchip_required, kernel_irqchip_split) in struct MachineState.
> > 
> > Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> > accelerator property")
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> Please also add a Reported-by line for Vitaly Kuznetsov.

Sure.

> > ---
> >  accel/kvm/kvm-all.c | 3 +++
> >  include/hw/boards.h | 3 ---
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index b2f1a5bcb5ef..40f74094f8d3 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -3044,8 +3044,11 @@ bool kvm_kernel_irqchip_split(void)
> >  static void kvm_accel_instance_init(Object *obj)
> >  {
> >      KVMState *s = KVM_STATE(obj);
> > +    MachineClass *mc = MACHINE_GET_CLASS(current_machine);
> > 
> >      s->kvm_shadow_mem = -1;
> > +    s->kernel_irqchip_allowed = true;
> > +    s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
> 
> Can you initialize this from the init_machine method instead of assuming that
> current_machine has been initialized earlier?

OK, will do it in v2.

> Thanks for the quick fix!

BTW, it seems that this patch makes kernel_irqchip default on to workaround the
bug. 
However, when explicitly configuring kernel_irqchip=off, guest still fails
booting due to "KVM: failed to send PV IPI: -95" with a latest upstream kernel
ubuntu guest. Any idea about this? 

> Paolo
> >  }
> > 
> >  static void kvm_accel_class_init(ObjectClass *oc, void *data)
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 61f8bb8e5a42..fb1b43d5b972 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -271,9 +271,6 @@ struct MachineState {
> > 
> >      /*< public >*/
> > 
> > -    bool kernel_irqchip_allowed;
> > -    bool kernel_irqchip_required;
> > -    bool kernel_irqchip_split;
> >      char *dtb;
> >      char *dumpdtb;
> >      int phandle_start;

Re: [PATCH] accel/kvm: Make "kernel_irqchip" default on

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

Il sab 28 dic 2019, 10:24 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:

> BTW, it seems that this patch makes kernel_irqchip default on to
> workaround the
> bug.
> However, when explicitly configuring kernel_irqchip=off, guest still fails
> booting due to "KVM: failed to send PV IPI: -95" with a latest upstream
> kernel
> ubuntu guest. Any idea about this?
>

We need to clear the PV IPI feature for userspace irqchip. Are you using
-cpu host by chance?

Paolo


> > Paolo
> > >  }
> > >
> > >  static void kvm_accel_class_init(ObjectClass *oc, void *data)
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index 61f8bb8e5a42..fb1b43d5b972 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -271,9 +271,6 @@ struct MachineState {
> > >
> > >      /*< public >*/
> > >
> > > -    bool kernel_irqchip_allowed;
> > > -    bool kernel_irqchip_required;
> > > -    bool kernel_irqchip_split;
> > >      char *dtb;
> > >      char *dumpdtb;
> > >      int phandle_start;
>
>

[-- Attachment #2: Type: text/html, Size: 1799 bytes --]

Re: [PATCH] accel/kvm: Make "kernel_irqchip" default on

[Xiaoyao Li]

On Sat, 2019-12-28 at 10:57 +0000, Paolo Bonzini wrote:
> 
> 
> Il sab 28 dic 2019, 10:24 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:
> > BTW, it seems that this patch makes kernel_irqchip default on to workaround
> > the
> > bug. 
> > However, when explicitly configuring kernel_irqchip=off, guest still fails
> > booting due to "KVM: failed to send PV IPI: -95" with a latest upstream
> > kernel
> > ubuntu guest. Any idea about this? 
> 
> We need to clear the PV IPI feature for userspace irqchip. Are you using -cpu
> host by chance?

Yes, I used -cpu host.

After using "-cpu host,-kvm-pv-ipi" with kernel_irqchip=off, it can boot
successfully.

> Paolo
> 
> > > Paolo
> > > >  }
> > > > 
> > > >  static void kvm_accel_class_init(ObjectClass *oc, void *data)
> > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > index 61f8bb8e5a42..fb1b43d5b972 100644
> > > > --- a/include/hw/boards.h
> > > > +++ b/include/hw/boards.h
> > > > @@ -271,9 +271,6 @@ struct MachineState {
> > > > 
> > > >      /*< public >*/
> > > > 
> > > > -    bool kernel_irqchip_allowed;
> > > > -    bool kernel_irqchip_required;
> > > > -    bool kernel_irqchip_split;
> > > >      char *dtb;
> > > >      char *dumpdtb;
> > > >      int phandle_start;
> >