Re: [WARNING: UNSCANNABLE EXTRACTION FAILED][PATCH v2 1/3] selftest: kvm: Reorder vcpu_load_state steps for AMX

[Sean Christopherson <seanjc@google.com>]

On Wed, Dec 22, 2021, Yang Zhong wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> For AMX support it is recommended to load XCR0 after XFD, so
> that KVM does not see XFD=0, XCR=1 for a save state that will
> eventually be disabled (which would lead to premature allocation
> of the space required for that save state).

It would be very helpful to clarify that XFD is loaded via KVM_SET_MSRS.  It took
me longer than it should have to understand what was going on.  The large amount of
whitespace noise in this patch certainly didn't help.  E.g. just a simple tweak:

  For AMX support it is recommended to load XCR0 after XFD, i.e. after MSRs, so

> It is also required to load XSAVE data after XCR0 and XFD, so
> that KVM can trigger allocation of the extra space required to
> store AMX state.
> 
> Adjust vcpu_load_state to obey these new requirements.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  .../selftests/kvm/lib/x86_64/processor.c      | 29 ++++++++++---------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 00324d73c687..9b5abf488211 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1192,9 +1192,14 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_x86_state *s
>  	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
>  	int r;
>  
> -	r = ioctl(vcpu->fd, KVM_SET_XSAVE, &state->xsave);
> -        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_XSAVE, r: %i",
> -                r);
> +	r = ioctl(vcpu->fd, KVM_SET_SREGS, &state->sregs);
> +	TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_SREGS, r: %i",
> +		r);

If we're going to bother replacing spaces with tabs, might as well get rid of all
the gratuituous newlines as well.

> +
> +	r = ioctl(vcpu->fd, KVM_SET_MSRS, &state->msrs);
> +	TEST_ASSERT(r == state->msrs.nmsrs,
> +		"Unexpected result from KVM_SET_MSRS,r: %i (failed at %x)",
> +		r, r == state->msrs.nmsrs ? -1 : state->msrs.entries[r].index);

Most people not named "Paolo" prefer to align this with the opening "(" :-)

E.g.

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 97f8c2f2df36..971f41afa689 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1158,44 +1158,36 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_x86_state *s
        int r;

        r = ioctl(vcpu->fd, KVM_SET_SREGS, &state->sregs);
-       TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_SREGS, r: %i",
-               r);
+       TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_SREGS, r: %i", r);

        r = ioctl(vcpu->fd, KVM_SET_MSRS, &state->msrs);
        TEST_ASSERT(r == state->msrs.nmsrs,
-               "Unexpected result from KVM_SET_MSRS,r: %i (failed at %x)",
-               r, r == state->msrs.nmsrs ? -1 : state->msrs.entries[r].index);
+                   "Unexpected result from KVM_SET_MSRS,r: %i (failed at %x)",
+                   r, r == state->msrs.nmsrs ? -1 : state->msrs.entries[r].index);

        if (kvm_check_cap(KVM_CAP_XCRS)) {
                r = ioctl(vcpu->fd, KVM_SET_XCRS, &state->xcrs);
-               TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_XCRS, r: %i",
-                           r);
+               TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_XCRS, r: %i", r);
        }

        r = ioctl(vcpu->fd, KVM_SET_XSAVE, &state->xsave);
-       TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_XSAVE, r: %i",
-               r);
+       TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_XSAVE, r: %i", r);

        r = ioctl(vcpu->fd, KVM_SET_VCPU_EVENTS, &state->events);
-       TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_VCPU_EVENTS, r: %i",
-               r);
+       TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_VCPU_EVENTS, r: %i", r);

        r = ioctl(vcpu->fd, KVM_SET_MP_STATE, &state->mp_state);
-        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_MP_STATE, r: %i",
-                r);
+        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_MP_STATE, r: %i", r);

        r = ioctl(vcpu->fd, KVM_SET_DEBUGREGS, &state->debugregs);
-        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_DEBUGREGS, r: %i",
-                r);
+        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_DEBUGREGS, r: %i", r);

        r = ioctl(vcpu->fd, KVM_SET_REGS, &state->regs);
-       TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_REGS, r: %i",
-               r);
+       TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_REGS, r: %i", r);

        if (state->nested.size) {
                r = ioctl(vcpu->fd, KVM_SET_NESTED_STATE, &state->nested);
-               TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_NESTED_STATE, r: %i",
-                       r);
+               TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_NESTED_STATE, r: %i", r);
        }
 }