[Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Alex Williamson]

Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
the default for the pc-q35-4.0 machine type to use split irqchip, which
turned out to have disasterous effects on vfio-pci INTx support.  KVM
resampling irqfds are registered for handling these interrupts, but
these are non-functional in split irqchip mode.  We can't simply test
for split irqchip in QEMU as userspace handling of this interrupt is a
significant performance regression versus KVM handling (GeForce GPUs
assigned to Windows VMs are non-functional without forcing MSI mode or
re-enabling kernel irqchip).

The resolution is to revert the change in default irqchip mode in the
pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
configurations for devices requiring legacy INTx support without
explicitly modifying the VM configuration to use kernel irqchip.

Link: https://bugs.launchpad.net/qemu/+bug/1826422
Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/core/machine.c    |    3 +++
 hw/i386/pc.c         |    3 +++
 hw/i386/pc_q35.c     |   16 ++++++++++++++--
 include/hw/boards.h  |    3 +++
 include/hw/i386/pc.h |    3 +++
 5 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 5d046a43e3d2..e41e6698ac9f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -24,6 +24,9 @@
 #include "hw/pci/pci.h"
 #include "hw/mem/nvdimm.h"
 
+GlobalProperty hw_compat_4_0_1[] = {};
+const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
+
 GlobalProperty hw_compat_4_0[] = {};
 const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d98b737b8f3b..b5311e7e2bd5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -115,6 +115,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 /* Physical Address of PVH entry point read from kernel ELF NOTE */
 static size_t pvh_start_addr;
 
+GlobalProperty pc_compat_4_0_1[] = {};
+const size_t pc_compat_4_0_1_len = G_N_ELEMENTS(pc_compat_4_0_1);
+
 GlobalProperty pc_compat_4_0[] = {};
 const size_t pc_compat_4_0_len = G_N_ELEMENTS(pc_compat_4_0);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 37dd350511a9..dcddc6466200 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -357,7 +357,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->default_kernel_irqchip_split = false;
     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);
@@ -374,10 +374,22 @@ static void pc_q35_4_1_machine_options(MachineClass *m)
 DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
                    pc_q35_4_1_machine_options);
 
-static void pc_q35_4_0_machine_options(MachineClass *m)
+static void pc_q35_4_0_1_machine_options(MachineClass *m)
 {
     pc_q35_4_1_machine_options(m);
     m->alias = NULL;
+    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
+    compat_props_add(m->compat_props, pc_compat_4_0_1, pc_compat_4_0_1_len);
+}
+
+DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
+                   pc_q35_4_0_1_machine_options);
+
+static void pc_q35_4_0_machine_options(MachineClass *m)
+{
+    pc_q35_4_0_1_machine_options(m);
+    m->default_kernel_irqchip_split = true;
+    m->alias = NULL;
     compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
     compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
 }
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 6f7916f88f02..6ff02bf3e472 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -292,6 +292,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_4_0_1[];
+extern const size_t hw_compat_4_0_1_len;
+
 extern GlobalProperty hw_compat_4_0[];
 extern const size_t hw_compat_4_0_len;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 43df7230a22b..5d5636241e34 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -293,6 +293,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
 int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
+extern GlobalProperty pc_compat_4_0_1[];
+extern const size_t pc_compat_4_0_1_len;
+
 extern GlobalProperty pc_compat_4_0[];
 extern const size_t pc_compat_4_0_len;
 

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Peter Xu]

On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
> Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> the default for the pc-q35-4.0 machine type to use split irqchip, which
> turned out to have disasterous effects on vfio-pci INTx support.  KVM
> resampling irqfds are registered for handling these interrupts, but
> these are non-functional in split irqchip mode.  We can't simply test
> for split irqchip in QEMU as userspace handling of this interrupt is a
> significant performance regression versus KVM handling (GeForce GPUs
> assigned to Windows VMs are non-functional without forcing MSI mode or
> re-enabling kernel irqchip).
> 
> The resolution is to revert the change in default irqchip mode in the
> pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> configurations for devices requiring legacy INTx support without
> explicitly modifying the VM configuration to use kernel irqchip.
> 
> Link: https://bugs.launchpad.net/qemu/+bug/1826422
> Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Hi, Alex,

I have two (probably naive) questions about the patch, possibly due to
lack of context of previous discussions so please let me know if
there's any upstream discussion that I can read.

Firstly, could I ask why we need this 4.0.1 machine type specific for
fixing this problem?  Asked because this seems to be the first time
QEMU introduces the X.Y.Z machine type in master.  Could it be somehow
delayed to the release of QEMU 4.1?  From the planning page I see that
it's releasing on Aug 06th/13th, a bit far away but not really that
much imho.  I'm perfectly fine with this, but I just want to make sure
I have the correct understanding of the motivations.

The second question is about our previous decision to introduce QEMU
4.1 machine type before it's released (which is not related to the
patch at all).  Is it really correct to do so before releasing of 4.1?
So now even with a development QEMU 4.0 branch the user will be able
to create 4.1 machines using "-M pc-q35-4.1", then what if the user
migrated a real 4.1 machine (with the to-be-released QEMU 4.1 binary)
to some 4.1 machine that was run with such an old 4.0 QEMU binary?
The problem is we can add more compatible properties into
pc_q35_4_1_machine_options and future pc_compat_4_1 array before QEMU
4.1 is finally released and then "-M pc-q35-4.1" will actually have
different combination of properties IMHO, which seems to break
compatibility.  Am I wrong somewhere?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Daniel P. Berrangé]

On Wed, May 15, 2019 at 02:15:03PM +0800, Peter Xu wrote:
> On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
> > Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> > the default for the pc-q35-4.0 machine type to use split irqchip, which
> > turned out to have disasterous effects on vfio-pci INTx support.  KVM
> > resampling irqfds are registered for handling these interrupts, but
> > these are non-functional in split irqchip mode.  We can't simply test
> > for split irqchip in QEMU as userspace handling of this interrupt is a
> > significant performance regression versus KVM handling (GeForce GPUs
> > assigned to Windows VMs are non-functional without forcing MSI mode or
> > re-enabling kernel irqchip).
> > 
> > The resolution is to revert the change in default irqchip mode in the
> > pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> > branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> > configurations for devices requiring legacy INTx support without
> > explicitly modifying the VM configuration to use kernel irqchip.
> > 
> > Link: https://bugs.launchpad.net/qemu/+bug/1826422
> > Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Hi, Alex,
> 
> I have two (probably naive) questions about the patch, possibly due to
> lack of context of previous discussions so please let me know if
> there's any upstream discussion that I can read.
> 
> Firstly, could I ask why we need this 4.0.1 machine type specific for
> fixing this problem?  Asked because this seems to be the first time
> QEMU introduces the X.Y.Z machine type in master.  Could it be somehow
> delayed to the release of QEMU 4.1?  From the planning page I see that
> it's releasing on Aug 06th/13th, a bit far away but not really that
> much imho.  I'm perfectly fine with this, but I just want to make sure
> I have the correct understanding of the motivations.


> The second question is about our previous decision to introduce QEMU
> 4.1 machine type before it's released (which is not related to the
> patch at all).  Is it really correct to do so before releasing of 4.1?
> So now even with a development QEMU 4.0 branch the user will be able
> to create 4.1 machines using "-M pc-q35-4.1", then what if the user
> migrated a real 4.1 machine (with the to-be-released QEMU 4.1 binary)
> to some 4.1 machine that was run with such an old 4.0 QEMU binary?
> The problem is we can add more compatible properties into
> pc_q35_4_1_machine_options and future pc_compat_4_1 array before QEMU
> 4.1 is finally released and then "-M pc-q35-4.1" will actually have
> different combination of properties IMHO, which seems to break
> compatibility.  Am I wrong somewhere?

You are correct - we can't release a pc-q35-4.1 machine type to stable
because this machine type may change arbitrarily again before release.

Alex's suggestion of a pc-q35-4.0.1 is best thought of as releasing
a point in time snapshot of the pc-q35-4.1 machine type to stable.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchipQEM

[Alex Williamson]

On Wed, 15 May 2019 14:15:03 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
> > Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> > the default for the pc-q35-4.0 machine type to use split irqchip, which
> > turned out to have disasterous effects on vfio-pci INTx support.  KVM
> > resampling irqfds are registered for handling these interrupts, but
> > these are non-functional in split irqchip mode.  We can't simply test
> > for split irqchip in QEMU as userspace handling of this interrupt is a
> > significant performance regression versus KVM handling (GeForce GPUs
> > assigned to Windows VMs are non-functional without forcing MSI mode or
> > re-enabling kernel irqchip).
> > 
> > The resolution is to revert the change in default irqchip mode in the
> > pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> > branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> > configurations for devices requiring legacy INTx support without
> > explicitly modifying the VM configuration to use kernel irqchip.
> > 
> > Link: https://bugs.launchpad.net/qemu/+bug/1826422
> > Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> 
> Hi, Alex,
> 
> I have two (probably naive) questions about the patch, possibly due to
> lack of context of previous discussions so please let me know if
> there's any upstream discussion that I can read.
> 
> Firstly, could I ask why we need this 4.0.1 machine type specific for
> fixing this problem?  Asked because this seems to be the first time
> QEMU introduces the X.Y.Z machine type in master.  Could it be somehow
> delayed to the release of QEMU 4.1?  From the planning page I see that
> it's releasing on Aug 06th/13th, a bit far away but not really that
> much imho.  I'm perfectly fine with this, but I just want to make sure
> I have the correct understanding of the motivations.

As I see it, this is a regression from previous releases, therefore it
should be fixed in 4.0-stable.  Users are encountering this issue and
leaning on support groups like reddit.com/r/VFIO to find workarounds.
It would be a disservice to our user base and downstream consumers to
simply ignore this regression until the 4.1 release.  If this is the
first z-stream release of upstream QEMU with a new machine type, we've
been lucky, but previous discussions indicate that we cannot currently
change the irqchip mode without rev'ing the machine type for migration
compatibility.
 
> The second question is about our previous decision to introduce QEMU
> 4.1 machine type before it's released (which is not related to the
> patch at all).  Is it really correct to do so before releasing of 4.1?
> So now even with a development QEMU 4.0 branch the user will be able
> to create 4.1 machines using "-M pc-q35-4.1", then what if the user
> migrated a real 4.1 machine (with the to-be-released QEMU 4.1 binary)
> to some 4.1 machine that was run with such an old 4.0 QEMU binary?
> The problem is we can add more compatible properties into
> pc_q35_4_1_machine_options and future pc_compat_4_1 array before QEMU
> 4.1 is finally released and then "-M pc-q35-4.1" will actually have
> different combination of properties IMHO, which seems to break
> compatibility.  Am I wrong somewhere?

Users who expect migration stability from VMs based on unreleased
development code are in for a world of hurt.  I assume that the 4.1
machine types are entirely unstable until 4.1 is released.  We
introduce them early in the development cycle because we've been burned
in the past introducing them late and inconsistently.  Ideally this
change would trigger a migration regression test to generate a warning
for the in-development machine type changing in an incompatible way,
we'd acknowledge that, perhaps log it to a changelog, and move on, but
I suspect we don't have such automated testing in place.  Thanks,

Alex

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchipQEM

[Peter Xu]

On Wed, May 15, 2019 at 07:23:13AM -0600, Alex Williamson wrote:
> On Wed, 15 May 2019 14:15:03 +0800
> Peter Xu <peterx@redhat.com> wrote:
> 
> > On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
> > > Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> > > the default for the pc-q35-4.0 machine type to use split irqchip, which
> > > turned out to have disasterous effects on vfio-pci INTx support.  KVM
> > > resampling irqfds are registered for handling these interrupts, but
> > > these are non-functional in split irqchip mode.  We can't simply test
> > > for split irqchip in QEMU as userspace handling of this interrupt is a
> > > significant performance regression versus KVM handling (GeForce GPUs
> > > assigned to Windows VMs are non-functional without forcing MSI mode or
> > > re-enabling kernel irqchip).
> > > 
> > > The resolution is to revert the change in default irqchip mode in the
> > > pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> > > branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> > > configurations for devices requiring legacy INTx support without
> > > explicitly modifying the VM configuration to use kernel irqchip.
> > > 
> > > Link: https://bugs.launchpad.net/qemu/+bug/1826422
> > > Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> > 
> > Hi, Alex,
> > 
> > I have two (probably naive) questions about the patch, possibly due to
> > lack of context of previous discussions so please let me know if
> > there's any upstream discussion that I can read.
> > 
> > Firstly, could I ask why we need this 4.0.1 machine type specific for
> > fixing this problem?  Asked because this seems to be the first time
> > QEMU introduces the X.Y.Z machine type in master.  Could it be somehow
> > delayed to the release of QEMU 4.1?  From the planning page I see that
> > it's releasing on Aug 06th/13th, a bit far away but not really that
> > much imho.  I'm perfectly fine with this, but I just want to make sure
> > I have the correct understanding of the motivations.
> 
> As I see it, this is a regression from previous releases, therefore it
> should be fixed in 4.0-stable.  Users are encountering this issue and
> leaning on support groups like reddit.com/r/VFIO to find workarounds.
> It would be a disservice to our user base and downstream consumers to
> simply ignore this regression until the 4.1 release.  If this is the
> first z-stream release of upstream QEMU with a new machine type, we've
> been lucky, but previous discussions indicate that we cannot currently
> change the irqchip mode without rev'ing the machine type for migration
> compatibility.
>  
> > The second question is about our previous decision to introduce QEMU
> > 4.1 machine type before it's released (which is not related to the
> > patch at all).  Is it really correct to do so before releasing of 4.1?
> > So now even with a development QEMU 4.0 branch the user will be able
> > to create 4.1 machines using "-M pc-q35-4.1", then what if the user
> > migrated a real 4.1 machine (with the to-be-released QEMU 4.1 binary)
> > to some 4.1 machine that was run with such an old 4.0 QEMU binary?
> > The problem is we can add more compatible properties into
> > pc_q35_4_1_machine_options and future pc_compat_4_1 array before QEMU
> > 4.1 is finally released and then "-M pc-q35-4.1" will actually have
> > different combination of properties IMHO, which seems to break
> > compatibility.  Am I wrong somewhere?
> 
> Users who expect migration stability from VMs based on unreleased
> development code are in for a world of hurt.  I assume that the 4.1
> machine types are entirely unstable until 4.1 is released.  We
> introduce them early in the development cycle because we've been burned
> in the past introducing them late and inconsistently.  Ideally this
> change would trigger a migration regression test to generate a warning
> for the in-development machine type changing in an incompatible way,
> we'd acknowledge that, perhaps log it to a changelog, and move on, but
> I suspect we don't have such automated testing in place.  Thanks,

I see the points, thanks for explaining (to Dan as well).

About "introducing them late and inconsistently" for the machine types
- I completely agree it was mostly always too late, e.g., in most
cases the new machine type will only be introduced by the one who will
need the first compatilble property (no matter which arch he/she is
working on and which module...).  It seems more ideal to me to just
introduce the major new machine types along with each of the QEMU
release (I mean the final release no matter which RC, it's just the
point when we push the release tag), but of course that'll be a burden
to the project maintainer or machine type maintainers... and anyway
ignoring compatible with development branches looks reasonable too.

In all cases, this patch looks good to me:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Michael S. Tsirkin]

On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
> Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> the default for the pc-q35-4.0 machine type to use split irqchip, which
> turned out to have disasterous effects on vfio-pci INTx support.  KVM
> resampling irqfds are registered for handling these interrupts, but
> these are non-functional in split irqchip mode.  We can't simply test
> for split irqchip in QEMU as userspace handling of this interrupt is a
> significant performance regression versus KVM handling (GeForce GPUs
> assigned to Windows VMs are non-functional without forcing MSI mode or
> re-enabling kernel irqchip).
> 
> The resolution is to revert the change in default irqchip mode in the
> pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> configurations for devices requiring legacy INTx support without
> explicitly modifying the VM configuration to use kernel irqchip.
> 
> Link: https://bugs.launchpad.net/qemu/+bug/1826422
> Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

OK I guess but it's really a kvm patch.
So I'd like Paolo to review and merge if appropriate.

Can't say this makes me too happy. split irqchip
has a bunch of advantages.

> ---
>  hw/core/machine.c    |    3 +++
>  hw/i386/pc.c         |    3 +++
>  hw/i386/pc_q35.c     |   16 ++++++++++++++--
>  include/hw/boards.h  |    3 +++
>  include/hw/i386/pc.h |    3 +++
>  5 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 5d046a43e3d2..e41e6698ac9f 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -24,6 +24,9 @@
>  #include "hw/pci/pci.h"
>  #include "hw/mem/nvdimm.h"
>  
> +GlobalProperty hw_compat_4_0_1[] = {};
> +const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
> +
>  GlobalProperty hw_compat_4_0[] = {};
>  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d98b737b8f3b..b5311e7e2bd5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -115,6 +115,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>  /* Physical Address of PVH entry point read from kernel ELF NOTE */
>  static size_t pvh_start_addr;
>  
> +GlobalProperty pc_compat_4_0_1[] = {};
> +const size_t pc_compat_4_0_1_len = G_N_ELEMENTS(pc_compat_4_0_1);
> +
>  GlobalProperty pc_compat_4_0[] = {};
>  const size_t pc_compat_4_0_len = G_N_ELEMENTS(pc_compat_4_0);
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 37dd350511a9..dcddc6466200 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -357,7 +357,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->default_kernel_irqchip_split = false;
>      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);
> @@ -374,10 +374,22 @@ static void pc_q35_4_1_machine_options(MachineClass *m)
>  DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
>                     pc_q35_4_1_machine_options);
>  
> -static void pc_q35_4_0_machine_options(MachineClass *m)
> +static void pc_q35_4_0_1_machine_options(MachineClass *m)
>  {
>      pc_q35_4_1_machine_options(m);
>      m->alias = NULL;
> +    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
> +    compat_props_add(m->compat_props, pc_compat_4_0_1, pc_compat_4_0_1_len);
> +}
> +
> +DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
> +                   pc_q35_4_0_1_machine_options);
> +
> +static void pc_q35_4_0_machine_options(MachineClass *m)
> +{
> +    pc_q35_4_0_1_machine_options(m);
> +    m->default_kernel_irqchip_split = true;
> +    m->alias = NULL;
>      compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
>      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
>  }
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 6f7916f88f02..6ff02bf3e472 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -292,6 +292,9 @@ struct MachineState {
>      } \
>      type_init(machine_initfn##_register_types)
>  
> +extern GlobalProperty hw_compat_4_0_1[];
> +extern const size_t hw_compat_4_0_1_len;
> +
>  extern GlobalProperty hw_compat_4_0[];
>  extern const size_t hw_compat_4_0_len;
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 43df7230a22b..5d5636241e34 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -293,6 +293,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>  int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
> +extern GlobalProperty pc_compat_4_0_1[];
> +extern const size_t pc_compat_4_0_1_len;
> +
>  extern GlobalProperty pc_compat_4_0[];
>  extern const size_t pc_compat_4_0_len;
>  

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Alex Williamson]

On Tue, 28 May 2019 23:30:20 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
> > Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> > the default for the pc-q35-4.0 machine type to use split irqchip, which
> > turned out to have disasterous effects on vfio-pci INTx support.  KVM
> > resampling irqfds are registered for handling these interrupts, but
> > these are non-functional in split irqchip mode.  We can't simply test
> > for split irqchip in QEMU as userspace handling of this interrupt is a
> > significant performance regression versus KVM handling (GeForce GPUs
> > assigned to Windows VMs are non-functional without forcing MSI mode or
> > re-enabling kernel irqchip).
> > 
> > The resolution is to revert the change in default irqchip mode in the
> > pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> > branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> > configurations for devices requiring legacy INTx support without
> > explicitly modifying the VM configuration to use kernel irqchip.
> > 
> > Link: https://bugs.launchpad.net/qemu/+bug/1826422
> > Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> 
> OK I guess but it's really a kvm patch.
> So I'd like Paolo to review and merge if appropriate.
> 
> Can't say this makes me too happy. split irqchip
> has a bunch of advantages.

What exactly are those advantages relative to a our default QEMU
users?  I see lots of users assigning consumer class hardware and
seeing regressions.  They don't care about a theoretically improved
security attack surface at the cost of performance for legacy devices,
or especially functionality.  Should these users be our focus for the
default machine type?  I think we could make these sorts of compromises
in a legacy-free or virt/cloud/enterprise machine type, but for a
general purpose PC emulator, it doesn't seem a good match given the
issues we have currently.  Obviously users can continue to tune the q35
machine type on their own and enable split irqchip if it's important to
them.  Thanks,

Alex

> > ---
> >  hw/core/machine.c    |    3 +++
> >  hw/i386/pc.c         |    3 +++
> >  hw/i386/pc_q35.c     |   16 ++++++++++++++--
> >  include/hw/boards.h  |    3 +++
> >  include/hw/i386/pc.h |    3 +++
> >  5 files changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 5d046a43e3d2..e41e6698ac9f 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -24,6 +24,9 @@
> >  #include "hw/pci/pci.h"
> >  #include "hw/mem/nvdimm.h"
> >  
> > +GlobalProperty hw_compat_4_0_1[] = {};
> > +const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
> > +
> >  GlobalProperty hw_compat_4_0[] = {};
> >  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
> >  
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index d98b737b8f3b..b5311e7e2bd5 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -115,6 +115,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> >  /* Physical Address of PVH entry point read from kernel ELF NOTE */
> >  static size_t pvh_start_addr;
> >  
> > +GlobalProperty pc_compat_4_0_1[] = {};
> > +const size_t pc_compat_4_0_1_len = G_N_ELEMENTS(pc_compat_4_0_1);
> > +
> >  GlobalProperty pc_compat_4_0[] = {};
> >  const size_t pc_compat_4_0_len = G_N_ELEMENTS(pc_compat_4_0);
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 37dd350511a9..dcddc6466200 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -357,7 +357,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->default_kernel_irqchip_split = false;
> >      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);
> > @@ -374,10 +374,22 @@ static void pc_q35_4_1_machine_options(MachineClass *m)
> >  DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
> >                     pc_q35_4_1_machine_options);
> >  
> > -static void pc_q35_4_0_machine_options(MachineClass *m)
> > +static void pc_q35_4_0_1_machine_options(MachineClass *m)
> >  {
> >      pc_q35_4_1_machine_options(m);
> >      m->alias = NULL;
> > +    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
> > +    compat_props_add(m->compat_props, pc_compat_4_0_1, pc_compat_4_0_1_len);
> > +}
> > +
> > +DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
> > +                   pc_q35_4_0_1_machine_options);
> > +
> > +static void pc_q35_4_0_machine_options(MachineClass *m)
> > +{
> > +    pc_q35_4_0_1_machine_options(m);
> > +    m->default_kernel_irqchip_split = true;
> > +    m->alias = NULL;
> >      compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
> >      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
> >  }
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 6f7916f88f02..6ff02bf3e472 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -292,6 +292,9 @@ struct MachineState {
> >      } \
> >      type_init(machine_initfn##_register_types)
> >  
> > +extern GlobalProperty hw_compat_4_0_1[];
> > +extern const size_t hw_compat_4_0_1_len;
> > +
> >  extern GlobalProperty hw_compat_4_0[];
> >  extern const size_t hw_compat_4_0_len;
> >  
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 43df7230a22b..5d5636241e34 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -293,6 +293,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >  int e820_get_num_entries(void);
> >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >  
> > +extern GlobalProperty pc_compat_4_0_1[];
> > +extern const size_t pc_compat_4_0_1_len;
> > +
> >  extern GlobalProperty pc_compat_4_0[];
> >  extern const size_t pc_compat_4_0_len;
> >    

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Michael S. Tsirkin]

On Wed, May 29, 2019 at 07:43:56AM -0600, Alex Williamson wrote:
> On Tue, 28 May 2019 23:30:20 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
> > > Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> > > the default for the pc-q35-4.0 machine type to use split irqchip, which
> > > turned out to have disasterous effects on vfio-pci INTx support.  KVM
> > > resampling irqfds are registered for handling these interrupts, but
> > > these are non-functional in split irqchip mode.  We can't simply test
> > > for split irqchip in QEMU as userspace handling of this interrupt is a
> > > significant performance regression versus KVM handling (GeForce GPUs
> > > assigned to Windows VMs are non-functional without forcing MSI mode or
> > > re-enabling kernel irqchip).
> > > 
> > > The resolution is to revert the change in default irqchip mode in the
> > > pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> > > branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> > > configurations for devices requiring legacy INTx support without
> > > explicitly modifying the VM configuration to use kernel irqchip.
> > > 
> > > Link: https://bugs.launchpad.net/qemu/+bug/1826422
> > > Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> > 
> > OK I guess but it's really a kvm patch.
> > So I'd like Paolo to review and merge if appropriate.
> > 
> > Can't say this makes me too happy. split irqchip
> > has a bunch of advantages.
> 
> What exactly are those advantages relative to a our default QEMU
> users?  I see lots of users assigning consumer class hardware and
> seeing regressions.  They don't care about a theoretically improved
> security attack surface at the cost of performance for legacy devices,
> or especially functionality.  Should these users be our focus for the
> default machine type?  I think we could make these sorts of compromises
> in a legacy-free or virt/cloud/enterprise machine type, but for a
> general purpose PC emulator, it doesn't seem a good match given the
> issues we have currently.  Obviously users can continue to tune the q35
> machine type on their own and enable split irqchip if it's important to
> them.  Thanks,
> 
> Alex

It's a reasonable argument. Again I think Paolo should make the call.

> > > ---
> > >  hw/core/machine.c    |    3 +++
> > >  hw/i386/pc.c         |    3 +++
> > >  hw/i386/pc_q35.c     |   16 ++++++++++++++--
> > >  include/hw/boards.h  |    3 +++
> > >  include/hw/i386/pc.h |    3 +++
> > >  5 files changed, 26 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 5d046a43e3d2..e41e6698ac9f 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -24,6 +24,9 @@
> > >  #include "hw/pci/pci.h"
> > >  #include "hw/mem/nvdimm.h"
> > >  
> > > +GlobalProperty hw_compat_4_0_1[] = {};
> > > +const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
> > > +
> > >  GlobalProperty hw_compat_4_0[] = {};
> > >  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
> > >  
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index d98b737b8f3b..b5311e7e2bd5 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -115,6 +115,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> > >  /* Physical Address of PVH entry point read from kernel ELF NOTE */
> > >  static size_t pvh_start_addr;
> > >  
> > > +GlobalProperty pc_compat_4_0_1[] = {};
> > > +const size_t pc_compat_4_0_1_len = G_N_ELEMENTS(pc_compat_4_0_1);
> > > +
> > >  GlobalProperty pc_compat_4_0[] = {};
> > >  const size_t pc_compat_4_0_len = G_N_ELEMENTS(pc_compat_4_0);
> > >  
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index 37dd350511a9..dcddc6466200 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -357,7 +357,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->default_kernel_irqchip_split = false;
> > >      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);
> > > @@ -374,10 +374,22 @@ static void pc_q35_4_1_machine_options(MachineClass *m)
> > >  DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
> > >                     pc_q35_4_1_machine_options);
> > >  
> > > -static void pc_q35_4_0_machine_options(MachineClass *m)
> > > +static void pc_q35_4_0_1_machine_options(MachineClass *m)
> > >  {
> > >      pc_q35_4_1_machine_options(m);
> > >      m->alias = NULL;
> > > +    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
> > > +    compat_props_add(m->compat_props, pc_compat_4_0_1, pc_compat_4_0_1_len);
> > > +}
> > > +
> > > +DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
> > > +                   pc_q35_4_0_1_machine_options);
> > > +
> > > +static void pc_q35_4_0_machine_options(MachineClass *m)
> > > +{
> > > +    pc_q35_4_0_1_machine_options(m);
> > > +    m->default_kernel_irqchip_split = true;
> > > +    m->alias = NULL;
> > >      compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
> > >      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
> > >  }
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index 6f7916f88f02..6ff02bf3e472 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -292,6 +292,9 @@ struct MachineState {
> > >      } \
> > >      type_init(machine_initfn##_register_types)
> > >  
> > > +extern GlobalProperty hw_compat_4_0_1[];
> > > +extern const size_t hw_compat_4_0_1_len;
> > > +
> > >  extern GlobalProperty hw_compat_4_0[];
> > >  extern const size_t hw_compat_4_0_len;
> > >  
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 43df7230a22b..5d5636241e34 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -293,6 +293,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> > >  int e820_get_num_entries(void);
> > >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> > >  
> > > +extern GlobalProperty pc_compat_4_0_1[];
> > > +extern const size_t pc_compat_4_0_1_len;
> > > +
> > >  extern GlobalProperty pc_compat_4_0[];
> > >  extern const size_t pc_compat_4_0_len;
> > >    

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Alex Williamson]

Paolo?  Please see MST's request below.  Thanks,

Alex

On Wed, 29 May 2019 12:01:14 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, May 29, 2019 at 07:43:56AM -0600, Alex Williamson wrote:
> > On Tue, 28 May 2019 23:30:20 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:  
> > > > Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> > > > the default for the pc-q35-4.0 machine type to use split irqchip, which
> > > > turned out to have disasterous effects on vfio-pci INTx support.  KVM
> > > > resampling irqfds are registered for handling these interrupts, but
> > > > these are non-functional in split irqchip mode.  We can't simply test
> > > > for split irqchip in QEMU as userspace handling of this interrupt is a
> > > > significant performance regression versus KVM handling (GeForce GPUs
> > > > assigned to Windows VMs are non-functional without forcing MSI mode or
> > > > re-enabling kernel irqchip).
> > > > 
> > > > The resolution is to revert the change in default irqchip mode in the
> > > > pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> > > > branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> > > > configurations for devices requiring legacy INTx support without
> > > > explicitly modifying the VM configuration to use kernel irqchip.
> > > > 
> > > > Link: https://bugs.launchpad.net/qemu/+bug/1826422
> > > > Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>    
> > > 
> > > OK I guess but it's really a kvm patch.
> > > So I'd like Paolo to review and merge if appropriate.
> > > 
> > > Can't say this makes me too happy. split irqchip
> > > has a bunch of advantages.  
> > 
> > What exactly are those advantages relative to a our default QEMU
> > users?  I see lots of users assigning consumer class hardware and
> > seeing regressions.  They don't care about a theoretically improved
> > security attack surface at the cost of performance for legacy devices,
> > or especially functionality.  Should these users be our focus for the
> > default machine type?  I think we could make these sorts of compromises
> > in a legacy-free or virt/cloud/enterprise machine type, but for a
> > general purpose PC emulator, it doesn't seem a good match given the
> > issues we have currently.  Obviously users can continue to tune the q35
> > machine type on their own and enable split irqchip if it's important to
> > them.  Thanks,
> > 
> > Alex  
> 
> It's a reasonable argument. Again I think Paolo should make the call.
> 
> > > > ---
> > > >  hw/core/machine.c    |    3 +++
> > > >  hw/i386/pc.c         |    3 +++
> > > >  hw/i386/pc_q35.c     |   16 ++++++++++++++--
> > > >  include/hw/boards.h  |    3 +++
> > > >  include/hw/i386/pc.h |    3 +++
> > > >  5 files changed, 26 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index 5d046a43e3d2..e41e6698ac9f 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -24,6 +24,9 @@
> > > >  #include "hw/pci/pci.h"
> > > >  #include "hw/mem/nvdimm.h"
> > > >  
> > > > +GlobalProperty hw_compat_4_0_1[] = {};
> > > > +const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
> > > > +
> > > >  GlobalProperty hw_compat_4_0[] = {};
> > > >  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
> > > >  
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index d98b737b8f3b..b5311e7e2bd5 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -115,6 +115,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> > > >  /* Physical Address of PVH entry point read from kernel ELF NOTE */
> > > >  static size_t pvh_start_addr;
> > > >  
> > > > +GlobalProperty pc_compat_4_0_1[] = {};
> > > > +const size_t pc_compat_4_0_1_len = G_N_ELEMENTS(pc_compat_4_0_1);
> > > > +
> > > >  GlobalProperty pc_compat_4_0[] = {};
> > > >  const size_t pc_compat_4_0_len = G_N_ELEMENTS(pc_compat_4_0);
> > > >  
> > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > index 37dd350511a9..dcddc6466200 100644
> > > > --- a/hw/i386/pc_q35.c
> > > > +++ b/hw/i386/pc_q35.c
> > > > @@ -357,7 +357,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->default_kernel_irqchip_split = false;
> > > >      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);
> > > > @@ -374,10 +374,22 @@ static void pc_q35_4_1_machine_options(MachineClass *m)
> > > >  DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
> > > >                     pc_q35_4_1_machine_options);
> > > >  
> > > > -static void pc_q35_4_0_machine_options(MachineClass *m)
> > > > +static void pc_q35_4_0_1_machine_options(MachineClass *m)
> > > >  {
> > > >      pc_q35_4_1_machine_options(m);
> > > >      m->alias = NULL;
> > > > +    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
> > > > +    compat_props_add(m->compat_props, pc_compat_4_0_1, pc_compat_4_0_1_len);
> > > > +}
> > > > +
> > > > +DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
> > > > +                   pc_q35_4_0_1_machine_options);
> > > > +
> > > > +static void pc_q35_4_0_machine_options(MachineClass *m)
> > > > +{
> > > > +    pc_q35_4_0_1_machine_options(m);
> > > > +    m->default_kernel_irqchip_split = true;
> > > > +    m->alias = NULL;
> > > >      compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
> > > >      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
> > > >  }
> > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > index 6f7916f88f02..6ff02bf3e472 100644
> > > > --- a/include/hw/boards.h
> > > > +++ b/include/hw/boards.h
> > > > @@ -292,6 +292,9 @@ struct MachineState {
> > > >      } \
> > > >      type_init(machine_initfn##_register_types)
> > > >  
> > > > +extern GlobalProperty hw_compat_4_0_1[];
> > > > +extern const size_t hw_compat_4_0_1_len;
> > > > +
> > > >  extern GlobalProperty hw_compat_4_0[];
> > > >  extern const size_t hw_compat_4_0_len;
> > > >  
> > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > index 43df7230a22b..5d5636241e34 100644
> > > > --- a/include/hw/i386/pc.h
> > > > +++ b/include/hw/i386/pc.h
> > > > @@ -293,6 +293,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> > > >  int e820_get_num_entries(void);
> > > >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> > > >  
> > > > +extern GlobalProperty pc_compat_4_0_1[];
> > > > +extern const size_t pc_compat_4_0_1_len;
> > > > +
> > > >  extern GlobalProperty pc_compat_4_0[];
> > > >  extern const size_t pc_compat_4_0_len;
> > > >      

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Paolo Bonzini]

On 29/05/19 05:30, Michael S. Tsirkin wrote:
> On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
>> Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
>> the default for the pc-q35-4.0 machine type to use split irqchip, which
>> turned out to have disasterous effects on vfio-pci INTx support.  KVM
>> resampling irqfds are registered for handling these interrupts, but
>> these are non-functional in split irqchip mode.  We can't simply test
>> for split irqchip in QEMU as userspace handling of this interrupt is a
>> significant performance regression versus KVM handling (GeForce GPUs
>> assigned to Windows VMs are non-functional without forcing MSI mode or
>> re-enabling kernel irqchip).
>>
>> The resolution is to revert the change in default irqchip mode in the
>> pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
>> branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
>> configurations for devices requiring legacy INTx support without
>> explicitly modifying the VM configuration to use kernel irqchip.
>>
>> Link: https://bugs.launchpad.net/qemu/+bug/1826422
>> Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> OK I guess but it's really a kvm patch.
> So I'd like Paolo to review and merge if appropriate.
> 
> Can't say this makes me too happy. split irqchip
> has a bunch of advantages.

Yeah, me too but I don't see an alternative.  I'll merge it today.

Paolo

> 
>> ---
>>  hw/core/machine.c    |    3 +++
>>  hw/i386/pc.c         |    3 +++
>>  hw/i386/pc_q35.c     |   16 ++++++++++++++--
>>  include/hw/boards.h  |    3 +++
>>  include/hw/i386/pc.h |    3 +++
>>  5 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 5d046a43e3d2..e41e6698ac9f 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -24,6 +24,9 @@
>>  #include "hw/pci/pci.h"
>>  #include "hw/mem/nvdimm.h"
>>  
>> +GlobalProperty hw_compat_4_0_1[] = {};
>> +const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
>> +
>>  GlobalProperty hw_compat_4_0[] = {};
>>  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
>>  
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index d98b737b8f3b..b5311e7e2bd5 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -115,6 +115,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>>  /* Physical Address of PVH entry point read from kernel ELF NOTE */
>>  static size_t pvh_start_addr;
>>  
>> +GlobalProperty pc_compat_4_0_1[] = {};
>> +const size_t pc_compat_4_0_1_len = G_N_ELEMENTS(pc_compat_4_0_1);
>> +
>>  GlobalProperty pc_compat_4_0[] = {};
>>  const size_t pc_compat_4_0_len = G_N_ELEMENTS(pc_compat_4_0);
>>  
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 37dd350511a9..dcddc6466200 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -357,7 +357,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->default_kernel_irqchip_split = false;
>>      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);
>> @@ -374,10 +374,22 @@ static void pc_q35_4_1_machine_options(MachineClass *m)
>>  DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
>>                     pc_q35_4_1_machine_options);
>>  
>> -static void pc_q35_4_0_machine_options(MachineClass *m)
>> +static void pc_q35_4_0_1_machine_options(MachineClass *m)
>>  {
>>      pc_q35_4_1_machine_options(m);
>>      m->alias = NULL;
>> +    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
>> +    compat_props_add(m->compat_props, pc_compat_4_0_1, pc_compat_4_0_1_len);
>> +}
>> +
>> +DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
>> +                   pc_q35_4_0_1_machine_options);
>> +
>> +static void pc_q35_4_0_machine_options(MachineClass *m)
>> +{
>> +    pc_q35_4_0_1_machine_options(m);
>> +    m->default_kernel_irqchip_split = true;
>> +    m->alias = NULL;
>>      compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
>>      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
>>  }
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 6f7916f88f02..6ff02bf3e472 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -292,6 +292,9 @@ struct MachineState {
>>      } \
>>      type_init(machine_initfn##_register_types)
>>  
>> +extern GlobalProperty hw_compat_4_0_1[];
>> +extern const size_t hw_compat_4_0_1_len;
>> +
>>  extern GlobalProperty hw_compat_4_0[];
>>  extern const size_t hw_compat_4_0_len;
>>  
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 43df7230a22b..5d5636241e34 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -293,6 +293,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>>  int e820_get_num_entries(void);
>>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>>  
>> +extern GlobalProperty pc_compat_4_0_1[];
>> +extern const size_t pc_compat_4_0_1_len;
>> +
>>  extern GlobalProperty pc_compat_4_0[];
>>  extern const size_t pc_compat_4_0_len;
>>  

Re: [Qemu-devel] [PATCH for-4.1 v2] q35: Revert to kernel irqchip

[Daniel P. Berrangé]

On Mon, Jun 03, 2019 at 10:06:06AM +0200, Paolo Bonzini wrote:
> On 29/05/19 05:30, Michael S. Tsirkin wrote:
> > On Tue, May 14, 2019 at 02:14:41PM -0600, Alex Williamson wrote:
> >> Commit b2fc91db8447 ("q35: set split kernel irqchip as default") changed
> >> the default for the pc-q35-4.0 machine type to use split irqchip, which
> >> turned out to have disasterous effects on vfio-pci INTx support.  KVM
> >> resampling irqfds are registered for handling these interrupts, but
> >> these are non-functional in split irqchip mode.  We can't simply test
> >> for split irqchip in QEMU as userspace handling of this interrupt is a
> >> significant performance regression versus KVM handling (GeForce GPUs
> >> assigned to Windows VMs are non-functional without forcing MSI mode or
> >> re-enabling kernel irqchip).
> >>
> >> The resolution is to revert the change in default irqchip mode in the
> >> pc-q35-4.1 machine and create a pc-q35-4.0.1 machine for the 4.0-stable
> >> branch.  The qemu-q35-4.0 machine type should not be used in vfio-pci
> >> configurations for devices requiring legacy INTx support without
> >> explicitly modifying the VM configuration to use kernel irqchip.
> >>
> >> Link: https://bugs.launchpad.net/qemu/+bug/1826422
> >> Fixes: b2fc91db8447 ("q35: set split kernel irqchip as default")
> >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > OK I guess but it's really a kvm patch.
> > So I'd like Paolo to review and merge if appropriate.
> > 
> > Can't say this makes me too happy. split irqchip
> > has a bunch of advantages.
> 
> Yeah, me too but I don't see an alternative.  I'll merge it today.

FYI in Fedora we've had another unrelated regression bug that was identified
as caused by the split irqchip change. With a Windows 7 guest, the clock
is way too fast, for every 1 second wallclock time, 15 seconds passes
in the guest. See the bug for more info:

  https://bugzilla.redhat.com/show_bug.cgi?id=1704375

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|