| Message ID | 20211006203617.13045-1-michael.roth@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Wed, Oct 6, 2021 at 1:39 PM Michael Roth <michael.roth@amd.com> wrote: > > Recent kernels have checks to ensure the GPA values in special-purpose > registers like CR3 are within the maximum physical address range and > don't overlap with anything in the upper/reserved range. In the case of > SEV kselftest guests booting directly into 64-bit mode, CR3 needs to be > initialized to the GPA of the page table root, with the encryption bit > set. The kernel accounts for this encryption bit by removing it from > reserved bit range when the guest advertises the bit position via > KVM_SET_CPUID*, but kselftests currently call KVM_SET_SREGS as part of > vm_vcpu_add_default(), *prior* to vCPU creation, so there's no > opportunity to call KVM_SET_CPUID* in advance. As a result, > KVM_SET_SREGS will return an error in these cases. > > Address this by moving vcpu_set_cpuid() (which calls KVM_SET_CPUID*) > ahead of vcpu_setup() (which calls KVM_SET_SREGS). > > While there, address a typo in the assertion that triggers when > KVM_SET_SREGS fails. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Michael Roth <michael.roth@amd.com> > --- > tools/testing/selftests/kvm/lib/kvm_util.c | 2 +- > tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +--- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index ef88fdc7e46b..646cffd86d09 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -1906,7 +1906,7 @@ void vcpu_sregs_get(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs) > void vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs) > { > int ret = _vcpu_sregs_set(vm, vcpuid, sregs); > - TEST_ASSERT(ret == 0, "KVM_RUN IOCTL failed, " > + TEST_ASSERT(ret == 0, "KVM_SET_SREGS IOCTL failed, " > "rc: %i errno: %i", ret, errno); > } > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index 0bbd88fe1127..1ab4c20f5d12 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -660,6 +660,7 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) > > /* Create VCPU */ > vm_vcpu_add(vm, vcpuid); > + vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid()); > vcpu_setup(vm, vcpuid); > > /* Setup guest general purpose registers */ > @@ -672,9 +673,6 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) > /* Setup the MP state */ > mp_state.mp_state = 0; > vcpu_set_mp_state(vm, vcpuid, &mp_state); > - > - /* Setup supported CPUIDs */ > - vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid()); > } > > /* > -- > 2.25.1 > Good catch. Reviewed-by: Nathan Tempelman <natet@google.com>
On 10/6/21 1:36 PM, Michael Roth wrote: > KVM_SET_CPUID*, but kselftests currently call KVM_SET_SREGS as part of > vm_vcpu_add_default(),*prior* to vCPU creation, so there's no > opportunity to call KVM_SET_CPUID* in advance. As a result, In the current code, I see that KVM_SET_SREGS is called by vcpu_setup() which is called after vm_vcpu_add() that calls KVM_CREATE_VCPU. Since you mentioned "prior", I wanted to check if the wording was wrong or if I missed something.
On Tue, Oct 12, 2021 at 06:45:09PM -0700, Krish Sadhukhan wrote: > > On 10/6/21 1:36 PM, Michael Roth wrote: > > KVM_SET_CPUID*, but kselftests currently call KVM_SET_SREGS as part of > > vm_vcpu_add_default(),*prior* to vCPU creation, so there's no > > opportunity to call KVM_SET_CPUID* in advance. As a result, > In the current code, I see that KVM_SET_SREGS is called by vcpu_setup() > which is called after vm_vcpu_add() that calls KVM_CREATE_VCPU. Since you > mentioned "prior", I wanted to check if the wording was wrong or if I missed > something. Ah, yes, just poorly worded. What I meant to convey is that from the perspective the test program the vm_vcpu_add* call that creates the vcpu does the KVM_SET_SREGS, so there's no way to call KVM_SET_CPUID in advance other than to have vm_vcpu_add* do it as part of creating the vcpu. I get the wording fixed up on that.
On 06/10/21 22:36, Michael Roth wrote: > Recent kernels have checks to ensure the GPA values in special-purpose > registers like CR3 are within the maximum physical address range and > don't overlap with anything in the upper/reserved range. In the case of > SEV kselftest guests booting directly into 64-bit mode, CR3 needs to be > initialized to the GPA of the page table root, with the encryption bit > set. The kernel accounts for this encryption bit by removing it from > reserved bit range when the guest advertises the bit position via > KVM_SET_CPUID*, but kselftests currently call KVM_SET_SREGS as part of > vm_vcpu_add_default(), *prior* to vCPU creation, so there's no > opportunity to call KVM_SET_CPUID* in advance. As a result, > KVM_SET_SREGS will return an error in these cases. > > Address this by moving vcpu_set_cpuid() (which calls KVM_SET_CPUID*) > ahead of vcpu_setup() (which calls KVM_SET_SREGS). > > While there, address a typo in the assertion that triggers when > KVM_SET_SREGS fails. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Michael Roth <michael.roth@amd.com> > --- > tools/testing/selftests/kvm/lib/kvm_util.c | 2 +- > tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +--- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index ef88fdc7e46b..646cffd86d09 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -1906,7 +1906,7 @@ void vcpu_sregs_get(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs) > void vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs) > { > int ret = _vcpu_sregs_set(vm, vcpuid, sregs); > - TEST_ASSERT(ret == 0, "KVM_RUN IOCTL failed, " > + TEST_ASSERT(ret == 0, "KVM_SET_SREGS IOCTL failed, " > "rc: %i errno: %i", ret, errno); > } > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index 0bbd88fe1127..1ab4c20f5d12 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -660,6 +660,7 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) > > /* Create VCPU */ > vm_vcpu_add(vm, vcpuid); > + vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid()); > vcpu_setup(vm, vcpuid); > > /* Setup guest general purpose registers */ > @@ -672,9 +673,6 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) > /* Setup the MP state */ > mp_state.mp_state = 0; > vcpu_set_mp_state(vm, vcpuid, &mp_state); > - > - /* Setup supported CPUIDs */ > - vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid()); > } > > /* > Queued, thanks. Paolo
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index ef88fdc7e46b..646cffd86d09 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1906,7 +1906,7 @@ void vcpu_sregs_get(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs) void vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs) { int ret = _vcpu_sregs_set(vm, vcpuid, sregs); - TEST_ASSERT(ret == 0, "KVM_RUN IOCTL failed, " + TEST_ASSERT(ret == 0, "KVM_SET_SREGS IOCTL failed, " "rc: %i errno: %i", ret, errno); } diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index 0bbd88fe1127..1ab4c20f5d12 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -660,6 +660,7 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) /* Create VCPU */ vm_vcpu_add(vm, vcpuid); + vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid()); vcpu_setup(vm, vcpuid); /* Setup guest general purpose registers */ @@ -672,9 +673,6 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) /* Setup the MP state */ mp_state.mp_state = 0; vcpu_set_mp_state(vm, vcpuid, &mp_state); - - /* Setup supported CPUIDs */ - vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid()); } /*
Recent kernels have checks to ensure the GPA values in special-purpose registers like CR3 are within the maximum physical address range and don't overlap with anything in the upper/reserved range. In the case of SEV kselftest guests booting directly into 64-bit mode, CR3 needs to be initialized to the GPA of the page table root, with the encryption bit set. The kernel accounts for this encryption bit by removing it from reserved bit range when the guest advertises the bit position via KVM_SET_CPUID*, but kselftests currently call KVM_SET_SREGS as part of vm_vcpu_add_default(), *prior* to vCPU creation, so there's no opportunity to call KVM_SET_CPUID* in advance. As a result, KVM_SET_SREGS will return an error in these cases. Address this by moving vcpu_set_cpuid() (which calls KVM_SET_CPUID*) ahead of vcpu_setup() (which calls KVM_SET_SREGS). While there, address a typo in the assertion that triggers when KVM_SET_SREGS fails. Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- tools/testing/selftests/kvm/lib/kvm_util.c | 2 +- tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-)