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

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 7 Jan 2020 11:32:42 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 28/12/19 11:43, Xiaoyao Li wrote:
> > 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.

FWIW, this also fixes s390x, which requires kernel_irqchip=on.

> > 
> > Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")
> > Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > ---
> > Changes in v2:
> >   - Add Reported-by tag;
> >   - Initialize kernel_irqchip_split in init_machine();  
> 
> Now that I am actually reviewing the patch on something other than a
> phone, I think this would break "-machine kernel_irqchip=split".  I'll
> test, and squash if it works, something like this:
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index afbbe0a1af..ea35433170 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -98,7 +98,7 @@ struct KVMState
>      int kvm_shadow_mem;
>      bool kernel_irqchip_allowed;
>      bool kernel_irqchip_required;
> -    bool kernel_irqchip_split;
> +    OnOffAuto kernel_irqchip_split;
>      bool sync_mmu;
>      bool manual_dirty_log_protect;
>      /* The man page (and posix) say ioctl numbers are signed int, but
> @@ -1799,7 +1799,7 @@ static void kvm_irqchip_create(KVMState *s)
>       * in-kernel irqchip for us */
>      ret = kvm_arch_irqchip_create(s);
>      if (ret == 0) {
> -        if (s->kernel_irqchip_split) {
> +        if (s->kernel_irqchip_split == ON_OFF_AUTO_ON) {
>              perror("Split IRQ chip mode not supported.");
>              exit(1);
>          } else {
> @@ -2070,7 +2070,9 @@ static int kvm_init(MachineState *ms)
>          goto err;
>      }
>  
> -    s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
> +    if (s->kernel_irqchip_split == ON_OFF_AUTO_AUTO) {
> +        s->kernel_irqchip_split = mc->default_kernel_irqchip_split ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> +    }
>  
>      if (s->kernel_irqchip_allowed) {
>          kvm_irqchip_create(s);
> @@ -3007,17 +3009,17 @@ static void kvm_set_kernel_irqchip(Object *obj, Visitor *v,
>          case ON_OFF_SPLIT_ON:
>              s->kernel_irqchip_allowed = true;
>              s->kernel_irqchip_required = true;
> -            s->kernel_irqchip_split = false;
> +            s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
>              break;
>          case ON_OFF_SPLIT_OFF:
>              s->kernel_irqchip_allowed = false;
>              s->kernel_irqchip_required = false;
> -            s->kernel_irqchip_split = false;
> +            s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
>              break;
>          case ON_OFF_SPLIT_SPLIT:
>              s->kernel_irqchip_allowed = true;
>              s->kernel_irqchip_required = true;
> -            s->kernel_irqchip_split = true;
> +            s->kernel_irqchip_split = ON_OFF_AUTO_ON;
>              break;
>          default:
>              /* The value was checked in visit_type_OnOffSplit() above. If
> @@ -3040,7 +3042,7 @@ bool kvm_kernel_irqchip_required(void)
>  
>  bool kvm_kernel_irqchip_split(void)
>  {
> -    return kvm_state->kernel_irqchip_split;
> +    return kvm_state->kernel_irqchip_split == ON_OFF_AUTO_ON;
>  }
>  
>  static void kvm_accel_instance_init(Object *obj)
> @@ -3049,6 +3051,7 @@ static void kvm_accel_instance_init(Object *obj)
>  
>      s->kvm_shadow_mem = -1;
>      s->kernel_irqchip_allowed = true;
> +    s->kernel_irqchip_split = ON_OFF_AUTO_AUTO;
>  }
>  
>  static void kvm_accel_class_init(ObjectClass *oc, void *data)

These changes looks sane to me (but did not test).

> 
> As a follow up, kernel_irqchip_allowed and kernel_irqchip_required could also be changed to a single OnOffAuto field, I think.

That would probably be a nice cleanup.

> 
> Paolo
> 
>