[PULL 00/11] QEMU changes for 2021-03-02

[PULL 00/11] QEMU changes for 2021-03-02

[Paolo Bonzini]

The following changes since commit 99c53410bc9d50e556f565b0960673cccb566452:

  Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2022-02-28' into staging (2022-03-01 13:25:54 +0000)

are available in the Git repository at:

  https://gitlab.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to 9e685c6c574a9e1f1e3affbb900f7c38fb4bff6e:

  target/i386: Throw a #SS when loading a non-canonical IST (2022-03-02 10:38:40 +0100)

----------------------------------------------------------------
* whpx fixes in preparation for GDB support (Ivan)
* VSS header fixes (Marc-André)
* Add 5-level EPT support to vmxcap (Vitaly)
* Bundle changes to MSI routes (Longpeng)
* More precise emulation of #SS (Gareth)

----------------------------------------------------------------
Gareth Webb (1):
      target/i386: Throw a #SS when loading a non-canonical IST

Ivan Shcherbakov (2):
      whpx: Fixed reporting of the CPU context to GDB for 64-bit
      whpx: Fixed incorrect CR8/TPR synchronization

Longpeng (Mike) (2):
      kvm-irqchip: introduce new API to support route change
      kvm/msi: do explicit commit when adding msi routes

Marc-André Lureau (3):
      meson: fix generic location of vss headers
      qga/vss-win32: check old VSS SDK headers
      qga/vss: update informative message about MinGW

Paolo Bonzini (2):
      update meson-buildoptions.sh
      target/i386: only include bits in pg_mode if they are not ignored

Vitaly Kuznetsov (1):
      vmxcap: Add 5-level EPT bit

 accel/kvm/kvm-all.c                  |  7 +++---
 accel/stubs/kvm-stub.c               |  2 +-
 hw/misc/ivshmem.c                    |  5 +++-
 hw/vfio/pci.c                        |  5 +++-
 hw/virtio/virtio-pci.c               |  4 ++-
 include/sysemu/kvm.h                 | 23 +++++++++++++++--
 meson.build                          |  5 +++-
 qga/meson.build                      |  2 +-
 qga/vss-win32/install.cpp            |  4 +++
 qga/vss-win32/provider.cpp           |  4 +++
 qga/vss-win32/vss-common.h           |  3 ++-
 scripts/kvm/vmxcap                   |  1 +
 scripts/meson-buildoptions.sh        |  2 +-
 target/i386/kvm/kvm.c                |  4 ++-
 target/i386/tcg/seg_helper.c         | 49 +++++++++++++++++++++++++++++++++++-
 target/i386/tcg/sysemu/excp_helper.c | 40 ++---------------------------
 target/i386/whpx/whpx-all.c          | 30 +++++++++++++++++++++-
 17 files changed, 136 insertions(+), 54 deletions(-)
-- 
2.34.1

[PULL 01/11] whpx: Fixed reporting of the CPU context to GDB for 64-bit

[Paolo Bonzini]

From: Ivan Shcherbakov <ivan@sysprogs.com>

Make sure that pausing the VM while in 64-bit mode will set the
HF_CS64_MASK flag in env->hflags (see x86_update_hflags() in
target/i386/cpu.c).

Without it, the code in gdbstub.c would only use the 32-bit register values
when debugging 64-bit targets, making debugging effectively impossible.

Signed-off-by: Ivan Shcherbakov <ivan@sysprogs.com>
Message-Id: <00f701d82874$68b02000$3a106000$@sysprogs.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/whpx/whpx-all.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index ef896da0a2..edd4fafbdf 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -604,6 +604,8 @@ static void whpx_get_registers(CPUState *cpu)
         whpx_apic_get(x86_cpu->apic_state);
     }
 
+    x86_update_hflags(env);
+
     return;
 }
 
-- 
2.34.1

[PULL 02/11] whpx: Fixed incorrect CR8/TPR synchronization

[Paolo Bonzini]

From: Ivan Shcherbakov <ivan@sysprogs.com>

This fixes the following error triggered when stopping and resuming a 64-bit
Linux kernel via gdb:

qemu-system-x86_64.exe: WHPX: Failed to set virtual processor context, hr=c0350005

The previous logic for synchronizing the values did not take into account
that the lower 4 bits of the CR8 register, containing the priority level,
mapped to bits 7:4 of the APIC.TPR register (see section 10.8.6.1 of
Volume 3 of Intel 64 and IA-32 Architectures Software Developer's Manual).
The caused WHvSetVirtualProcessorRegisters() to fail with an error,
effectively preventing GDB from changing the guest context.

Signed-off-by: Ivan Shcherbakov <ivan@sysprogs.com>
Message-Id: <010b01d82874$bb4ef160$31ecd420$@sysprogs.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/whpx/whpx-all.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index edd4fafbdf..63203730bc 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -256,6 +256,21 @@ static int whpx_set_tsc(CPUState *cpu)
     return 0;
 }
 
+/*
+ * The CR8 register in the CPU is mapped to the TPR register of the APIC,
+ * however, they use a slightly different encoding. Specifically:
+ *
+ *     APIC.TPR[bits 7:4] = CR8[bits 3:0]
+ *
+ * This mechanism is described in section 10.8.6.1 of Volume 3 of Intel 64
+ * and IA-32 Architectures Software Developer's Manual.
+ */
+
+static uint64_t whpx_apic_tpr_to_cr8(uint64_t tpr)
+{
+    return tpr >> 4;
+}
+
 static void whpx_set_registers(CPUState *cpu, int level)
 {
     struct whpx_state *whpx = &whpx_global;
@@ -284,7 +299,7 @@ static void whpx_set_registers(CPUState *cpu, int level)
     v86 = (env->eflags & VM_MASK);
     r86 = !(env->cr[0] & CR0_PE_MASK);
 
-    vcpu->tpr = cpu_get_apic_tpr(x86_cpu->apic_state);
+    vcpu->tpr = whpx_apic_tpr_to_cr8(cpu_get_apic_tpr(x86_cpu->apic_state));
     vcpu->apic_base = cpu_get_apic_base(x86_cpu->apic_state);
 
     idx = 0;
@@ -475,6 +490,17 @@ static void whpx_get_registers(CPUState *cpu)
                      hr);
     }
 
+    if (whpx_apic_in_platform()) {
+        /*
+         * Fetch the TPR value from the emulated APIC. It may get overwritten
+         * below with the value from CR8 returned by
+         * WHvGetVirtualProcessorRegisters().
+         */
+        whpx_apic_get(x86_cpu->apic_state);
+        vcpu->tpr = whpx_apic_tpr_to_cr8(
+            cpu_get_apic_tpr(x86_cpu->apic_state));
+    }
+
     idx = 0;
 
     /* Indexes for first 16 registers match between HV and QEMU definitions */
-- 
2.34.1

[PULL 03/11] vmxcap: Add 5-level EPT bit

[Paolo Bonzini]

From: Vitaly Kuznetsov <vkuznets@redhat.com>

5-level EPT is present in Icelake Server CPUs and is supported by QEMU
('vmx-page-walk-5').

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Message-Id: <20220221145316.576138-2-vkuznets@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/kvm/vmxcap | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/kvm/vmxcap b/scripts/kvm/vmxcap
index 6fe66d5f57..f140040104 100755
--- a/scripts/kvm/vmxcap
+++ b/scripts/kvm/vmxcap
@@ -249,6 +249,7 @@ controls = [
         bits = {
             0: 'Execute-only EPT translations',
             6: 'Page-walk length 4',
+            7: 'Page-walk length 5',
             8: 'Paging-structure memory type UC',
             14: 'Paging-structure memory type WB',
             16: '2MB EPT pages',
-- 
2.34.1

[PULL 04/11] meson: fix generic location of vss headers

[Paolo Bonzini]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This is a left-over, despite requesting the change before the merge.

Fixes: commit 8821a389 ("configure, meson: replace VSS SDK checks and options with --enable-vss-sdk")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20220222194008.610377-2-marcandre.lureau@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 8df40bfac4..b871098dbb 100644
--- a/meson.build
+++ b/meson.build
@@ -1936,7 +1936,7 @@ have_vss = false
 if targetos == 'windows' and link_language == 'cpp'
   have_vss = cxx.compiles('''
     #define __MIDL_user_allocate_free_DEFINED__
-    #include <inc/win2003/vss.h>
+    #include <vss.h>
     int main(void) { return VSS_CTX_BACKUP; }''')
 endif
 
-- 
2.34.1

[PULL 05/11] qga/vss-win32: check old VSS SDK headers

[Paolo Bonzini]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The VssCoordinator & VssAdmin interfaces have been moved to vsadmin.h in
the Windows SDK.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20220222194008.610377-3-marcandre.lureau@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                | 3 +++
 qga/vss-win32/install.cpp  | 4 ++++
 qga/vss-win32/provider.cpp | 4 ++++
 qga/vss-win32/vss-common.h | 3 ++-
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index b871098dbb..101a3f2d31 100644
--- a/meson.build
+++ b/meson.build
@@ -1933,12 +1933,15 @@ config_host_data.set('CONFIG_AF_VSOCK', cc.compiles(gnu_source_prefix + '''
   }'''))
 
 have_vss = false
+have_vss_sdk = false # old xp/2003 SDK
 if targetos == 'windows' and link_language == 'cpp'
   have_vss = cxx.compiles('''
     #define __MIDL_user_allocate_free_DEFINED__
     #include <vss.h>
     int main(void) { return VSS_CTX_BACKUP; }''')
+  have_vss_sdk = cxx.has_header('vscoordint.h')
 endif
+config_host_data.set('HAVE_VSS_SDK', have_vss_sdk)
 
 have_ntddscsi = false
 if targetos == 'windows'
diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index efc5bb9909..8076efe3cb 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -13,7 +13,11 @@
 #include "qemu/osdep.h"
 
 #include "vss-common.h"
+#ifdef HAVE_VSS_SDK
 #include <vscoordint.h>
+#else
+#include <vsadmin.h>
+#endif
 #include "install.h"
 #include <wbemidl.h>
 #include <comdef.h>
diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
index fd187fb66f..1b885e24ee 100644
--- a/qga/vss-win32/provider.cpp
+++ b/qga/vss-win32/provider.cpp
@@ -12,7 +12,11 @@
 
 #include "qemu/osdep.h"
 #include "vss-common.h"
+#ifdef HAVE_VSS_SDK
 #include <vscoordint.h>
+#else
+#include <vsadmin.h>
+#endif
 #include <vsprov.h>
 
 #define VSS_TIMEOUT_MSEC (60*1000)
diff --git a/qga/vss-win32/vss-common.h b/qga/vss-win32/vss-common.h
index 54f8de8c88..0e67e7822c 100644
--- a/qga/vss-win32/vss-common.h
+++ b/qga/vss-win32/vss-common.h
@@ -64,12 +64,13 @@ const CLSID CLSID_QGAVSSProvider = { 0x6e6a3492, 0x8d4d, 0x440c,
 const TCHAR g_szClsid[] = TEXT("{6E6A3492-8D4D-440C-9619-5E5D0CC31CA8}");
 const TCHAR g_szProgid[] = TEXT("QGAVSSProvider");
 
+#ifdef HAVE_VSS_SDK
 /* Enums undefined in VSS SDK 7.2 but defined in newer Windows SDK */
 enum __VSS_VOLUME_SNAPSHOT_ATTRIBUTES {
     VSS_VOLSNAP_ATTR_NO_AUTORECOVERY       = 0x00000002,
     VSS_VOLSNAP_ATTR_TXF_RECOVERY          = 0x02000000
 };
-
+#endif
 
 /* COM pointer utility; call ->Release() when it goes out of scope */
 template <class T>
-- 
2.34.1

[PULL 06/11] qga/vss: update informative message about MinGW

[Paolo Bonzini]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The headers are now all available in MinGW master branch.
(commit 13390dbbf885f and earlier) aiming for 10.0.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20220222194008.610377-4-marcandre.lureau@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qga/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/meson.build b/qga/meson.build
index 54f2da5b07..62472747f1 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -15,7 +15,7 @@ have_qga_vss = get_option('qga_vss') \
     If your Visual Studio installation doesn't have the VSS headers,
     Please download and install Microsoft VSS SDK:
     http://www.microsoft.com/en-us/download/details.aspx?id=23490
-    On POSIX-systems, MinGW doesn't yet provide working headers.
+    On POSIX-systems, MinGW should provide headers in >=10.0 releases.
     you can extract the SDK headers by:
     $ scripts/extract-vsssdk-headers setup.exe
     The headers are extracted in the directory 'inc/win2003'.
-- 
2.34.1

[PULL 07/11] update meson-buildoptions.sh

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/meson-buildoptions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 9ee684ef03..1e26f4571e 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -20,7 +20,6 @@ meson_options_help() {
   printf "%s\n" '  --enable-malloc=CHOICE   choose memory allocator to use [system] (choices:'
   printf "%s\n" '                           jemalloc/system/tcmalloc)'
   printf "%s\n" '  --enable-profiler        profiler support'
-  printf "%s\n" '  --enable-qga-vss         build QGA VSS support'
   printf "%s\n" '  --enable-qom-cast-debug  cast debugging support'
   printf "%s\n" '  --enable-rng-none        dummy RNG, avoid using /dev/(u)random and'
   printf "%s\n" '                           getrandom()'
@@ -97,6 +96,7 @@ meson_options_help() {
   printf "%s\n" '  parallels       parallels image format support'
   printf "%s\n" '  qcow1           qcow1 image format support'
   printf "%s\n" '  qed             qed image format support'
+  printf "%s\n" '  qga-vss         build QGA VSS support (broken with MinGW)'
   printf "%s\n" '  rbd             Ceph block device driver'
   printf "%s\n" '  replication     replication support'
   printf "%s\n" '  sdl             SDL user interface'
-- 
2.34.1

[PULL 08/11] kvm-irqchip: introduce new API to support route change

[Paolo Bonzini]

From: "Longpeng(Mike)" <longpeng2@huawei.com>

Paolo suggested adding the new API to support route changes [1]. We should invoke
kvm_irqchip_begin_route_changes() before changing the routes, increasing the
KVMRouteChange.changes if the routes are changed, and commit the changes at last.

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg02898.html

Signed-off-by: Longpeng <longpeng2@huawei.com>
Message-Id: <20220222141116.2091-2-longpeng2@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/sysemu/kvm.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 6eb39a088b..36e6d40191 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -224,6 +224,11 @@ DECLARE_INSTANCE_CHECKER(KVMState, KVM_STATE,
 extern KVMState *kvm_state;
 typedef struct Notifier Notifier;
 
+typedef struct KVMRouteChange {
+     KVMState *s;
+     int changes;
+} KVMRouteChange;
+
 /* external API */
 
 bool kvm_has_free_slot(MachineState *ms);
@@ -494,6 +499,20 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev);
 int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
                                  PCIDevice *dev);
 void kvm_irqchip_commit_routes(KVMState *s);
+
+static inline KVMRouteChange kvm_irqchip_begin_route_changes(KVMState *s)
+{
+    return (KVMRouteChange) { .s = s, .changes = 0 };
+}
+
+static inline void kvm_irqchip_commit_route_changes(KVMRouteChange *c)
+{
+    if (c->changes) {
+        kvm_irqchip_commit_routes(c->s);
+        c->changes = 0;
+    }
+}
+
 void kvm_irqchip_release_virq(KVMState *s, int virq);
 
 int kvm_irqchip_add_adapter_route(KVMState *s, AdapterInfo *adapter);
-- 
2.34.1

[PULL 09/11] kvm/msi: do explicit commit when adding msi routes

[Paolo Bonzini]

From: "Longpeng(Mike)" <longpeng2@huawei.com>

We invoke the kvm_irqchip_commit_routes() for each addition to MSI route
table, which is not efficient if we are adding lots of routes in some cases.

This patch lets callers invoke the kvm_irqchip_commit_routes(), so the
callers can decide how to optimize.

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00967.html

Signed-off-by: Longpeng <longpeng2@huawei.com>
Message-Id: <20220222141116.2091-3-longpeng2@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c    | 7 ++++---
 accel/stubs/kvm-stub.c | 2 +-
 hw/misc/ivshmem.c      | 5 ++++-
 hw/vfio/pci.c          | 5 ++++-
 hw/virtio/virtio-pci.c | 4 +++-
 include/sysemu/kvm.h   | 4 ++--
 target/i386/kvm/kvm.c  | 4 +++-
 7 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 0e66ebb497..27864dfaea 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1961,10 +1961,11 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
     return kvm_set_irq(s, route->kroute.gsi, 1);
 }
 
-int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
+int kvm_irqchip_add_msi_route(KVMRouteChange *c, int vector, PCIDevice *dev)
 {
     struct kvm_irq_routing_entry kroute = {};
     int virq;
+    KVMState *s = c->s;
     MSIMessage msg = {0, 0};
 
     if (pci_available && dev) {
@@ -2004,7 +2005,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
 
     kvm_add_routing_entry(s, &kroute);
     kvm_arch_add_msi_route_post(&kroute, vector, dev);
-    kvm_irqchip_commit_routes(s);
+    c->changes++;
 
     return virq;
 }
@@ -2162,7 +2163,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
     abort();
 }
 
-int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
+int kvm_irqchip_add_msi_route(KVMRouteChange *c, int vector, PCIDevice *dev)
 {
     return -ENOSYS;
 }
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 5319573e00..ae6e8e9aa7 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -81,7 +81,7 @@ int kvm_on_sigbus(int code, void *addr)
 }
 
 #ifndef CONFIG_USER_ONLY
-int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
+int kvm_irqchip_add_msi_route(KVMRouteChange *c, int vector, PCIDevice *dev)
 {
     return -ENOSYS;
 }
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 299837e5c1..2307f4a513 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -424,16 +424,19 @@ static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector,
                                      Error **errp)
 {
     PCIDevice *pdev = PCI_DEVICE(s);
+    KVMRouteChange c;
     int ret;
 
     IVSHMEM_DPRINTF("ivshmem_add_kvm_msi_virq vector:%d\n", vector);
     assert(!s->msi_vectors[vector].pdev);
 
-    ret = kvm_irqchip_add_msi_route(kvm_state, vector, pdev);
+    c = kvm_irqchip_begin_route_changes(kvm_state);
+    ret = kvm_irqchip_add_msi_route(&c, vector, pdev);
     if (ret < 0) {
         error_setg(errp, "kvm_irqchip_add_msi_route failed");
         return;
     }
+    kvm_irqchip_commit_route_changes(&c);
 
     s->msi_vectors[vector].virq = ret;
     s->msi_vectors[vector].pdev = pdev;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 7b45353ce2..d07a4e99b1 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -412,6 +412,7 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool msix)
 static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
                                   int vector_n, bool msix)
 {
+    KVMRouteChange c;
     int virq;
 
     if ((msix && vdev->no_kvm_msix) || (!msix && vdev->no_kvm_msi)) {
@@ -422,11 +423,13 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
         return;
     }
 
-    virq = kvm_irqchip_add_msi_route(kvm_state, vector_n, &vdev->pdev);
+    c = kvm_irqchip_begin_route_changes(kvm_state);
+    virq = kvm_irqchip_add_msi_route(&c, vector_n, &vdev->pdev);
     if (virq < 0) {
         event_notifier_cleanup(&vector->kvm_interrupt);
         return;
     }
+    kvm_irqchip_commit_route_changes(&c);
 
     if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, &vector->kvm_interrupt,
                                        NULL, virq) < 0) {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f9cf9592fd..7cf1231c1c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -683,10 +683,12 @@ static int kvm_virtio_pci_vq_vector_use(VirtIOPCIProxy *proxy,
     int ret;
 
     if (irqfd->users == 0) {
-        ret = kvm_irqchip_add_msi_route(kvm_state, vector, &proxy->pci_dev);
+        KVMRouteChange c = kvm_irqchip_begin_route_changes(kvm_state);
+        ret = kvm_irqchip_add_msi_route(&c, vector, &proxy->pci_dev);
         if (ret < 0) {
             return ret;
         }
+        kvm_irqchip_commit_route_changes(&c);
         irqfd->virq = ret;
     }
     irqfd->users++;
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 36e6d40191..e83280521a 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -486,7 +486,7 @@ void kvm_init_cpu_signals(CPUState *cpu);
 
 /**
  * kvm_irqchip_add_msi_route - Add MSI route for specific vector
- * @s:      KVM state
+ * @c:      KVMRouteChange instance.
  * @vector: which vector to add. This can be either MSI/MSIX
  *          vector. The function will automatically detect whether
  *          MSI/MSIX is enabled, and fetch corresponding MSI
@@ -495,7 +495,7 @@ void kvm_init_cpu_signals(CPUState *cpu);
  *          as @NULL, an empty MSI message will be inited.
  * @return: virq (>=0) when success, errno (<0) when failed.
  */
-int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev);
+int kvm_irqchip_add_msi_route(KVMRouteChange *c, int vector, PCIDevice *dev);
 int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
                                  PCIDevice *dev);
 void kvm_irqchip_commit_routes(KVMState *s);
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 2c8feb4a6f..cfef36a14e 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -4939,16 +4939,18 @@ void kvm_arch_init_irq_routing(KVMState *s)
     kvm_gsi_routing_allowed = true;
 
     if (kvm_irqchip_is_split()) {
+        KVMRouteChange c = kvm_irqchip_begin_route_changes(s);
         int i;
 
         /* If the ioapic is in QEMU and the lapics are in KVM, reserve
            MSI routes for signaling interrupts to the local apics. */
         for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-            if (kvm_irqchip_add_msi_route(s, 0, NULL) < 0) {
+            if (kvm_irqchip_add_msi_route(&c, 0, NULL) < 0) {
                 error_report("Could not enable split IRQ mode.");
                 exit(1);
             }
         }
+        kvm_irqchip_commit_route_changes(&c);
     }
 }
 
-- 
2.34.1

[PULL 10/11] target/i386: only include bits in pg_mode if they are not ignored

[Paolo Bonzini]

LA57/PKE/PKS is only relevant in 64-bit mode, and NXE is only relevant if
PAE is in use.  Since there is code that checks PG_MODE_LA57 to determine
the canonicality of addresses, make sure that the bit is not set by
mistake in 32-bit mode.  While it would not be a problem because 32-bit
addresses by definition fit in both 48-bit and 57-bit address spaces,
it is nicer if get_pg_mode() actually returns whether a feature is enabled,
and it allows a few simplifications in the page table walker.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/excp_helper.c | 34 ++++++++++++++--------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 5ba739fbed..0410170d64 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -24,32 +24,35 @@
 int get_pg_mode(CPUX86State *env)
 {
     int pg_mode = 0;
+    if (!(env->cr[0] & CR0_PG_MASK)) {
+        return 0;
+    }
     if (env->cr[0] & CR0_WP_MASK) {
         pg_mode |= PG_MODE_WP;
     }
     if (env->cr[4] & CR4_PAE_MASK) {
         pg_mode |= PG_MODE_PAE;
+        if (env->efer & MSR_EFER_NXE) {
+            pg_mode |= PG_MODE_NXE;
+        }
     }
     if (env->cr[4] & CR4_PSE_MASK) {
         pg_mode |= PG_MODE_PSE;
     }
-    if (env->cr[4] & CR4_PKE_MASK) {
-        pg_mode |= PG_MODE_PKE;
-    }
-    if (env->cr[4] & CR4_PKS_MASK) {
-        pg_mode |= PG_MODE_PKS;
-    }
     if (env->cr[4] & CR4_SMEP_MASK) {
         pg_mode |= PG_MODE_SMEP;
     }
-    if (env->cr[4] & CR4_LA57_MASK) {
-        pg_mode |= PG_MODE_LA57;
-    }
     if (env->hflags & HF_LMA_MASK) {
         pg_mode |= PG_MODE_LMA;
-    }
-    if (env->efer & MSR_EFER_NXE) {
-        pg_mode |= PG_MODE_NXE;
+        if (env->cr[4] & CR4_PKE_MASK) {
+            pg_mode |= PG_MODE_PKE;
+        }
+        if (env->cr[4] & CR4_PKS_MASK) {
+            pg_mode |= PG_MODE_PKS;
+        }
+        if (env->cr[4] & CR4_LA57_MASK) {
+            pg_mode |= PG_MODE_LA57;
+        }
     }
     return pg_mode;
 }
@@ -278,9 +281,7 @@ do_check_protect_pse36:
         *prot |= PAGE_EXEC;
     }
 
-    if (!(pg_mode & PG_MODE_LMA)) {
-        pkr = 0;
-    } else if (ptep & PG_USER_MASK) {
+    if (ptep & PG_USER_MASK) {
         pkr = pg_mode & PG_MODE_PKE ? env->pkru : 0;
     } else {
         pkr = pg_mode & PG_MODE_PKS ? env->pkrs : 0;
@@ -343,8 +344,7 @@ do_check_protect_pse36:
     if (is_user)
         error_code |= PG_ERROR_U_MASK;
     if (is_write1 == 2 &&
-        (((pg_mode & PG_MODE_NXE) && (pg_mode & PG_MODE_PAE)) ||
-         (pg_mode & PG_MODE_SMEP)))
+        ((pg_mode & PG_MODE_NXE) || (pg_mode & PG_MODE_SMEP)))
         error_code |= PG_ERROR_I_D_MASK;
     return error_code;
 }
-- 
2.34.1

[PULL 11/11] target/i386: Throw a #SS when loading a non-canonical IST

[Paolo Bonzini]

From: Gareth Webb <gareth.webb@umbralsoftware.co.uk>

Loading a non-canonical address into rsp when handling an interrupt or
performing a far call should raise a #SS not a #GP.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/870
Signed-off-by: Gareth Webb <gareth.webb@umbralsoftware.co.uk>
Message-Id: <164529651121.25406.15337137068584246397-0@git.sr.ht>
[Move get_pg_mode to seg_helper.c for user-mode emulators. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/seg_helper.c         | 49 +++++++++++++++++++++++++++-
 target/i386/tcg/sysemu/excp_helper.c | 36 --------------------
 2 files changed, 48 insertions(+), 37 deletions(-)

diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c
index baa905a0cd..4cf1f973cf 100644
--- a/target/i386/tcg/seg_helper.c
+++ b/target/i386/tcg/seg_helper.c
@@ -28,6 +28,42 @@
 #include "helper-tcg.h"
 #include "seg_helper.h"
 
+int get_pg_mode(CPUX86State *env)
+{
+    int pg_mode = 0;
+    if (!(env->cr[0] & CR0_PG_MASK)) {
+        return 0;
+    }
+    if (env->cr[0] & CR0_WP_MASK) {
+        pg_mode |= PG_MODE_WP;
+    }
+    if (env->cr[4] & CR4_PAE_MASK) {
+        pg_mode |= PG_MODE_PAE;
+        if (env->efer & MSR_EFER_NXE) {
+            pg_mode |= PG_MODE_NXE;
+        }
+    }
+    if (env->cr[4] & CR4_PSE_MASK) {
+        pg_mode |= PG_MODE_PSE;
+    }
+    if (env->cr[4] & CR4_SMEP_MASK) {
+        pg_mode |= PG_MODE_SMEP;
+    }
+    if (env->hflags & HF_LMA_MASK) {
+        pg_mode |= PG_MODE_LMA;
+        if (env->cr[4] & CR4_PKE_MASK) {
+            pg_mode |= PG_MODE_PKE;
+        }
+        if (env->cr[4] & CR4_PKS_MASK) {
+            pg_mode |= PG_MODE_PKS;
+        }
+        if (env->cr[4] & CR4_LA57_MASK) {
+            pg_mode |= PG_MODE_LA57;
+        }
+    }
+    return pg_mode;
+}
+
 /* return non zero if error */
 static inline int load_segment_ra(CPUX86State *env, uint32_t *e1_ptr,
                                uint32_t *e2_ptr, int selector,
@@ -795,6 +831,8 @@ static inline target_ulong get_rsp_from_tss(CPUX86State *env, int level)
 {
     X86CPU *cpu = env_archcpu(env);
     int index;
+    target_ulong rsp;
+    int32_t sext;
 
 #if 0
     printf("TR: base=" TARGET_FMT_lx " limit=%x\n",
@@ -808,7 +846,16 @@ static inline target_ulong get_rsp_from_tss(CPUX86State *env, int level)
     if ((index + 7) > env->tr.limit) {
         raise_exception_err(env, EXCP0A_TSS, env->tr.selector & 0xfffc);
     }
-    return cpu_ldq_kernel(env, env->tr.base + index);
+
+    rsp = cpu_ldq_kernel(env, env->tr.base + index);
+
+    /* test virtual address sign extension */
+    sext = rsp >> (get_pg_mode(env) & PG_MODE_LA57 ? 56 : 47);
+    if (sext != 0 && sext != -1) {
+        raise_exception_err(env, EXCP0C_STACK, 0);
+    }
+
+    return rsp;
 }
 
 /* 64 bit interrupt */
diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 0410170d64..db4c266c86 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -21,42 +21,6 @@
 #include "cpu.h"
 #include "tcg/helper-tcg.h"
 
-int get_pg_mode(CPUX86State *env)
-{
-    int pg_mode = 0;
-    if (!(env->cr[0] & CR0_PG_MASK)) {
-        return 0;
-    }
-    if (env->cr[0] & CR0_WP_MASK) {
-        pg_mode |= PG_MODE_WP;
-    }
-    if (env->cr[4] & CR4_PAE_MASK) {
-        pg_mode |= PG_MODE_PAE;
-        if (env->efer & MSR_EFER_NXE) {
-            pg_mode |= PG_MODE_NXE;
-        }
-    }
-    if (env->cr[4] & CR4_PSE_MASK) {
-        pg_mode |= PG_MODE_PSE;
-    }
-    if (env->cr[4] & CR4_SMEP_MASK) {
-        pg_mode |= PG_MODE_SMEP;
-    }
-    if (env->hflags & HF_LMA_MASK) {
-        pg_mode |= PG_MODE_LMA;
-        if (env->cr[4] & CR4_PKE_MASK) {
-            pg_mode |= PG_MODE_PKE;
-        }
-        if (env->cr[4] & CR4_PKS_MASK) {
-            pg_mode |= PG_MODE_PKS;
-        }
-        if (env->cr[4] & CR4_LA57_MASK) {
-            pg_mode |= PG_MODE_LA57;
-        }
-    }
-    return pg_mode;
-}
-
 #define PG_ERROR_OK (-1)
 
 typedef hwaddr (*MMUTranslateFunc)(CPUState *cs, hwaddr gphys, MMUAccessType access_type,
-- 
2.34.1

Re: [PULL 00/11] QEMU changes for 2021-03-02

[Peter Maydell]

On Wed, 2 Mar 2022 at 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The following changes since commit 99c53410bc9d50e556f565b0960673cccb566452:
>
>   Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2022-02-28' into staging (2022-03-01 13:25:54 +0000)
>
> are available in the Git repository at:
>
>   https://gitlab.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 9e685c6c574a9e1f1e3affbb900f7c38fb4bff6e:
>
>   target/i386: Throw a #SS when loading a non-canonical IST (2022-03-02 10:38:40 +0100)
>
> ----------------------------------------------------------------
> * whpx fixes in preparation for GDB support (Ivan)
> * VSS header fixes (Marc-André)
> * Add 5-level EPT support to vmxcap (Vitaly)
> * Bundle changes to MSI routes (Longpeng)
> * More precise emulation of #SS (Gareth)
>
> ----------------------------------------------------------------

build-oss-fuzz detects a new memory leak:
https://gitlab.com/qemu-project/qemu/-/jobs/2155668404

==7088==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 7200 byte(s) in 9 object(s) allocated from:
#0 0x5645ae447747 in __interceptor_calloc
(/builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test+0x25c747)
#1 0x7f79c6b36510 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5a510)
#2 0x5645ae48044a in walk_path
/builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/qos-test.c:225:23
#3 0x5645ae4cf97e in qos_traverse_graph
/builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/libqos/qgraph.c:417:17
#4 0x5645ae4cf97e in qos_graph_foreach_test_path
/builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/libqos/qgraph.c:737:5
#5 0x5645ae4801c4 in main
/builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/qos-test.c:334:5
#6 0x7f79c65e555f in __libc_start_call_main (/lib64/libc.so.6+0x2d55f)
#7 0x7f79c65e560b in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2d60b)
#8 0x5645ae39fba4 in _start
(/builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test+0x1b4ba4)
Indirect leak of 1152 byte(s) in 9 object(s) allocated from:
#0 0x5645ae44792f in __interceptor_realloc
(/builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test+0x25c92f)
#1 0x7f79c6b3664f in g_realloc (/lib64/libglib-2.0.so.0+0x5a64f)
#2 0x7f79c6b5260b in g_string_sized_new (/lib64/libglib-2.0.so.0+0x7660b)
#3 0x5645ae480487 in walk_path
/builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/qos-test.c:232:25
#4 0x5645ae4cf97e in qos_traverse_graph
/builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/libqos/qgraph.c:417:17
#5 0x5645ae4cf97e in qos_graph_foreach_test_path
/builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/libqos/qgraph.c:737:5
#6 0x5645ae4801c4 in main
/builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/qos-test.c:334:5
#7 0x7f79c65e555f in __libc_start_call_main (/lib64/libc.so.6+0x2d55f)
#8 0x7f79c65e560b in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2d60b)
#9 0x5645ae39fba4 in _start
(/builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test+0x1b4ba4)
SUMMARY: AddressSanitizer: 8352 byte(s) leaked in 18 allocation(s).
(test program exited with status code 1)


thanks
-- PMM

Re: [PULL 00/11] QEMU changes for 2021-03-02

[Paolo Bonzini]

On 3/2/22 21:55, Peter Maydell wrote:
> On Wed, 2 Mar 2022 at 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> The following changes since commit 99c53410bc9d50e556f565b0960673cccb566452:
>>
>>    Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2022-02-28' into staging (2022-03-01 13:25:54 +0000)
>>
>> are available in the Git repository at:
>>
>>    https://gitlab.com/bonzini/qemu.git tags/for-upstream
>>
>> for you to fetch changes up to 9e685c6c574a9e1f1e3affbb900f7c38fb4bff6e:
>>
>>    target/i386: Throw a #SS when loading a non-canonical IST (2022-03-02 10:38:40 +0100)
>>
>> ----------------------------------------------------------------
>> * whpx fixes in preparation for GDB support (Ivan)
>> * VSS header fixes (Marc-André)
>> * Add 5-level EPT support to vmxcap (Vitaly)
>> * Bundle changes to MSI routes (Longpeng)
>> * More precise emulation of #SS (Gareth)
>>
>> ----------------------------------------------------------------
> 
> build-oss-fuzz detects a new memory leak:
> https://gitlab.com/qemu-project/qemu/-/jobs/2155668404

... which is impossible given what the pull request changes; the leak is 
in qos-test (the test executable itself, not QEMU) and the only non-QEMU 
changes in this pull request (to the build system) are for Windows 
systems only.  I've seen hangs of qos-test in the past, as well as this 
leak, and they've become more common lately.

The test seems to be flaky, I've been fighting with it all week---trying 
multiple versions of this pull request and removing patches until 
build-oss-fuzz passed.  The set of patches that triggered it or not was 
completely random, but I'll not that it did pass with this exact commit 
I'm submitting (https://gitlab.com/bonzini/qemu/-/jobs/2154365356).

I wanted to look at this today again before replying to you, but as you 
know I was sidetracked by work on the qemu.org infrastructure.  So, I 
can look at this but I really need to ask you one of two favors:

1) decide that the test is flaky and merge this pull request, and then 
I'll send before Monday the changes that I've omitted here (which again 
have nothing to do with qos-test).  I'll look at qos-test during soft 
freeze.

2) accept that I'll send another x86 pull request (not a large one) 
after soft freeze, so that I have more time to debug this (likely 
unrelated) build-oss-fuzz issue.

Paolo

> ==7088==ERROR: LeakSanitizer: detected memory leaks
> Direct leak of 7200 byte(s) in 9 object(s) allocated from:
> #0 0x5645ae447747 in __interceptor_calloc
> (/builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test+0x25c747)
> #1 0x7f79c6b36510 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5a510)
> #2 0x5645ae48044a in walk_path
> /builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/qos-test.c:225:23
> #3 0x5645ae4cf97e in qos_traverse_graph
> /builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/libqos/qgraph.c:417:17
> #4 0x5645ae4cf97e in qos_graph_foreach_test_path
> /builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/libqos/qgraph.c:737:5
> #5 0x5645ae4801c4 in main
> /builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/qos-test.c:334:5
> #6 0x7f79c65e555f in __libc_start_call_main (/lib64/libc.so.6+0x2d55f)
> #7 0x7f79c65e560b in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2d60b)
> #8 0x5645ae39fba4 in _start
> (/builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test+0x1b4ba4)
> Indirect leak of 1152 byte(s) in 9 object(s) allocated from:
> #0 0x5645ae44792f in __interceptor_realloc
> (/builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test+0x25c92f)
> #1 0x7f79c6b3664f in g_realloc (/lib64/libglib-2.0.so.0+0x5a64f)
> #2 0x7f79c6b5260b in g_string_sized_new (/lib64/libglib-2.0.so.0+0x7660b)
> #3 0x5645ae480487 in walk_path
> /builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/qos-test.c:232:25
> #4 0x5645ae4cf97e in qos_traverse_graph
> /builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/libqos/qgraph.c:417:17
> #5 0x5645ae4cf97e in qos_graph_foreach_test_path
> /builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/libqos/qgraph.c:737:5
> #6 0x5645ae4801c4 in main
> /builds/qemu-project/qemu/build-oss-fuzz/../tests/qtest/qos-test.c:334:5
> #7 0x7f79c65e555f in __libc_start_call_main (/lib64/libc.so.6+0x2d55f)
> #8 0x7f79c65e560b in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2d60b)
> #9 0x5645ae39fba4 in _start
> (/builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test+0x1b4ba4)
> SUMMARY: AddressSanitizer: 8352 byte(s) leaked in 18 allocation(s).
> (test program exited with status code 1)

Re: [PULL 00/11] QEMU changes for 2021-03-02

[Peter Maydell]

On Fri, 4 Mar 2022 at 17:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The test seems to be flaky, I've been fighting with it all week---trying
> multiple versions of this pull request and removing patches until
> build-oss-fuzz passed.  The set of patches that triggered it or not was
> completely random, but I'll not that it did pass with this exact commit
> I'm submitting (https://gitlab.com/bonzini/qemu/-/jobs/2154365356).
>
> I wanted to look at this today again before replying to you, but as you
> know I was sidetracked by work on the qemu.org infrastructure.  So, I
> can look at this but I really need to ask you one of two favors:
>
> 1) decide that the test is flaky and merge this pull request, and then
> I'll send before Monday the changes that I've omitted here (which again
> have nothing to do with qos-test).  I'll look at qos-test during soft
> freeze.
>
> 2) accept that I'll send another x86 pull request (not a large one)
> after soft freeze, so that I have more time to debug this (likely
> unrelated) build-oss-fuzz issue.

Either of these is fine; my requirement is only that either:
 (1) the oss-fuzz gitlab CI job needs to in practice actually
pass at least most of the time
 (2) we need to switch it to ok-to-fail or disable it

so I don't have CI failing for every merge I make.

We seem to have several intermittents right now (including one
which makes oss-fuzz hang, I think) which I'll try to find time
to investigate soon. Plus the CI infra in general is flaky:
some of the intermittents are clearly gitlab issues (like failing
to manage to git clone things).

thanks
-- PMM

Re: [PULL 00/11] QEMU changes for 2021-03-02

[Daniel P. Berrangé]

On Fri, Mar 04, 2022 at 06:46:51PM +0000, Peter Maydell wrote:
> On Fri, 4 Mar 2022 at 17:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > The test seems to be flaky, I've been fighting with it all week---trying
> > multiple versions of this pull request and removing patches until
> > build-oss-fuzz passed.  The set of patches that triggered it or not was
> > completely random, but I'll not that it did pass with this exact commit
> > I'm submitting (https://gitlab.com/bonzini/qemu/-/jobs/2154365356).
> >
> > I wanted to look at this today again before replying to you, but as you
> > know I was sidetracked by work on the qemu.org infrastructure.  So, I
> > can look at this but I really need to ask you one of two favors:
> >
> > 1) decide that the test is flaky and merge this pull request, and then
> > I'll send before Monday the changes that I've omitted here (which again
> > have nothing to do with qos-test).  I'll look at qos-test during soft
> > freeze.
> >
> > 2) accept that I'll send another x86 pull request (not a large one)
> > after soft freeze, so that I have more time to debug this (likely
> > unrelated) build-oss-fuzz issue.
> 
> Either of these is fine; my requirement is only that either:
>  (1) the oss-fuzz gitlab CI job needs to in practice actually
> pass at least most of the time
>  (2) we need to switch it to ok-to-fail or disable it
> 
> so I don't have CI failing for every merge I make.

This is far from the first time that oss-fuzz has caused us pain. It
feels like it has been flaky  for prolonged periods of time, for as
long as it has existed.

When I tried to switch CI to use Fedora 35 oss-fuzz was consistently
failing for months for no obvious reason that I could determine
despite days of debugging. Then one day I woke up and it magically
started working again, for no obvious reason. Inexplicable.

Conceptually we benefit from fuzzing to find obscure bugs.
Have we actually found any real bugs from the oss-fuzz CI
job we have though ? Between us all, we've certainly lost
many many many days of developer time debugging the thing
for false failures.

The magic set of args needed to run it make it even more
unpleasant to deal with, and that scripts/oss-fuzz/build.sh
runs different code inside GitLab vs outside GitLab also
make debugging worse.

Personally I favour turning it into a non-gating job as
I don't want to invest more of my own time debugging
non-bugs in it.

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: [PULL 00/11] QEMU changes for 2021-03-02

[Peter Maydell]

On Fri, 4 Mar 2022 at 19:15, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Fri, Mar 04, 2022 at 06:46:51PM +0000, Peter Maydell wrote:
> > Either of these is fine; my requirement is only that either:
> >  (1) the oss-fuzz gitlab CI job needs to in practice actually
> > pass at least most of the time
> >  (2) we need to switch it to ok-to-fail or disable it
> >
> > so I don't have CI failing for every merge I make.
>
> This is far from the first time that oss-fuzz has caused us pain. It
> feels like it has been flaky  for prolonged periods of time, for as
> long as it has existed.
>
> When I tried to switch CI to use Fedora 35 oss-fuzz was consistently
> failing for months for no obvious reason that I could determine
> despite days of debugging. Then one day I woke up and it magically
> started working again, for no obvious reason. Inexplicable.
>
> Conceptually we benefit from fuzzing to find obscure bugs.
> Have we actually found any real bugs from the oss-fuzz CI
> job we have though ?

It did find a buffer-overrun bug in the 9p pullreq less than
a month ago:
https://lore.kernel.org/qemu-devel/CAFEAcA-VRNzxOwMX4nPPm0vQba1ufL5yVwW5P1j9S2u7_fbW-w@mail.gmail.com/

But overall I'm sympathetic to the idea that as it stands it's
costing us more than it's helping.

-- PMM

Re: [PULL 00/11] QEMU changes for 2021-03-02

[Daniel P. Berrangé]

On Fri, Mar 04, 2022 at 07:22:16PM +0000, Peter Maydell wrote:
> On Fri, 4 Mar 2022 at 19:15, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Fri, Mar 04, 2022 at 06:46:51PM +0000, Peter Maydell wrote:
> > > Either of these is fine; my requirement is only that either:
> > >  (1) the oss-fuzz gitlab CI job needs to in practice actually
> > > pass at least most of the time
> > >  (2) we need to switch it to ok-to-fail or disable it
> > >
> > > so I don't have CI failing for every merge I make.
> >
> > This is far from the first time that oss-fuzz has caused us pain. It
> > feels like it has been flaky  for prolonged periods of time, for as
> > long as it has existed.
> >
> > When I tried to switch CI to use Fedora 35 oss-fuzz was consistently
> > failing for months for no obvious reason that I could determine
> > despite days of debugging. Then one day I woke up and it magically
> > started working again, for no obvious reason. Inexplicable.
> >
> > Conceptually we benefit from fuzzing to find obscure bugs.
> > Have we actually found any real bugs from the oss-fuzz CI
> > job we have though ?
> 
> It did find a buffer-overrun bug in the 9p pullreq less than
> a month ago:
> https://lore.kernel.org/qemu-devel/CAFEAcA-VRNzxOwMX4nPPm0vQba1ufL5yVwW5P1j9S2u7_fbW-w@mail.gmail.com/

Interesting. I wonder whether that detection is specifically related
to the fuzzing, or whether it is something we would have seen merely
by building with 'asan' and running 'make check' as normal.

IIUC, the oss-fuzz job we run in GitLab CI is mostly just a sanity
check and the real fuzzing work takes place in Google's fuzz service.

> But overall I'm sympathetic to the idea that as it stands it's
> costing us more than it's helping.

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: [PULL 00/11] QEMU changes for 2021-03-02

[Paolo Bonzini]

On 3/4/22 20:30, Daniel P. Berrangé wrote:
> Interesting. I wonder whether that detection is specifically related
> to the fuzzing, or whether it is something we would have seen merely
> by building with 'asan' and running 'make check' as normal.
> 
> IIUC, the oss-fuzz job we run in GitLab CI is mostly just a sanity
> check and the real fuzzing work takes place in Google's fuzz service.

If the intermittent leak is indeed a leak, it's not related to fuzzing 
and it would be found with with asan.

OTOH, if the intermittent leak is not a leak, it probably would be flaky 
even with just asan.

Paolo

Re: [PULL 00/11] QEMU changes for 2021-03-02

[Richard Henderson]

On 3/4/22 09:15, Daniel P. Berrangé wrote:
> Personally I favour turning it into a non-gating job as
> I don't want to invest more of my own time debugging
> non-bugs in it.

I would be in favor of removing it from CI entirely, so that we don't even waste cpu time 
on it by default.  Those that care can run it manually within their own build.


r~