| Message ID | 20211222214731.2912361-2-yang.zhong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMX KVM selftest | expand |
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); } }
Hi Sean, On Wed, Jan 12, 2022 at 09:05:02PM +0000, Sean Christopherson wrote: > 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 > Thanks, this is clearer. > > 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); > } > } Thanks for pointing out those issues. In fact, space issues are not only in this function, if we need handle those, we had better cleanup all those space issues. Since this series has been merged into KVM-Next, we can submit one patch to handle those space issue in the new 5.17-rcx release. thanks! Yang
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); + + 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); if (kvm_check_cap(KVM_CAP_XCRS)) { r = ioctl(vcpu->fd, KVM_SET_XCRS, &state->xcrs); @@ -1202,17 +1207,13 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_x86_state *s r); } - r = ioctl(vcpu->fd, KVM_SET_SREGS, &state->sregs); - 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); + 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_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", @@ -1223,8 +1224,8 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_x86_state *s 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);