| Message ID | 20220915000448.1674802-5-vannapurve@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Execute hypercalls from guests according to cpu type | expand |
On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote: > Cache the vendor CPU type in a global variable so that multiple calls > to is_intel_cpu() do not need to re-execute CPUID. > > Add cpu vendor check in kvm_hypercall() so that it executes correct > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids > exit to KVM which anyway tries to patch the instruction according to > the cpu type. The commit shortlog makes no mention (nor even implies) that this commit adds AMD support to kvm_hypercall(). Please break this commit up into 2. One to precompute the result of is_{intel,amd}_cpu() and one to add AMD support to kvm_hypercall(). If you really want to keep this as one commit (I don't know what the benefit would be), please change the shortlog and commit message to focus on the kvm_hypercall() change, as that is the real goal of this commit. The precomputation is arguably and implementation detail. e.g. KVM: selftest: Add support for AMD to kvm_hypercall() Make it possible to use kvm_hypercall() on AMD by checking if running on an AMD CPU and, if so, using vmmcall instead of vmcall. In order to avoid executing CPUID in the guest on every call t kvm_hypercall() (which would be slow), pre-compute the result of is_{intel,amd}_cpu() as part of kvm_selftest_arch_init() and sync it into the guest after loading the ELF image. But again, it'd be cleaner just to split it up. Caching the result of is_{intel,amd}_cpu() is useful in its own right, independent of the kvm_hypercall() change. > > As part of this change, sync the global variable is_cpu_amd into the > guest so the guest can determine which hypercall instruction to use > without needing to re-execute CPUID for every hypercall. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > --- > .../testing/selftests/kvm/lib/x86_64/processor.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index 25ae972f5c71..c0ae938772f6 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -19,6 +19,7 @@ > #define MAX_NR_CPUID_ENTRIES 100 > > vm_vaddr_t exception_handlers; > +static bool is_cpu_amd; > > static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent) > { > @@ -1019,7 +1020,7 @@ static bool cpu_vendor_string_is(const char *vendor) > > bool is_intel_cpu(void) > { > - return cpu_vendor_string_is("GenuineIntel"); > + return (is_cpu_amd == false); > } > > /* > @@ -1027,7 +1028,7 @@ bool is_intel_cpu(void) > */ > bool is_amd_cpu(void) > { > - return cpu_vendor_string_is("AuthenticAMD"); > + return (is_cpu_amd == true); > } > > void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits) > @@ -1182,9 +1183,15 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2, > { > uint64_t r; > > - asm volatile("vmcall" > + if (is_amd_cpu()) > + asm volatile("vmmcall" > : "=a"(r) > : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3)); > + else > + asm volatile("vmcall" > + : "=a"(r) > + : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3)); > + > return r; > } > > @@ -1314,8 +1321,10 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) > > void kvm_selftest_arch_init(void) > { > + is_cpu_amd = cpu_vendor_string_is("AuthenticAMD"); > } > > void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > { > + sync_global_to_guest(vm, is_cpu_amd); > } > -- > 2.37.2.789.g6183377224-goog >
On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote: > Cache the vendor CPU type in a global variable so that multiple calls > to is_intel_cpu() do not need to re-execute CPUID. > > Add cpu vendor check in kvm_hypercall() so that it executes correct > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids > exit to KVM which anyway tries to patch the instruction according to > the cpu type. Out of curiousity, why do we want to avoid this exit?
On Wed, Sep 21, 2022 at 2:19 PM David Matlack <dmatlack@google.com> wrote: > > On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote: > > Cache the vendor CPU type in a global variable so that multiple calls > > to is_intel_cpu() do not need to re-execute CPUID. > > > > Add cpu vendor check in kvm_hypercall() so that it executes correct > > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids > > exit to KVM which anyway tries to patch the instruction according to > > the cpu type. > > The commit shortlog makes no mention (nor even implies) that this commit > adds AMD support to kvm_hypercall(). Please break this commit up into 2. > One to precompute the result of is_{intel,amd}_cpu() and one to add AMD > support to kvm_hypercall(). > > If you really want to keep this as one commit (I don't know what the > benefit would be), please change the shortlog and commit message to > focus on the kvm_hypercall() change, as that is the real goal of this > commit. The precomputation is arguably and implementation detail. e.g. > is_amd_cpu is used by guest code within fix_hypercall_test.c, just caching the result will break the guest code execution. I have clubbed these two changes together in order to ensure that is_amd_cpu works fine for both host userspace and guest vm logic. > KVM: selftest: Add support for AMD to kvm_hypercall() > > Make it possible to use kvm_hypercall() on AMD by checking if running > on an AMD CPU and, if so, using vmmcall instead of vmcall. In order to > avoid executing CPUID in the guest on every call t kvm_hypercall() > (which would be slow), pre-compute the result of is_{intel,amd}_cpu() > as part of kvm_selftest_arch_init() and sync it into the guest > after loading the ELF image. > > But again, it'd be cleaner just to split it up. Caching the result of > is_{intel,amd}_cpu() is useful in its own right, independent of the > kvm_hypercall() change. > > > > > ... > > > > @@ -1314,8 +1321,10 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) > > > > void kvm_selftest_arch_init(void) > > { > > + is_cpu_amd = cpu_vendor_string_is("AuthenticAMD"); > > } > > > > void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > > { > > + sync_global_to_guest(vm, is_cpu_amd); > > } > > -- > > 2.37.2.789.g6183377224-goog > >
On Mon, Sep 26, 2022 at 4:27 PM Vishal Annapurve <vannapurve@google.com> wrote: > > On Wed, Sep 21, 2022 at 2:19 PM David Matlack <dmatlack@google.com> wrote: > > > > On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote: > > > Cache the vendor CPU type in a global variable so that multiple calls > > > to is_intel_cpu() do not need to re-execute CPUID. > > > > > > Add cpu vendor check in kvm_hypercall() so that it executes correct > > > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids > > > exit to KVM which anyway tries to patch the instruction according to > > > the cpu type. > > > > The commit shortlog makes no mention (nor even implies) that this commit > > adds AMD support to kvm_hypercall(). Please break this commit up into 2. > > One to precompute the result of is_{intel,amd}_cpu() and one to add AMD > > support to kvm_hypercall(). > > > > If you really want to keep this as one commit (I don't know what the > > benefit would be), please change the shortlog and commit message to > > focus on the kvm_hypercall() change, as that is the real goal of this > > commit. The precomputation is arguably and implementation detail. e.g. > > > > is_amd_cpu is used by guest code within fix_hypercall_test.c, just > caching the result will break the guest code execution. I have clubbed > these two changes together in order to ensure that is_amd_cpu works > fine for both host userspace and guest vm logic. Ah, so the sync_global_to_guest() part needs to go in the patch that adds caching to is_amd_cpu(). But the point still stands that adding AMD support to kvm_hypercall() is a logically independent change. > > > KVM: selftest: Add support for AMD to kvm_hypercall() > > > > Make it possible to use kvm_hypercall() on AMD by checking if running > > on an AMD CPU and, if so, using vmmcall instead of vmcall. In order to > > avoid executing CPUID in the guest on every call t kvm_hypercall() > > (which would be slow), pre-compute the result of is_{intel,amd}_cpu() > > as part of kvm_selftest_arch_init() and sync it into the guest > > after loading the ELF image. > > > > But again, it'd be cleaner just to split it up. Caching the result of > > is_{intel,amd}_cpu() is useful in its own right, independent of the > > kvm_hypercall() change. > > > > > > > > ... > > > > > > @@ -1314,8 +1321,10 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) > > > > > > void kvm_selftest_arch_init(void) > > > { > > > + is_cpu_amd = cpu_vendor_string_is("AuthenticAMD"); > > > } > > > > > > void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > > > { > > > + sync_global_to_guest(vm, is_cpu_amd); > > > } > > > -- > > > 2.37.2.789.g6183377224-goog > > >
On Mon, Sep 26, 2022 at 4:34 PM David Matlack <dmatlack@google.com> wrote: > > On Mon, Sep 26, 2022 at 4:27 PM Vishal Annapurve <vannapurve@google.com> wrote: > > > > On Wed, Sep 21, 2022 at 2:19 PM David Matlack <dmatlack@google.com> wrote: > > > ... > > > > is_amd_cpu is used by guest code within fix_hypercall_test.c, just > > caching the result will break the guest code execution. I have clubbed > > these two changes together in order to ensure that is_amd_cpu works > > fine for both host userspace and guest vm logic. > > Ah, so the sync_global_to_guest() part needs to go in the patch that > adds caching to is_amd_cpu(). > > But the point still stands that adding AMD support to kvm_hypercall() > is a logically independent change. > I see what you mean. Will split this change into two in the next series. > > ... > > > > -- > > > > 2.37.2.789.g6183377224-goog > > > >
On Wed, Sep 21, 2022 at 2:39 PM David Matlack <dmatlack@google.com> wrote: > > On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote: > > Cache the vendor CPU type in a global variable so that multiple calls > > to is_intel_cpu() do not need to re-execute CPUID. > > > > Add cpu vendor check in kvm_hypercall() so that it executes correct > > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids > > exit to KVM which anyway tries to patch the instruction according to > > the cpu type. > > Out of curiousity, why do we want to avoid this exit? Referring to the patch set posted for UPM selftests with non-confidential VMs [1], vmcall patching will not work for selftests executed with UPM feature enabled since guest memory can not be modified by KVM. So I tried to add a kvm_hypercall implementation that will execute the hypercall according to the cpu type. Hypercall updates in this series are done to ensure that such a change is done for all callers to allow consistency and avoid relying on KVM behavior to patch the vmmcall/vmcall instruction. [1] https://lore.kernel.org/lkml/20220819174659.2427983-5-vannapurve@google.com/
On Mon, Sep 26, 2022 at 4:48 PM Vishal Annapurve <vannapurve@google.com> wrote: > > On Wed, Sep 21, 2022 at 2:39 PM David Matlack <dmatlack@google.com> wrote: > > > > On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote: > > > Cache the vendor CPU type in a global variable so that multiple calls > > > to is_intel_cpu() do not need to re-execute CPUID. > > > > > > Add cpu vendor check in kvm_hypercall() so that it executes correct > > > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids > > > exit to KVM which anyway tries to patch the instruction according to > > > the cpu type. > > > > Out of curiousity, why do we want to avoid this exit? > > Referring to the patch set posted for UPM selftests with > non-confidential VMs [1], vmcall patching will not work for selftests > executed with UPM feature enabled since guest memory can not be > modified by KVM. So I tried to add a kvm_hypercall implementation that > will execute the hypercall according to the cpu type. > > Hypercall updates in this series are done to ensure that such a change > is done for all callers to allow consistency and avoid relying on KVM > behavior to patch the vmmcall/vmcall instruction. > > [1] https://lore.kernel.org/lkml/20220819174659.2427983-5-vannapurve@google.com/ Thanks! That makes a ton of sense. Please include that in the cover letter and any of the commit messages that avoid hypercall patching. That will help reviewers understand the context of why the changes are being made, and help anyone reviewing the git history in the future understand the same.
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index 25ae972f5c71..c0ae938772f6 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -19,6 +19,7 @@ #define MAX_NR_CPUID_ENTRIES 100 vm_vaddr_t exception_handlers; +static bool is_cpu_amd; static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent) { @@ -1019,7 +1020,7 @@ static bool cpu_vendor_string_is(const char *vendor) bool is_intel_cpu(void) { - return cpu_vendor_string_is("GenuineIntel"); + return (is_cpu_amd == false); } /* @@ -1027,7 +1028,7 @@ bool is_intel_cpu(void) */ bool is_amd_cpu(void) { - return cpu_vendor_string_is("AuthenticAMD"); + return (is_cpu_amd == true); } void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits) @@ -1182,9 +1183,15 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2, { uint64_t r; - asm volatile("vmcall" + if (is_amd_cpu()) + asm volatile("vmmcall" : "=a"(r) : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3)); + else + asm volatile("vmcall" + : "=a"(r) + : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3)); + return r; } @@ -1314,8 +1321,10 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) void kvm_selftest_arch_init(void) { + is_cpu_amd = cpu_vendor_string_is("AuthenticAMD"); } void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) { + sync_global_to_guest(vm, is_cpu_amd); }
Cache the vendor CPU type in a global variable so that multiple calls to is_intel_cpu() do not need to re-execute CPUID. Add cpu vendor check in kvm_hypercall() so that it executes correct vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids exit to KVM which anyway tries to patch the instruction according to the cpu type. As part of this change, sync the global variable is_cpu_amd into the guest so the guest can determine which hypercall instruction to use without needing to re-execute CPUID for every hypercall. Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Vishal Annapurve <vannapurve@google.com> --- .../testing/selftests/kvm/lib/x86_64/processor.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)