[PATCH qemu v6 1/1] Honor guest PAT on x86, absent Bochs display

[PATCH qemu v6 1/1] Honor guest PAT on x86, absent Bochs display

[~myrslint]

From: myrslint <qemu.haziness801@passinbox.com>

On x86_64, where most CPUs support self-snoop, it is preferrable to
always honor guest PAT. Not doing so is a quirk. There is a default
enabled KVM quirk flag which enforces not doing so due to a former bug
in Bochs display driver.

The bug has been fixed but not enough has yet passed since so we only
disable said quirk flag if a Bochs display is not configured for the
virtual machine.

This commit also moves around a bit of code that would be called when
the initialization of a VM is done.

Signed-off-by: myrslint <qemu.haziness801@passinbox.com>
---
 accel/kvm/kvm-all.c        |  1 +
 accel/stubs/kvm-stub.c     |  1 +
 hw/display/bochs-display.c |  5 +++++
 include/system/kvm.h       |  9 ++++++++
 target/i386/kvm/kvm.c      | 42 +++++++++++++++++++++++++++++++-------
 5 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 58802f7c3c..391ec08629 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -103,6 +103,7 @@ bool kvm_readonly_mem_allowed;
 bool kvm_vm_attributes_allowed;
 bool kvm_msi_use_devid;
 bool kvm_pre_fault_memory_supported;
+bool kvm_bochs_drm;
 static bool kvm_has_guest_debug;
 static int kvm_sstep_flags;
 static bool kvm_immediate_exit;
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 68cd33ba97..e56b745d5d 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -24,6 +24,7 @@ bool kvm_gsi_direct_mapping;
 bool kvm_allowed;
 bool kvm_readonly_mem_allowed;
 bool kvm_msi_use_devid;
+bool kvm_bochs_drm;
 
 void kvm_flush_coalesced_mmio_buffer(void)
 {
diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index ad2821c974..9a33a4cd26 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -20,6 +20,8 @@
 #include "ui/qemu-pixman.h"
 #include "qom/object.h"
 
+#include "system/kvm.h"
+
 typedef struct BochsDisplayMode {
     pixman_format_code_t format;
     uint32_t             bytepp;
@@ -261,6 +263,7 @@ static const GraphicHwOps bochs_display_gfx_ops = {
     .gfx_update = bochs_display_update,
 };
 
+
 static void bochs_display_realize(PCIDevice *dev, Error **errp)
 {
     BochsDisplayState *s = BOCHS_DISPLAY(dev);
@@ -309,6 +312,8 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
     }
 
     memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
+
+    kvm_bochs_drm = true;
 }
 
 static bool bochs_display_get_big_endian_fb(Object *obj, Error **errp)
diff --git a/include/system/kvm.h b/include/system/kvm.h
index 4fc09e3891..62b49deb3d 100644
--- a/include/system/kvm.h
+++ b/include/system/kvm.h
@@ -43,6 +43,7 @@ extern bool kvm_gsi_direct_mapping;
 extern bool kvm_readonly_mem_allowed;
 extern bool kvm_msi_use_devid;
 extern bool kvm_pre_fault_memory_supported;
+extern bool kvm_bochs_drm;
 
 #define kvm_enabled()           (kvm_allowed)
 /**
@@ -144,6 +145,13 @@ extern bool kvm_pre_fault_memory_supported;
  */
 #define kvm_msi_devid_required() (kvm_msi_use_devid)
 
+/**
+ * kvm_has_bochs_drm:
+ * Returns: true if KVM is possible and a Bochs DRM driver is
+ * in use for display.
+ */
+#define kvm_has_bochs_drm() (kvm_bochs_drm)
+
 #else
 
 #define kvm_enabled()           (0)
@@ -158,6 +166,7 @@ extern bool kvm_pre_fault_memory_supported;
 #define kvm_gsi_direct_mapping() (false)
 #define kvm_readonly_mem_enabled() (false)
 #define kvm_msi_devid_required() (false)
+#define kvm_has_bochs_drm() (false)
 
 #endif  /* CONFIG_KVM_IS_POSSIBLE */
 
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index db40caa341..a720cc791b 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2692,13 +2692,13 @@ static bool kvm_rdmsr_pkg_energy_status(X86CPU *cpu,
     return true;
 }
 
-static Notifier smram_machine_done;
+static Notifier kvm_machine_done;
 static KVMMemoryListener smram_listener;
 static AddressSpace smram_address_space;
 static MemoryRegion smram_as_root;
 static MemoryRegion smram_as_mem;
 
-static void register_smram_listener(Notifier *n, void *unused)
+static void register_smram_listener(void)
 {
     CPUState *cpu;
     MemoryRegion *smram =
@@ -2731,6 +2731,37 @@ static void register_smram_listener(Notifier *n, void *unused)
     }
 }
 
+static bool kvm_x86_smm_enabled(void)
+{
+    return object_dynamic_cast(OBJECT(current_machine), TYPE_X86_MACHINE) &&
+        x86_machine_is_smm_enabled(X86_MACHINE(current_machine));
+}
+
+static int kvm_x86_disable_quirsk2_mask(void)
+{
+    return kvm_check_extension(kvm_state, KVM_CAP_DISABLE_QUIRKS2);
+}
+
+static int kvm_disable_ignore_guest_pat(void)
+{
+    return kvm_vm_enable_cap(kvm_state, KVM_CAP_DISABLE_QUIRKS2, 0, \
+                             KVM_X86_QUIRK_IGNORE_GUEST_PAT);
+}
+
+static void handle_machine_done(Notifier *n, void *unused)
+{
+    if (kvm_x86_smm_enabled()) {
+        register_smram_listener();
+    }
+    if (!kvm_has_bochs_drm() && \
+        (kvm_x86_disable_quirsk2_mask() & KVM_X86_QUIRK_IGNORE_GUEST_PAT)) {
+        if (kvm_disable_ignore_guest_pat()) {
+            error_report("KVM_X86_QUIRK_IGNORE_GUEST_PAT available and "
+                         "modifiable but we failed to disable it\n");
+        }
+    }
+}
+
 static void *kvm_msr_energy_thread(void *data)
 {
     KVMState *s = data;
@@ -3311,11 +3342,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         return ret;
     }
 
-    if (object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE) &&
-        x86_machine_is_smm_enabled(X86_MACHINE(ms))) {
-        smram_machine_done.notify = register_smram_listener;
-        qemu_add_machine_init_done_notifier(&smram_machine_done);
-    }
+    kvm_machine_done.notify = handle_machine_done;
+    qemu_add_machine_init_done_notifier(&kvm_machine_done);
 
     if (enable_cpu_pm) {
         ret = kvm_vm_enable_disable_exits(s);
-- 
2.49.1

[PATCH qemu v6 0/1] Honor guest PAT on x86, absent Bochs display

[~myrslint]

This is a rebase and revision of the proposed patch to disable KVM
ignore guest PAT quirk, on x86_64 target, when KVM is available and
Bochs DRM display driver is not used. Hopefully, it alleviates the point
raised by Dmitry Osipenko <dmitry.osipenko@collabora.com>. I have tested
compiling for x86_64 and aarch64 targets to make sure no compile-time
errors are raised.

I have also tested that this issue is addressed by the patch:
https://gitlab.com/qemu-project/qemu/-/issues/2943

myrslint (1):
  Honor guest PAT on x86, absent Bochs display

 accel/kvm/kvm-all.c        |  1 +
 accel/stubs/kvm-stub.c     |  1 +
 hw/display/bochs-display.c |  5 +++++
 include/system/kvm.h       |  9 ++++++++
 target/i386/kvm/kvm.c      | 42 +++++++++++++++++++++++++++++++-------
 5 files changed, 51 insertions(+), 7 deletions(-)

-- 
2.49.1

Re: [PATCH qemu v6 1/1] Honor guest PAT on x86, absent Bochs display

[Dmitry Osipenko]

Hi,

On 10/10/25 05:43, ~myrslint wrote:
> From: myrslint <qemu.haziness801@passinbox.com>
> 
> On x86_64, where most CPUs support self-snoop, it is preferrable to
> always honor guest PAT. Not doing so is a quirk. There is a default
> enabled KVM quirk flag which enforces not doing so due to a former bug
> in Bochs display driver.
> 
> The bug has been fixed but not enough has yet passed since so we only
> disable said quirk flag if a Bochs display is not configured for the
> virtual machine.
> 
> This commit also moves around a bit of code that would be called when
> the initialization of a VM is done.
> 
> Signed-off-by: myrslint <qemu.haziness801@passinbox.com>
> ---
>  accel/kvm/kvm-all.c        |  1 +
>  accel/stubs/kvm-stub.c     |  1 +
>  hw/display/bochs-display.c |  5 +++++
>  include/system/kvm.h       |  9 ++++++++
>  target/i386/kvm/kvm.c      | 42 +++++++++++++++++++++++++++++++-------
>  5 files changed, 51 insertions(+), 7 deletions(-)
> 

...
> +#include "system/kvm.h"
> +
>  typedef struct BochsDisplayMode {
>      pixman_format_code_t format;
>      uint32_t             bytepp;
> @@ -261,6 +263,7 @@ static const GraphicHwOps bochs_display_gfx_ops = {
>      .gfx_update = bochs_display_update,
>  };
>  
> +

Nit: please remove the extra newline

>  static void bochs_display_realize(PCIDevice *dev, Error **errp)
>  {
>      BochsDisplayState *s = BOCHS_DISPLAY(dev);
> @@ -309,6 +312,8 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
>      }
>  
>      memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
> +
> +    kvm_bochs_drm = true;

The patch looks good, only minor suggestions from me:

1. rename kvm_bochs_drm -> kvm_bochs_drm_quirk for more clarity

2. add clarifying comment to the code explaining what this quirk does
and why it's needed, basically repeating the commit msg

...
> +static void handle_machine_done(Notifier *n, void *unused)
> +{
> +    if (kvm_x86_smm_enabled()) {
> +        register_smram_listener();
> +    }
> +    if (!kvm_has_bochs_drm() && \
> +        (kvm_x86_disable_quirsk2_mask() & KVM_X86_QUIRK_IGNORE_GUEST_PAT)) {
> +        if (kvm_disable_ignore_guest_pat()) {
> +            error_report("KVM_X86_QUIRK_IGNORE_GUEST_PAT available and "
> +                         "modifiable but we failed to disable it\n");

Nit: the extra "\n" shouldn't be needed for error_report()


-- 
Best regards,
Dmitry