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

[Paolo Bonzini <pbonzini@redhat.com>]

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.
> 
> 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)

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

Paolo