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

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

~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