| Message ID | 20220915000448.1674802-4-vannapurve@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Execute hypercalls from guests according to cpu type | expand |
On Thu, Sep 15, 2022 at 12:04:43AM +0000, Vishal Annapurve wrote: > Add arch specific API kvm_selftest_post_vm_elf_load to possibly communicate > information to VM that is already known to selftest VMM logic. > > This API will be used in followup commit to convey cpu vendor type to the > guest vm. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > --- > tools/testing/selftests/kvm/include/kvm_util_base.h | 4 ++++ > tools/testing/selftests/kvm/lib/aarch64/processor.c | 4 ++++ > tools/testing/selftests/kvm/lib/elf.c | 2 ++ > tools/testing/selftests/kvm/lib/riscv/processor.c | 4 ++++ > tools/testing/selftests/kvm/lib/s390x/processor.c | 4 ++++ > tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 ++++ > 6 files changed, 22 insertions(+) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index 98edbbda9f97..73cfee3ebd76 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm) > */ > void kvm_selftest_arch_init(void); > > +/* > + * API to execute architecture specific setup after loading the vm elf. It's not a "vm elf" per-se, it's "loading the elf into the VM". How about: /* * API to execute arch-specific logic after loading the selftest ELF image * into the VM. */ > + */ > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm); > #endif /* SELFTEST_KVM_UTIL_BASE_H */ > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c > index 2281d6c5d02f..12627c560f66 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c > @@ -528,3 +528,7 @@ void kvm_selftest_arch_init(void) > { > guest_modes_append_default(); > } > + > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > +{ > +} > diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c > index 9f54c098d9d0..b8963a7146ce 100644 > --- a/tools/testing/selftests/kvm/lib/elf.c > +++ b/tools/testing/selftests/kvm/lib/elf.c > @@ -189,4 +189,6 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename) > phdr.p_filesz); > } > } > + > + kvm_arch_post_vm_elf_load(vm); > } Same suggestion here as the previous patch: Use __weak to define a default no-op implementation of kvm_arch_post_vm_elf_load(). > diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c > index 26660dd2ba78..4491c0d4be45 100644 > --- a/tools/testing/selftests/kvm/lib/riscv/processor.c > +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c > @@ -366,3 +366,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu) > void kvm_selftest_arch_init(void) > { > } > + > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > +{ > +} > diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c > index 8654ec74009a..332501b3693f 100644 > --- a/tools/testing/selftests/kvm/lib/s390x/processor.c > +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c > @@ -222,3 +222,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu) > void kvm_selftest_arch_init(void) > { > } > + > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > +{ > +} > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index 20bf125f9363..25ae972f5c71 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -1315,3 +1315,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) > void kvm_selftest_arch_init(void) > { > } > + > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > +{ > +} > -- > 2.37.2.789.g6183377224-goog >
On Wed, Sep 21, 2022 at 1:54 PM David Matlack <dmatlack@google.com> wrote: > > On Thu, Sep 15, 2022 at 12:04:43AM +0000, Vishal Annapurve wrote: > > Add arch specific API kvm_selftest_post_vm_elf_load to possibly communicate > > information to VM that is already known to selftest VMM logic. > > > > This API will be used in followup commit to convey cpu vendor type to the > > guest vm. > > > > Suggested-by: Sean Christopherson <seanjc@google.com> > > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > > --- > > tools/testing/selftests/kvm/include/kvm_util_base.h | 4 ++++ > > tools/testing/selftests/kvm/lib/aarch64/processor.c | 4 ++++ > > tools/testing/selftests/kvm/lib/elf.c | 2 ++ > > tools/testing/selftests/kvm/lib/riscv/processor.c | 4 ++++ > > tools/testing/selftests/kvm/lib/s390x/processor.c | 4 ++++ > > tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 ++++ > > 6 files changed, 22 insertions(+) > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > index 98edbbda9f97..73cfee3ebd76 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm) > > */ > > void kvm_selftest_arch_init(void); > > > > +/* > > + * API to execute architecture specific setup after loading the vm elf. > > It's not a "vm elf" per-se, it's "loading the elf into the VM". How > about: > > /* > * API to execute arch-specific logic after loading the selftest ELF image > * into the VM. > */ > Ack. Will update this in the next series. > > + */ > > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm); > > #endif /* SELFTEST_KVM_UTIL_BASE_H */ > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c > > index 2281d6c5d02f..12627c560f66 100644 > > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c > > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c > > @@ -528,3 +528,7 @@ void kvm_selftest_arch_init(void) > > { > > guest_modes_append_default(); > > } > > + > > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > > +{ > > +} > > diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c > > index 9f54c098d9d0..b8963a7146ce 100644 > > --- a/tools/testing/selftests/kvm/lib/elf.c > > +++ b/tools/testing/selftests/kvm/lib/elf.c > > @@ -189,4 +189,6 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename) > > phdr.p_filesz); > > } > > } > > + > > + kvm_arch_post_vm_elf_load(vm); > > } > > Same suggestion here as the previous patch: Use __weak to define a > default no-op implementation of kvm_arch_post_vm_elf_load(). > > > diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c > > index 26660dd2ba78..4491c0d4be45 100644 > > --- a/tools/testing/selftests/kvm/lib/riscv/processor.c > > +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c > > @@ -366,3 +366,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu) > > void kvm_selftest_arch_init(void) > > { > > } > > + > > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > > +{ > > +} > > diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c > > index 8654ec74009a..332501b3693f 100644 > > --- a/tools/testing/selftests/kvm/lib/s390x/processor.c > > +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c > > @@ -222,3 +222,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu) > > void kvm_selftest_arch_init(void) > > { > > } > > + > > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > > +{ > > +} > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > index 20bf125f9363..25ae972f5c71 100644 > > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > @@ -1315,3 +1315,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) > > void kvm_selftest_arch_init(void) > > { > > } > > + > > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) > > +{ > > +} > > -- > > 2.37.2.789.g6183377224-goog > >
On Mon, Sep 26, 2022, Vishal Annapurve wrote: > On Wed, Sep 21, 2022 at 1:54 PM David Matlack <dmatlack@google.com> wrote: > > > > On Thu, Sep 15, 2022 at 12:04:43AM +0000, Vishal Annapurve wrote: > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > > index 98edbbda9f97..73cfee3ebd76 100644 > > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > > @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm) > > > */ > > > void kvm_selftest_arch_init(void); > > > > > > +/* > > > + * API to execute architecture specific setup after loading the vm elf. > > > > It's not a "vm elf" per-se, it's "loading the elf into the VM". How > > about: > > > > /* > > * API to execute arch-specific logic after loading the selftest ELF image > > * into the VM. > > */ > > > > Ack. Will update this in the next series. Even better, call it from __vm_create() and name it something like kvm_arch_vm_post_create(). Like David said, while the hook has a dependency on being called after loading the ELF image, the action that arch code is expected to take doesn't have anything to do with loading the ELF image. And then instead of introducing an arch hook with no implementation, the patch that adds the hook can instead use it to replace the x86-64 #ifdef in __vm_create(). diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index dafe4471a6c7..593dfadb662e 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -298,9 +298,8 @@ struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint32_t nr_runnable_vcpus, kvm_vm_elf_load(vm, program_invocation_name); -#ifdef __x86_64__ - vm_create_irqchip(vm); -#endif + kvm_arch_vm_post_create(vm); + return vm; }
On Mon, Oct 3, 2022 at 8:42 AM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Sep 26, 2022, Vishal Annapurve wrote: > > On Wed, Sep 21, 2022 at 1:54 PM David Matlack <dmatlack@google.com> wrote: > > > > > > On Thu, Sep 15, 2022 at 12:04:43AM +0000, Vishal Annapurve wrote: > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > > > index 98edbbda9f97..73cfee3ebd76 100644 > > > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > > > @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm) > > > > */ > > > > void kvm_selftest_arch_init(void); > > > > > > > > +/* > > > > + * API to execute architecture specific setup after loading the vm elf. > > > > > > It's not a "vm elf" per-se, it's "loading the elf into the VM". How > > > about: > > > > > > /* > > > * API to execute arch-specific logic after loading the selftest ELF image > > > * into the VM. > > > */ > > > > > > > Ack. Will update this in the next series. > > Even better, call it from __vm_create() and name it something like > kvm_arch_vm_post_create(). Like David said, while the hook has a dependency on > being called after loading the ELF image, the action that arch code is expected > to take doesn't have anything to do with loading the ELF image. > > And then instead of introducing an arch hook with no implementation, the patch that > adds the hook can instead use it to replace the x86-64 #ifdef in __vm_create(). > Today upstream kernel selftests don't have scenarios where kvm_vm_elf_load can get called directly outside ___vm_create but there are selftests that are up for review [1], [2] that may call kvm_vm_elf_load directly. Above suggestion will not work in this scenario, is it suitable to assume that all the callers of kvm_vm_elf_load will eventually execute kvm_arch_vm_post_create? [1] https://lore.kernel.org/lkml/20220810152033.946942-12-pgonda@google.com/ [2] https://lore.kernel.org/lkml/20220819174659.2427983-1-vannapurve@google.com/T/#u > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index dafe4471a6c7..593dfadb662e 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -298,9 +298,8 @@ struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint32_t nr_runnable_vcpus, > > kvm_vm_elf_load(vm, program_invocation_name); > > -#ifdef __x86_64__ > - vm_create_irqchip(vm); > -#endif > + kvm_arch_vm_post_create(vm); > + > return vm; > } > >
On Mon, Oct 03, 2022, Vishal Annapurve wrote: > On Mon, Oct 3, 2022 at 8:42 AM Sean Christopherson <seanjc@google.com> wrote: > > Even better, call it from __vm_create() and name it something like > > kvm_arch_vm_post_create(). Like David said, while the hook has a dependency on > > being called after loading the ELF image, the action that arch code is expected > > to take doesn't have anything to do with loading the ELF image. > > > > And then instead of introducing an arch hook with no implementation, the patch that > > adds the hook can instead use it to replace the x86-64 #ifdef in __vm_create(). > > > > Today upstream kernel selftests don't have scenarios where > kvm_vm_elf_load can get called directly outside ___vm_create but there > are selftests that are up for review [1], [2] that may call > kvm_vm_elf_load directly. Above suggestion will not work in this > scenario, is it suitable to assume that all the callers of > kvm_vm_elf_load will eventually execute kvm_arch_vm_post_create? No, but that's irrelevant. And actually, in any reasonable hypothetical situation I can think of, it's actually undesirable to always call kvm_arch_vm_post_create() after kvm_vm_elf_load(). Hypothetically, if there were a use case where kvm_vm_elf_load() is called multiple times, then stuffing the "Intel vs. "AMD" flag should only be done for the binary that actually defines that flag. The flag is defined by the library's processor.c, and so the hook should be tied to the library's loading of its binary, i.e. to the creation of the VM. If a test were loading multiple binaries, and the test wanted to tweak things specific to a binary after loading said binary, then the test can and should do that without needed a library arch hook. If the arch hook was to take action specific to loading _any_ binary, than yes, a hook in kvm_vm_elf_load() would be in order, but this hook is about taking action when creating a VM, not to loading a binary. But this is all very, very hypothetical. I can't think of a scenario where loading multiple binaries would be less complex than solving whatever hypothetical problem makes it difficult to link everything into a single binary. And if a test manually loads a binary _and_ wants to actually run the guest after doing vm_create_barebones() or ____vm_create(), then the test is doing it wrong, as those low level APIs are provided _only_ for cases where a test doesn't need to run vCPUs.
On Wed, Oct 5, 2022 at 1:40 AM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Oct 03, 2022, Vishal Annapurve wrote: > > On Mon, Oct 3, 2022 at 8:42 AM Sean Christopherson <seanjc@google.com> wrote: > > > Even better, call it from __vm_create() and name it something like > > > kvm_arch_vm_post_create(). Like David said, while the hook has a dependency on > > > being called after loading the ELF image, the action that arch code is expected > > > to take doesn't have anything to do with loading the ELF image. > > > > > > And then instead of introducing an arch hook with no implementation, the patch that > > > adds the hook can instead use it to replace the x86-64 #ifdef in __vm_create(). > > > > > > > Today upstream kernel selftests don't have scenarios where > > kvm_vm_elf_load can get called directly outside ___vm_create but there > > are selftests that are up for review [1], [2] that may call > > kvm_vm_elf_load directly. Above suggestion will not work in this > > scenario, is it suitable to assume that all the callers of > > kvm_vm_elf_load will eventually execute kvm_arch_vm_post_create? > > No, but that's irrelevant. And actually, in any reasonable hypothetical situation > I can think of, it's actually undesirable to always call kvm_arch_vm_post_create() > after kvm_vm_elf_load(). > > Hypothetically, if there were a use case where kvm_vm_elf_load() is called multiple > times, then stuffing the "Intel vs. "AMD" flag should only be done for the binary > that actually defines that flag. The flag is defined by the library's processor.c, > and so the hook should be tied to the library's loading of its binary, i.e. to the > creation of the VM. > > If a test were loading multiple binaries, and the test wanted to tweak things > specific to a binary after loading said binary, then the test can and should do > that without needed a library arch hook. > > If the arch hook was to take action specific to loading _any_ binary, than yes, a > hook in kvm_vm_elf_load() would be in order, but this hook is about taking action > when creating a VM, not to loading a binary. > > But this is all very, very hypothetical. I can't think of a scenario where > loading multiple binaries would be less complex than solving whatever hypothetical > problem makes it difficult to link everything into a single binary. > > And if a test manually loads a binary _and_ wants to actually run the guest after > doing vm_create_barebones() or ____vm_create(), then the test is doing it wrong, > as those low level APIs are provided _only_ for cases where a test doesn't need > to run vCPUs. Ack, will update this patch as per the feedback here.
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 98edbbda9f97..73cfee3ebd76 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm) */ void kvm_selftest_arch_init(void); +/* + * API to execute architecture specific setup after loading the vm elf. + */ +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm); #endif /* SELFTEST_KVM_UTIL_BASE_H */ diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c index 2281d6c5d02f..12627c560f66 100644 --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c @@ -528,3 +528,7 @@ void kvm_selftest_arch_init(void) { guest_modes_append_default(); } + +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) +{ +} diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c index 9f54c098d9d0..b8963a7146ce 100644 --- a/tools/testing/selftests/kvm/lib/elf.c +++ b/tools/testing/selftests/kvm/lib/elf.c @@ -189,4 +189,6 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename) phdr.p_filesz); } } + + kvm_arch_post_vm_elf_load(vm); } diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c index 26660dd2ba78..4491c0d4be45 100644 --- a/tools/testing/selftests/kvm/lib/riscv/processor.c +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c @@ -366,3 +366,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu) void kvm_selftest_arch_init(void) { } + +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) +{ +} diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c index 8654ec74009a..332501b3693f 100644 --- a/tools/testing/selftests/kvm/lib/s390x/processor.c +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c @@ -222,3 +222,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu) void kvm_selftest_arch_init(void) { } + +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) +{ +} diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index 20bf125f9363..25ae972f5c71 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -1315,3 +1315,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) void kvm_selftest_arch_init(void) { } + +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm) +{ +}
Add arch specific API kvm_selftest_post_vm_elf_load to possibly communicate information to VM that is already known to selftest VMM logic. This API will be used in followup commit to convey cpu vendor type to the guest vm. Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Vishal Annapurve <vannapurve@google.com> --- tools/testing/selftests/kvm/include/kvm_util_base.h | 4 ++++ tools/testing/selftests/kvm/lib/aarch64/processor.c | 4 ++++ tools/testing/selftests/kvm/lib/elf.c | 2 ++ tools/testing/selftests/kvm/lib/riscv/processor.c | 4 ++++ tools/testing/selftests/kvm/lib/s390x/processor.c | 4 ++++ tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 ++++ 6 files changed, 22 insertions(+)