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

["Alex Bennée" <alex.bennee@linaro.org>]

~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