[PATCH qemu 0/1] A small patch to address Issue #2943

[PATCH qemu 0/1] A small patch to address Issue #2943

[~myrslint]

Details of the issue can be found at: https://gitlab.com/qemu-
project/qemu/-/issues/2943

This patch by default disables the KVM_X86_QUIRK_IGNORE_GUEST_PAT quirk
because most Intel CPUs in current use do not need it. The bochs video
driver bug which required it has been fixed as well.

The end result is that guest PAT is honored.

myrslint (1):
  Default disable ignore guest PAT quirk

 accel/kvm/kvm-all.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

-- 
2.49.1

[PATCH qemu 1/1] Default disable ignore guest PAT quirk

[~myrslint]

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

Addresses this issue: https://gitlab.com/qemu-project/qemu/-/issues/2943

Most Intel CPUs in current use have self-snoop. The few added lines of
code also check for availability of the quirk disablement option so if
some CPU does not have this feature no change of behavior will occur.
---
 accel/kvm/kvm-all.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 890d5ea9f8..c3d06ae2f8 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2682,6 +2682,32 @@ static int kvm_init(AccelState *as, MachineState *ms)
 
     s->vmfd = ret;
 
+/* if target platform has no notion of this or kernel version does
+ * not have it there is no use for compiling this in */
+#ifdef KVM_X86_QUIRK_IGNORE_GUEST_PAT
+    /* first check for modifiable quirks bitmask */
+    ret = kvm_check_extension(s, KVM_CAP_DISABLE_QUIRKS2);
+    /* next make sure disabling it is allowed */
+    if (ret & KVM_X86_QUIRK_IGNORE_GUEST_PAT) {
+        struct kvm_enable_cap *cap;
+        cap = calloc(1, sizeof(struct kvm_enable_cap));
+        if (cap) {
+            cap->cap = KVM_CAP_DISABLE_QUIRKS2;
+            cap->args[0] = KVM_X86_QUIRK_IGNORE_GUEST_PAT;
+            /* if intel cpu does not support self-snoop this is a nop */
+            ret = kvm_vm_ioctl(s, KVM_ENABLE_CAP, cap);
+            if (ret < 0) {
+                error_printf("KVM_X86_QUIRK_IGNORE_GUEST_PAT available and "
+                             "modifiable but we failed to disable it\n");
+            }
+            free(cap);
+        } else {
+            error_printf("KVM_X86_QUIRK_IGNORE_GUEST_PAT: could not "
+                         "allocate memory\n");
+        }
+    }
+#endif
+
     s->nr_as = kvm_vm_check_extension(s, KVM_CAP_MULTI_ADDRESS_SPACE);
     if (s->nr_as <= 1) {
         s->nr_as = 1;
-- 
2.49.1

Re: [PATCH qemu 1/1] Default disable ignore guest PAT quirk

[Alex Bennée]

~myrslint <myrslint@git.sr.ht> writes:

> From: myrslint <qemu.haziness801@passinbox.com>
>
> Addresses this issue:
> https://gitlab.com/qemu-project/qemu/-/issues/2943

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2943

> Most Intel CPUs in current use have self-snoop. The few added lines of
> code also check for availability of the quirk disablement option so if
> some CPU does not have this feature no change of behavior will occur.
> ---
>  accel/kvm/kvm-all.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 890d5ea9f8..c3d06ae2f8 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2682,6 +2682,32 @@ static int kvm_init(AccelState *as, MachineState *ms)
>  
>      s->vmfd = ret;
>  
> +/* if target platform has no notion of this or kernel version does
> + * not have it there is no use for compiling this in */
> +#ifdef KVM_X86_QUIRK_IGNORE_GUEST_PAT

architecture specific changes should be in kvm_arch_init (in this case
in target/i386/kvm/kvm.c)

> +    /* first check for modifiable quirks bitmask */
> +    ret = kvm_check_extension(s, KVM_CAP_DISABLE_QUIRKS2);
> +    /* next make sure disabling it is allowed */
> +    if (ret & KVM_X86_QUIRK_IGNORE_GUEST_PAT) {
> +        struct kvm_enable_cap *cap;
> +        cap = calloc(1, sizeof(struct kvm_enable_cap));

I few things, style says:

  Use of the ``malloc/free/realloc/calloc/valloc/memalign/posix_memalign``
  APIs is not allowed in the QEMU codebase. Instead of these routines,
  use the GLib memory allocation routines
  ``g_malloc/g_malloc0/g_new/g_new0/g_realloc/g_free``
  or QEMU's ``qemu_memalign/qemu_blockalign/qemu_vfree`` APIs.

And to avoid manual free you would do:

  g_autofree kvm_enable_cap *cap = g_new0(kvm_enable_cap, 1);

However why not just have a structure on the stack, e.g:

  struct kvm_enable_cap cap = { .cap = KVM_CAP_DISABLE_QUIRKS2,
                                .args = { KVM_X86_QUIRK_IGNORE_GUEST_PAT };

In fact looking at kvm_vcpu_enable_cap() you should probably just use that.

> +        if (cap) {
> +            cap->cap = KVM_CAP_DISABLE_QUIRKS2;
> +            cap->args[0] = KVM_X86_QUIRK_IGNORE_GUEST_PAT;
> +            /* if intel cpu does not support self-snoop this is a nop */
> +            ret = kvm_vm_ioctl(s, KVM_ENABLE_CAP, cap);
> +            if (ret < 0) {
> +                error_printf("KVM_X86_QUIRK_IGNORE_GUEST_PAT available and "
> +                             "modifiable but we failed to disable
> it\n");

I think this should be error_report

> +            }
> +            free(cap);
> +        } else {

You don't need the else leg if you are dynamically allocating as g_new
and friends will abort().

> +            error_printf("KVM_X86_QUIRK_IGNORE_GUEST_PAT: could not "
> +                         "allocate memory\n");
> +        }
> +    }
> +#endif
> +
>      s->nr_as = kvm_vm_check_extension(s, KVM_CAP_MULTI_ADDRESS_SPACE);
>      if (s->nr_as <= 1) {
>          s->nr_as = 1;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro