[Qemu-devel] [PATCH 0/4] q35: change defaults for kernel irqchip and IR

[Qemu-devel] [PATCH 0/4] q35: change defaults for kernel irqchip and IR

[Peter Xu]

This only changes q35.  Nothing else.

Before this series, we have these default parameters:

  - machine kernel-irqchip: on
  - intel-iommu IR: off

This series wants to change these default variables into:

  - machine kernel-irqchip: split
  - intel-iommu IR: on

and at the meantime we should keep compatibility with old kernels and
old versions of QEMU.

For old versions of QEMU: we used machine compat properties.

For old kernels (<4.4): if user didn't specify split kernel irqchip,
we'll take it only as the first priority if it's supported by the
kernel; otherwise, we will continue with complete kernel irqchip.

Both of these parameters should be good to have (split irqchip gains
more security, while IR gets it too but even more, like x2apic).  So
let's try to make them as default if capable.

Tests ("split" stands for whether kernel split irqchip enabled, "IR"
stands for whether IR is turned on):

   |--------------------------------------------------------+-------+----|
   | params                                                 | split | IR |
   |--------------------------------------------------------+-------+----|
   | -M q35                                                 |     0 | /  |
   | -M q35,accel=kvm                                       |     1 | /  |
   | -M pc-q35-3.1,accel=kvm                                |     0 | /  |
   | -M q35,accel=kvm,kernel-irqchip=off                    |     0 | /  |
   | -M q35,accel=kvm,kernel-irqchip=on                     |     0 | /  |
   | -M q35 -device intel-iommu                             |     0 | 1  |
   | -M q35,accel=kvm -device intel-iommu                   |     1 | 1  |
   | -M q35,accel=kvm,kernel-irqchip=on -device intel-iommu |     0 | 0  |
   |--------------------------------------------------------+-------+----|

I didn't try old kernels, though.

Any comment would be welcomed, thanks.

Peter Xu (4):
  kvm: let split be optional for kvm_arch_irqchip_create
  q35: set split kernel irqchip as default
  x86-iommu: switch intr_supported to OnOffAuto type
  x86-iommu: turn on IR by default if proper

 accel/kvm/kvm-all.c         |  3 ++-
 hw/core/machine.c           |  2 ++
 hw/i386/acpi-build.c        |  6 +++---
 hw/i386/amd_iommu.c         |  2 +-
 hw/i386/intel_iommu.c       |  6 +++---
 hw/i386/pc.c                |  2 +-
 hw/i386/pc_q35.c            |  2 ++
 hw/i386/x86-iommu.c         | 18 +++++++++++++++---
 include/hw/boards.h         |  1 +
 include/hw/i386/x86-iommu.h |  4 +++-
 target/i386/kvm.c           |  6 +++---
 11 files changed, 36 insertions(+), 16 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/4] kvm: let split be optional for kvm_arch_irqchip_create

[Peter Xu]

This patch allows the kvm_arch_irqchip_create() to return 0 if the
split irqchip is specified but not forced by the user.  Also, modify
kvm_irqchip_create() similiarly.

This patch should have no functional change for existing code since
currently if split is specified it must be forced by the user so we'll
always have machine_kernel_irqchip_required() returns true. However it
could potentially be used in follow up patches when we want to turn
split kernel irqchip as default for QEMU 4.0 which could trigger the
case that kernel_irqchip_required=N while kernel_irqchip_split=Y. When
with that, we'll first try with split irqchip, and falls back to
normal kernel irqchip when split capability is not provided by the
kernel.

This brings us benefit that we can even run a default QEMU 4.0 on old
kernels that does not support split irqchip (<4.4) but at the same
time enable split irqchip for new kernels (>=4.4) as default.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/kvm/kvm-all.c | 3 ++-
 target/i386/kvm.c   | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 4880a05399..b008364041 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1468,7 +1468,8 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s)
      * in-kernel irqchip for us */
     ret = kvm_arch_irqchip_create(machine, s);
     if (ret == 0) {
-        if (machine_kernel_irqchip_split(machine)) {
+        if (machine_kernel_irqchip_required(machine) &&
+            machine_kernel_irqchip_split(machine)) {
             perror("Split IRQ chip mode not supported.");
             exit(1);
         } else {
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 739cf8c8ea..8f919f8f9f 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -3685,9 +3685,9 @@ int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
     if (machine_kernel_irqchip_split(ms)) {
         ret = kvm_vm_enable_cap(s, KVM_CAP_SPLIT_IRQCHIP, 0, 24);
         if (ret) {
-            error_report("Could not enable split irqchip mode: %s",
-                         strerror(-ret));
-            exit(1);
+            assert(ret < 0);
+            /* If split not required, return 0 instead to retry */
+            return machine_kernel_irqchip_required(ms) ? ret : 0;
         } else {
             DPRINTF("Enabled KVM_CAP_SPLIT_IRQCHIP\n");
             kvm_split_irqchip = true;
-- 
2.17.1

[Qemu-devel] [PATCH 2/4] q35: set split kernel irqchip as default

[Peter Xu]

Starting from QEMU 4.0, let's specify "split" as the default value for
kernel-irqchip.

So for QEMU>=4.0 we'll have: allowed=Y,required=N,split=Y
   for QEMU<=3.1 we'll have: allowed=Y,required=N,split=N
   (omitting all the "kernel_irqchip_" prefix)

Note that this "split" is optional - we'll first try to enable split
kernel irqchip, and we'll fall back to complete kernel irqchip if we
found that the kernel capability is missing.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c   | 2 ++
 hw/i386/pc_q35.c    | 2 ++
 include/hw/boards.h | 1 +
 3 files changed, 5 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c51423b647..4439ea663f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -653,8 +653,10 @@ static void machine_class_base_init(ObjectClass *oc, void *data)
 static void machine_initfn(Object *obj)
 {
     MachineState *ms = MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
 
     ms->kernel_irqchip_allowed = true;
+    ms->kernel_irqchip_split = mc->default_kernel_irqchip_split;
     ms->kvm_shadow_mem = -1;
     ms->dump_guest_core = true;
     ms->mem_merge = true;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 58459bdab5..d2fb0fa49f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -304,6 +304,7 @@ static void pc_q35_machine_options(MachineClass *m)
     m->units_per_default_bus = 1;
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
+    m->default_kernel_irqchip_split = true;
     m->no_floppy = 1;
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
@@ -323,6 +324,7 @@ DEFINE_Q35_MACHINE(v4_0, "pc-q35-4.0", NULL,
 static void pc_q35_3_1_machine_options(MachineClass *m)
 {
     pc_q35_4_0_machine_options(m);
+    m->default_kernel_irqchip_split = false;
     m->alias = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_3_1);
 }
diff --git a/include/hw/boards.h b/include/hw/boards.h
index f82f28468b..362384815e 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -195,6 +195,7 @@ struct MachineClass {
     const char *hw_version;
     ram_addr_t default_ram_size;
     const char *default_cpu_type;
+    bool default_kernel_irqchip_split;
     bool option_rom_has_mr;
     bool rom_file_has_mr;
     int minimum_page_bits;
-- 
2.17.1

[Qemu-devel] [PATCH 3/4] x86-iommu: switch intr_supported to OnOffAuto type

[Peter Xu]

Switch the intr_supported variable from a boolean to OnOffAuto type so
that we can know whether the user specified it or not.  With that
we'll have a chance to help the user to choose more wisely where
possible.  Introduce x86_iommu_ir_supported() to mask these changes.

No functional change at all.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/acpi-build.c        |  6 +++---
 hw/i386/amd_iommu.c         |  2 +-
 hw/i386/intel_iommu.c       |  6 +++---
 hw/i386/pc.c                |  2 +-
 hw/i386/x86-iommu.c         | 15 +++++++++++++--
 include/hw/i386/x86-iommu.h |  4 +++-
 6 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 236a20eaa8..7012f97cac 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2426,7 +2426,7 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker)
     IntelIOMMUState *intel_iommu = INTEL_IOMMU_DEVICE(iommu);
 
     assert(iommu);
-    if (iommu->intr_supported) {
+    if (x86_iommu_ir_supported(iommu)) {
         dmar_flags |= 0x1;      /* Flags: 0x1: INT_REMAP */
     }
 
@@ -2499,7 +2499,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
      * When interrupt remapping is supported, we add a special IVHD device
      * for type IO-APIC.
      */
-    if (x86_iommu_get_default()->intr_supported) {
+    if (x86_iommu_ir_supported(x86_iommu_get_default())) {
         ivhd_table_len += 8;
     }
     /* IVHD length */
@@ -2535,7 +2535,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
      * Linux IOMMU driver checks for the special IVHD device (type IO-APIC).
      * See Linux kernel commit 'c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059'
      */
-    if (x86_iommu_get_default()->intr_supported) {
+    if (x86_iommu_ir_supported(x86_iommu_get_default())) {
         build_append_int_noprefix(table_data,
                                  (0x1ull << 56) |           /* type IOAPIC */
                                  (IOAPIC_SB_DEVID << 40) |  /* IOAPIC devid */
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 353a810e6b..8ad707aba0 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1233,7 +1233,7 @@ static int amdvi_int_remap_msi(AMDVIState *iommu,
     }
 
     /* validate that we are configure with intremap=on */
-    if (!X86_IOMMU_DEVICE(iommu)->intr_supported) {
+    if (!x86_iommu_ir_supported(X86_IOMMU_DEVICE(iommu))) {
         trace_amdvi_err("Interrupt remapping is enabled in the guest but "
                         "not in the host. Use intremap=on to enable interrupt "
                         "remapping in amd-iommu.");
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index d97bcbc2f7..3df4b0a550 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3137,7 +3137,7 @@ static void vtd_init(IntelIOMMUState *s)
     vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits);
     vtd_paging_entry_rsvd_field[8] = VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
 
-    if (x86_iommu->intr_supported) {
+    if (x86_iommu_ir_supported(x86_iommu)) {
         s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
         if (s->intr_eim == ON_OFF_AUTO_ON) {
             s->ecap |= VTD_ECAP_EIM;
@@ -3238,14 +3238,14 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
 {
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
 
-    if (s->intr_eim == ON_OFF_AUTO_ON && !x86_iommu->intr_supported) {
+    if (s->intr_eim == ON_OFF_AUTO_ON && !x86_iommu_ir_supported(x86_iommu)) {
         error_setg(errp, "eim=on cannot be selected without intremap=on");
         return false;
     }
 
     if (s->intr_eim == ON_OFF_AUTO_AUTO) {
         s->intr_eim = (kvm_irqchip_in_kernel() || s->buggy_eim)
-                      && x86_iommu->intr_supported ?
+                      && x86_iommu_ir_supported(x86_iommu) ?
                                               ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
     }
     if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 115bc2825c..d95a0e3ad1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1244,7 +1244,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     if (pcms->apic_id_limit > 255 && !xen_enabled()) {
         IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
 
-        if (!iommu || !iommu->x86_iommu.intr_supported ||
+        if (!iommu || !x86_iommu_ir_supported(X86_IOMMU_DEVICE(iommu)) ||
             iommu->intr_eim != ON_OFF_AUTO_ON) {
             error_report("current -smp configuration requires "
                          "Extended Interrupt Mode enabled. "
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index abc3c03158..61ee0f1eaa 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -119,8 +119,13 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    /* If the user didn't specify IR, choose a default value for it */
+    if (x86_iommu->intr_supported == ON_OFF_AUTO_AUTO) {
+        x86_iommu->intr_supported = ON_OFF_AUTO_OFF;
+    }
+
     /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" */
-    if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
+    if (x86_iommu_ir_supported(x86_iommu) && kvm_irqchip_in_kernel() &&
         !kvm_irqchip_is_split()) {
         error_setg(errp, "Interrupt Remapping cannot work with "
                          "kernel-irqchip=on, please use 'split|off'.");
@@ -135,7 +140,8 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
 }
 
 static Property x86_iommu_properties[] = {
-    DEFINE_PROP_BOOL("intremap", X86IOMMUState, intr_supported, false),
+    DEFINE_PROP_ON_OFF_AUTO("intremap", X86IOMMUState,
+                            intr_supported, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
     DEFINE_PROP_BOOL("pt", X86IOMMUState, pt_supported, true),
     DEFINE_PROP_END_OF_LIST(),
@@ -148,6 +154,11 @@ static void x86_iommu_class_init(ObjectClass *klass, void *data)
     dc->props = x86_iommu_properties;
 }
 
+bool x86_iommu_ir_supported(X86IOMMUState *s)
+{
+    return s->intr_supported == ON_OFF_AUTO_ON;
+}
+
 static const TypeInfo x86_iommu_info = {
     .name          = TYPE_X86_IOMMU_DEVICE,
     .parent        = TYPE_SYS_BUS_DEVICE,
diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
index 2b22a579a3..dcd9719a2c 100644
--- a/include/hw/i386/x86-iommu.h
+++ b/include/hw/i386/x86-iommu.h
@@ -74,13 +74,15 @@ typedef struct IEC_Notifier IEC_Notifier;
 
 struct X86IOMMUState {
     SysBusDevice busdev;
-    bool intr_supported;        /* Whether vIOMMU supports IR */
+    OnOffAuto intr_supported;   /* Whether vIOMMU supports IR */
     bool dt_supported;          /* Whether vIOMMU supports DT */
     bool pt_supported;          /* Whether vIOMMU supports pass-through */
     IommuType type;             /* IOMMU type - AMD/Intel     */
     QLIST_HEAD(, IEC_Notifier) iec_notifiers; /* IEC notify list */
 };
 
+bool x86_iommu_ir_supported(X86IOMMUState *s);
+
 /* Generic IRQ entry information when interrupt remapping is enabled */
 struct X86IOMMUIrq {
     /* Used by both IOAPIC/MSI interrupt remapping */
-- 
2.17.1

[Qemu-devel] [PATCH 4/4] x86-iommu: turn on IR by default if proper

[Peter Xu]

When the user didn't specify "intremap" for the IOMMU device, we turn
it on by default if it is supported.  This will turn IR on for the
default Q35 platform as long as the IOMMU device is specified on new
kernels.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/x86-iommu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 61ee0f1eaa..d1534c1ae0 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -112,6 +112,7 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
     PCMachineState *pcms =
         PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
     QLIST_INIT(&x86_iommu->iec_notifiers);
+    bool irq_all_kernel = kvm_irqchip_in_kernel() && !kvm_irqchip_is_split();
 
     if (!pcms || !pcms->bus) {
         error_setg(errp, "Machine-type '%s' not supported by IOMMU",
@@ -121,12 +122,12 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
 
     /* If the user didn't specify IR, choose a default value for it */
     if (x86_iommu->intr_supported == ON_OFF_AUTO_AUTO) {
-        x86_iommu->intr_supported = ON_OFF_AUTO_OFF;
+        x86_iommu->intr_supported = irq_all_kernel ?
+            ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
     }
 
     /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" */
-    if (x86_iommu_ir_supported(x86_iommu) && kvm_irqchip_in_kernel() &&
-        !kvm_irqchip_is_split()) {
+    if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
         error_setg(errp, "Interrupt Remapping cannot work with "
                          "kernel-irqchip=on, please use 'split|off'.");
         return;
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/4] q35: set split kernel irqchip as default

[Michael S. Tsirkin]

On Wed, Dec 19, 2018 at 04:50:36PM +0800, Peter Xu wrote:
> Starting from QEMU 4.0, let's specify "split" as the default value for
> kernel-irqchip.
> 
> So for QEMU>=4.0 we'll have: allowed=Y,required=N,split=Y
>    for QEMU<=3.1 we'll have: allowed=Y,required=N,split=N
>    (omitting all the "kernel_irqchip_" prefix)
> 
> Note that this "split" is optional - we'll first try to enable split
> kernel irqchip, and we'll fall back to complete kernel irqchip if we
> found that the kernel capability is missing.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

I'm split on this one ;)
On the one hand why not make pc and q35 are consistent here?
On the other hand we really should work to leave PC alone
as much as we can ...

Paolo, what's your opinion?


> ---
>  hw/core/machine.c   | 2 ++
>  hw/i386/pc_q35.c    | 2 ++
>  include/hw/boards.h | 1 +
>  3 files changed, 5 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c51423b647..4439ea663f 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -653,8 +653,10 @@ static void machine_class_base_init(ObjectClass *oc, void *data)
>  static void machine_initfn(Object *obj)
>  {
>      MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
>  
>      ms->kernel_irqchip_allowed = true;
> +    ms->kernel_irqchip_split = mc->default_kernel_irqchip_split;
>      ms->kvm_shadow_mem = -1;
>      ms->dump_guest_core = true;
>      ms->mem_merge = true;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 58459bdab5..d2fb0fa49f 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -304,6 +304,7 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->units_per_default_bus = 1;
>      m->default_machine_opts = "firmware=bios-256k.bin";
>      m->default_display = "std";
> +    m->default_kernel_irqchip_split = true;
>      m->no_floppy = 1;
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
> @@ -323,6 +324,7 @@ DEFINE_Q35_MACHINE(v4_0, "pc-q35-4.0", NULL,
>  static void pc_q35_3_1_machine_options(MachineClass *m)
>  {
>      pc_q35_4_0_machine_options(m);
> +    m->default_kernel_irqchip_split = false;
>      m->alias = NULL;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_3_1);
>  }
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index f82f28468b..362384815e 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -195,6 +195,7 @@ struct MachineClass {
>      const char *hw_version;
>      ram_addr_t default_ram_size;
>      const char *default_cpu_type;
> +    bool default_kernel_irqchip_split;
>      bool option_rom_has_mr;
>      bool rom_file_has_mr;
>      int minimum_page_bits;
> -- 
> 2.17.1

Re: [Qemu-devel] [PATCH 1/4] kvm: let split be optional for kvm_arch_irqchip_create

[Michael S. Tsirkin]

On Wed, Dec 19, 2018 at 04:50:35PM +0800, Peter Xu wrote:
> This patch allows the kvm_arch_irqchip_create() to return 0 if the
> split irqchip is specified but not forced by the user.  Also, modify
> kvm_irqchip_create() similiarly.
> 
> This patch should have no functional change for existing code since
> currently if split is specified it must be forced by the user so we'll
> always have machine_kernel_irqchip_required() returns true. However it
> could potentially be used in follow up patches when we want to turn
> split kernel irqchip as default for QEMU 4.0 which could trigger the
> case that kernel_irqchip_required=N while kernel_irqchip_split=Y. When
> with that, we'll first try with split irqchip, and falls back to
> normal kernel irqchip when split capability is not provided by the
> kernel.
> 
> This brings us benefit that we can even run a default QEMU 4.0 on old
> kernels that does not support split irqchip (<4.4) but at the same
> time enable split irqchip for new kernels (>=4.4) as default.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Paolo, if you can ack this one, I can merge the rest.

> ---
>  accel/kvm/kvm-all.c | 3 ++-
>  target/i386/kvm.c   | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 4880a05399..b008364041 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1468,7 +1468,8 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s)
>       * in-kernel irqchip for us */
>      ret = kvm_arch_irqchip_create(machine, s);
>      if (ret == 0) {
> -        if (machine_kernel_irqchip_split(machine)) {
> +        if (machine_kernel_irqchip_required(machine) &&
> +            machine_kernel_irqchip_split(machine)) {
>              perror("Split IRQ chip mode not supported.");
>              exit(1);
>          } else {
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 739cf8c8ea..8f919f8f9f 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -3685,9 +3685,9 @@ int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
>      if (machine_kernel_irqchip_split(ms)) {
>          ret = kvm_vm_enable_cap(s, KVM_CAP_SPLIT_IRQCHIP, 0, 24);
>          if (ret) {
> -            error_report("Could not enable split irqchip mode: %s",
> -                         strerror(-ret));
> -            exit(1);
> +            assert(ret < 0);
> +            /* If split not required, return 0 instead to retry */
> +            return machine_kernel_irqchip_required(ms) ? ret : 0;
>          } else {
>              DPRINTF("Enabled KVM_CAP_SPLIT_IRQCHIP\n");
>              kvm_split_irqchip = true;
> -- 
> 2.17.1

Re: [Qemu-devel] [PATCH 2/4] q35: set split kernel irqchip as default

[Paolo Bonzini]

On 19/12/18 09:50, Peter Xu wrote:
> Starting from QEMU 4.0, let's specify "split" as the default value for
> kernel-irqchip.
> 
> So for QEMU>=4.0 we'll have: allowed=Y,required=N,split=Y
>    for QEMU<=3.1 we'll have: allowed=Y,required=N,split=N
>    (omitting all the "kernel_irqchip_" prefix)
> 
> Note that this "split" is optional - we'll first try to enable split
> kernel irqchip, and we'll fall back to complete kernel irqchip if we
> found that the kernel capability is missing.

Please just fail completely and require a new kernel for the 4.0 machine
type.  There are subtle differences between kernel and QEMU irqchip, I
don't think we want to open that can of worms.

Paolo

Re: [Qemu-devel] [PATCH 1/4] kvm: let split be optional for kvm_arch_irqchip_create

[Paolo Bonzini]

On 19/12/18 16:53, Michael S. Tsirkin wrote:
> On Wed, Dec 19, 2018 at 04:50:35PM +0800, Peter Xu wrote:
>> This patch allows the kvm_arch_irqchip_create() to return 0 if the
>> split irqchip is specified but not forced by the user.  Also, modify
>> kvm_irqchip_create() similiarly.
>>
>> This patch should have no functional change for existing code since
>> currently if split is specified it must be forced by the user so we'll
>> always have machine_kernel_irqchip_required() returns true. However it
>> could potentially be used in follow up patches when we want to turn
>> split kernel irqchip as default for QEMU 4.0 which could trigger the
>> case that kernel_irqchip_required=N while kernel_irqchip_split=Y. When
>> with that, we'll first try with split irqchip, and falls back to
>> normal kernel irqchip when split capability is not provided by the
>> kernel.
>>
>> This brings us benefit that we can even run a default QEMU 4.0 on old
>> kernels that does not support split irqchip (<4.4) but at the same
>> time enable split irqchip for new kernels (>=4.4) as default.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Paolo, if you can ack this one, I can merge the rest.

If I understand the code well, there is no change needed in the rest of
the code; having the semantics I asked for simply requires dropping this
patch.

However, the commit messages need some adjustment.

Paolo

> 
>> ---
>>  accel/kvm/kvm-all.c | 3 ++-
>>  target/i386/kvm.c   | 6 +++---
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 4880a05399..b008364041 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -1468,7 +1468,8 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s)
>>       * in-kernel irqchip for us */
>>      ret = kvm_arch_irqchip_create(machine, s);
>>      if (ret == 0) {
>> -        if (machine_kernel_irqchip_split(machine)) {
>> +        if (machine_kernel_irqchip_required(machine) &&
>> +            machine_kernel_irqchip_split(machine)) {
>>              perror("Split IRQ chip mode not supported.");
>>              exit(1);
>>          } else {
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 739cf8c8ea..8f919f8f9f 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -3685,9 +3685,9 @@ int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
>>      if (machine_kernel_irqchip_split(ms)) {
>>          ret = kvm_vm_enable_cap(s, KVM_CAP_SPLIT_IRQCHIP, 0, 24);
>>          if (ret) {
>> -            error_report("Could not enable split irqchip mode: %s",
>> -                         strerror(-ret));
>> -            exit(1);
>> +            assert(ret < 0);
>> +            /* If split not required, return 0 instead to retry */
>> +            return machine_kernel_irqchip_required(ms) ? ret : 0;
>>          } else {
>>              DPRINTF("Enabled KVM_CAP_SPLIT_IRQCHIP\n");
>>              kvm_split_irqchip = true;
>> -- 
>> 2.17.1

Re: [Qemu-devel] [PATCH 2/4] q35: set split kernel irqchip as default

[Paolo Bonzini]

On 19/12/18 16:52, Michael S. Tsirkin wrote:
> On Wed, Dec 19, 2018 at 04:50:36PM +0800, Peter Xu wrote:
>> Starting from QEMU 4.0, let's specify "split" as the default value for
>> kernel-irqchip.
>>
>> So for QEMU>=4.0 we'll have: allowed=Y,required=N,split=Y
>>    for QEMU<=3.1 we'll have: allowed=Y,required=N,split=N
>>    (omitting all the "kernel_irqchip_" prefix)
>>
>> Note that this "split" is optional - we'll first try to enable split
>> kernel irqchip, and we'll fall back to complete kernel irqchip if we
>> found that the kernel capability is missing.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> I'm split on this one ;)
> On the one hand why not make pc and q35 are consistent here?
> On the other hand we really should work to leave PC alone
> as much as we can ...
> 
> Paolo, what's your opinion?

The idea was to avoid bumping the minimal kernel version for PC, only
for Q35.  PC can still use split irqchip for security purposes, but only
Q35 needs it for features.

Paolo

Re: [Qemu-devel] [PATCH 2/4] q35: set split kernel irqchip as default

[Eduardo Habkost]

On Wed, Dec 19, 2018 at 09:12:55PM +0100, Paolo Bonzini wrote:
> On 19/12/18 09:50, Peter Xu wrote:
> > Starting from QEMU 4.0, let's specify "split" as the default value for
> > kernel-irqchip.
> > 
> > So for QEMU>=4.0 we'll have: allowed=Y,required=N,split=Y
> >    for QEMU<=3.1 we'll have: allowed=Y,required=N,split=N
> >    (omitting all the "kernel_irqchip_" prefix)
> > 
> > Note that this "split" is optional - we'll first try to enable split
> > kernel irqchip, and we'll fall back to complete kernel irqchip if we
> > found that the kernel capability is missing.
> 
> Please just fail completely and require a new kernel for the 4.0 machine
> type.  There are subtle differences between kernel and QEMU irqchip, I
> don't think we want to open that can of worms.

This would make existing VMs that are runnable with pc-q35-3.1.0
not runnable by only updating the machine-type.

The good news is that we can make this a non-issue by clearly
documenting that QEMU needs a more recent kernel (just like we'll
do for RDTSCP[1]).

[1] https://lore.kernel.org/lkml/20181210181328.GA762@zn.tnic/

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] q35: set split kernel irqchip as default

[Paolo Bonzini]

On 19/12/18 22:24, Eduardo Habkost wrote:
> On Wed, Dec 19, 2018 at 09:12:55PM +0100, Paolo Bonzini wrote:
>> On 19/12/18 09:50, Peter Xu wrote:
>>> Starting from QEMU 4.0, let's specify "split" as the default value for
>>> kernel-irqchip.
>>>
>>> So for QEMU>=4.0 we'll have: allowed=Y,required=N,split=Y
>>>    for QEMU<=3.1 we'll have: allowed=Y,required=N,split=N
>>>    (omitting all the "kernel_irqchip_" prefix)
>>>
>>> Note that this "split" is optional - we'll first try to enable split
>>> kernel irqchip, and we'll fall back to complete kernel irqchip if we
>>> found that the kernel capability is missing.
>>
>> Please just fail completely and require a new kernel for the 4.0 machine
>> type.  There are subtle differences between kernel and QEMU irqchip, I
>> don't think we want to open that can of worms.
> 
> This would make existing VMs that are runnable with pc-q35-3.1.0
> not runnable by only updating the machine-type.
> 
> The good news is that we can make this a non-issue by clearly
> documenting that QEMU needs a more recent kernel (just like we'll
> do for RDTSCP[1]).

Right, RDTSCP is exactly what came to mind.

Paolo

Re: [Qemu-devel] [PATCH 2/4] q35: set split kernel irqchip as default

[Peter Xu]

On Wed, Dec 19, 2018 at 10:45:40PM +0100, Paolo Bonzini wrote:
> On 19/12/18 22:24, Eduardo Habkost wrote:
> > On Wed, Dec 19, 2018 at 09:12:55PM +0100, Paolo Bonzini wrote:
> >> On 19/12/18 09:50, Peter Xu wrote:
> >>> Starting from QEMU 4.0, let's specify "split" as the default value for
> >>> kernel-irqchip.
> >>>
> >>> So for QEMU>=4.0 we'll have: allowed=Y,required=N,split=Y
> >>>    for QEMU<=3.1 we'll have: allowed=Y,required=N,split=N
> >>>    (omitting all the "kernel_irqchip_" prefix)
> >>>
> >>> Note that this "split" is optional - we'll first try to enable split
> >>> kernel irqchip, and we'll fall back to complete kernel irqchip if we
> >>> found that the kernel capability is missing.
> >>
> >> Please just fail completely and require a new kernel for the 4.0 machine
> >> type.  There are subtle differences between kernel and QEMU irqchip, I
> >> don't think we want to open that can of worms.
> > 
> > This would make existing VMs that are runnable with pc-q35-3.1.0
> > not runnable by only updating the machine-type.
> > 
> > The good news is that we can make this a non-issue by clearly
> > documenting that QEMU needs a more recent kernel (just like we'll
> > do for RDTSCP[1]).
> 
> Right, RDTSCP is exactly what came to mind.

Ok so I think I'll just make it even simpler by dropping patch 1.
Also I noticed that the documentation on linux kernel version
requirement has not yet reached master but I'll assume it'll be there
some day very soon so I'll ignore that part.

Thanks everyone!  I'll repost soon.

-- 
Peter Xu