[PATCH qemu v2 1/1] Default disable ignore guest PAT quirk (second revision)

[PATCH qemu v2 1/1] Default disable ignore guest PAT quirk (second revision)

[~myrslint]

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

Thanks to Alex Bennée <alex.bennee@linaro.org> for the kind code review
and helpful guidance. This is a second attempt at addressing 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.

Signed-off-by: Myrsky Lintu <qemu.haziness801@passinbox.com>
---
Hopefully, I have improved the patch based on kindly provided code
review.

The only point of divergence is that per

https://www.kernel.org/doc/html/latest/virt/kvm/api.html

this is a VM capability (section 7), not a VCPU one, so a call is made
to kvm_vm_enable_cap() rather than kvm_vcpu_enable_cap().

 target/i386/kvm/kvm.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 369626f8c8..124818bf94 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -16,6 +16,7 @@
 #include "qapi/qapi-events-run-state.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include <asm-x86/kvm.h>
 #include <math.h>
 #include <sys/ioctl.h>
 #include <sys/utsname.h>
@@ -3367,6 +3368,24 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
+/* if kernel version does not have it there is no point compiling this in */
+#ifdef KVM_X86_QUIRK_IGNORE_GUEST_PAT
+    /* rationale: most x86 cpus in current use have self-snoop so honoring
+     * guest pat is preferrable. as well, the bochs video driver bug which
+     * motivated making this a default enabled quirk in kvm was fixed long ago
+     * */
+    /* check if disabling this quirk is feasible and allowed */
+    ret = kvm_check_extension(s, KVM_CAP_DISABLE_QUIRKS2);
+    if (ret & KVM_X86_QUIRK_IGNORE_GUEST_PAT) {
+        ret = kvm_vm_enable_cap(s, KVM_CAP_DISABLE_QUIRKS2, 0, \
+                                KVM_X86_QUIRK_IGNORE_GUEST_PAT);
+        if (ret < 0) {
+            error_report("KVM_X86_QUIRK_IGNORE_GUEST_PAT available and "
+                         "modifiable but we failed to disable it\n");
+        }
+    }
+#endif
+
     return 0;
 }
 
-- 
2.49.1

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

[~myrslint]

Details of the issue addressed can be found here:
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.

myrslint (1):
  Default disable ignore guest PAT quirk (second revision)

 target/i386/kvm/kvm.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.49.1

Re: [PATCH qemu v2 1/1] Default disable ignore guest PAT quirk (second revision)

[Alex Bennée]

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

> From: myrslint <qemu.haziness801@passinbox.com>
>
> Thanks to Alex Bennée <alex.bennee@linaro.org> for the kind code review
> and helpful guidance.

This doesn't belong in the commit message, you can add it bellow the ---
if you want to track the changes while iterating a patch:

  https://qemu.readthedocs.io/en/master/devel/submitting-a-patch.html#id21

> This is a second attempt at addressing this issue:
> https://gitlab.com/qemu-project/qemu/-/issues/2943

This should be a trailer:

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

bellow your signed-of-by.

> 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.
>
> Signed-off-by: Myrsky Lintu <qemu.haziness801@passinbox.com>
> ---
> Hopefully, I have improved the patch based on kindly provided code
> review.
>
> The only point of divergence is that per
>
> https://www.kernel.org/doc/html/latest/virt/kvm/api.html
>
> this is a VM capability (section 7), not a VCPU one, so a call is made
> to kvm_vm_enable_cap() rather than kvm_vcpu_enable_cap().
>
>  target/i386/kvm/kvm.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 369626f8c8..124818bf94 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -16,6 +16,7 @@
>  #include "qapi/qapi-events-run-state.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
> +#include <asm-x86/kvm.h>
>  #include <math.h>
>  #include <sys/ioctl.h>
>  #include <sys/utsname.h>
> @@ -3367,6 +3368,24 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>          }
>      }
>  
> +/* if kernel version does not have it there is no point compiling this in */
> +#ifdef KVM_X86_QUIRK_IGNORE_GUEST_PAT
> +    /* rationale: most x86 cpus in current use have self-snoop so honoring
> +     * guest pat is preferrable. as well, the bochs video driver bug which
> +     * motivated making this a default enabled quirk in kvm was fixed long ago
> +     * */
> +    /* check if disabling this quirk is feasible and allowed */
> +    ret = kvm_check_extension(s, KVM_CAP_DISABLE_QUIRKS2);
> +    if (ret & KVM_X86_QUIRK_IGNORE_GUEST_PAT) {
> +        ret = kvm_vm_enable_cap(s, KVM_CAP_DISABLE_QUIRKS2, 0, \
> +                                KVM_X86_QUIRK_IGNORE_GUEST_PAT);
> +        if (ret < 0) {
> +            error_report("KVM_X86_QUIRK_IGNORE_GUEST_PAT available and "
> +                         "modifiable but we failed to disable it\n");
> +        }
> +    }
> +#endif

This looks good now, I'll leave it to the x86 maintainers to decide on
the automatic enabling of this feature.

> +
>      return 0;
>  }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro