On Wed, Dec 22, 2021, Yang Zhong wrote:
> From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Yang Zhong <yang.zhong@xxxxxxxxx>
> ---
> .../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);
}
}
