| Message ID | 20240528041926.3989-5-manali.shukla@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for the Idle HLT intercept feature | expand |
On Tue, May 28, 2024, Manali Shukla wrote: > From: Manali Shukla <Manali.Shukla@amd.com> > > The interface is used to read the data values of a specified vcpu stat > from the currenly available binary stats interface. > > Signed-off-by: Manali Shukla <Manali.Shukla@amd.com> > --- > .../kvm/include/kvm_arch_vcpu_states.h | 49 +++++++++++++++++++ > .../testing/selftests/kvm/include/kvm_util.h | 34 +++++++++++++ > tools/testing/selftests/kvm/lib/kvm_util.c | 32 ++++++++++++ > 3 files changed, 115 insertions(+) > create mode 100644 tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h > > diff --git a/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h b/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h > new file mode 100644 > index 000000000000..755ff7de53d9 > --- /dev/null > +++ b/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h > @@ -0,0 +1,49 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +/* > + * Arch-specific stats are added to the kvm_arch_vcpu_states.h. Sequence > + * of arch-specific vcpu_stat_type should be same as they are declared in > + * arch-specific kvm_vcpu_stat. > + */ > +#ifdef __x86_64__ This is backwards. If you want arch specific stats, put it them in an arch specific header. > +#define KVM_X86_VCPU_STATE(x) KVM_VCPU_STATE(x) > + > +KVM_X86_VCPU_STATE(PF_TAKEN) I'm pretty sure you want KVM_VCPU_STAT, KVM_X86_VCPU_STAT, kvm_arch_vcpu_states.h, etc. > +KVM_X86_VCPU_STATE(PF_FIXED) ... > +/* > + * Ensure that the sequence of the enum vcpu_stat_types matches the order of > + * kvm_vcpu_stats_desc[]. Otherwise, vcpu_get_stat() may return incorrect data > + * because __vcpu_get_stat() uses the enum type as an index to get the > + * descriptor for a given stat and then uses read_stat_data() to get the stats > + * from the descriptor. This isn't maintainable. Unless I'm missing something, the _order_ of KVM's stats isn't ABI, and blindly reading an entry and hoping its the right one is doomed to fail. I don't see any reason whatsoever to diverge from the core functionality of __vm_get_stat(). The only difference should be the origin of the stats file and header. I do see a lot of room for improvement, but that can and should be done for both VM and vCPU stats. E.g. provide an API (and a container/struct?) to get a direct pointer to stat so that selftests don't have to walk all descriptors when they're reading the same stat over and over. And to detect typos at compile time, {vcpu,vm}_get_stat() could either play macro games or use enums and array to detect usage of a stat that doesn't exist. E.g. static inline uint64_t vm_get_stat(struct kvm_vm *vm, int stat) { uint64_t data; __vm_get_stat(vm, kvm_vm_stats[stat], &data, 1); return data; } or #define vm_get_stat(vm, stat) \ ({ \ uin64_t __data; \ \ <concatenation trickery to trigger compiler error if the stat doesn't exit> __vm_get_stat(vm, #stat, &data, 1); \ data; \ }) I'd probably vote for macro games, e.g. so that it's all but impossible to pass a per-VM stat into vcpu_get_stat(), and vice versa.
Hi Sean, Thank you for reviewing my patches. Sorry for the delay in response. On 8/13/2024 10:07 PM, Sean Christopherson wrote: > On Tue, May 28, 2024, Manali Shukla wrote: >> From: Manali Shukla <Manali.Shukla@amd.com> >> >> The interface is used to read the data values of a specified vcpu stat >> from the currenly available binary stats interface. >> >> Signed-off-by: Manali Shukla <Manali.Shukla@amd.com> >> --- >> .../kvm/include/kvm_arch_vcpu_states.h | 49 +++++++++++++++++++ >> .../testing/selftests/kvm/include/kvm_util.h | 34 +++++++++++++ >> tools/testing/selftests/kvm/lib/kvm_util.c | 32 ++++++++++++ >> 3 files changed, 115 insertions(+) >> create mode 100644 tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h >> >> diff --git a/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h b/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h >> new file mode 100644 >> index 000000000000..755ff7de53d9 >> --- /dev/null >> +++ b/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h >> @@ -0,0 +1,49 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +/* >> + * Arch-specific stats are added to the kvm_arch_vcpu_states.h. Sequence >> + * of arch-specific vcpu_stat_type should be same as they are declared in >> + * arch-specific kvm_vcpu_stat. >> + */ >> +#ifdef __x86_64__ > > This is backwards. If you want arch specific stats, put it them in an arch specific > header. > >> +#define KVM_X86_VCPU_STATE(x) KVM_VCPU_STATE(x) >> + >> +KVM_X86_VCPU_STATE(PF_TAKEN) > > I'm pretty sure you want KVM_VCPU_STAT, KVM_X86_VCPU_STAT, kvm_arch_vcpu_states.h, > etc. > >> +KVM_X86_VCPU_STATE(PF_FIXED) > > ... > >> +/* >> + * Ensure that the sequence of the enum vcpu_stat_types matches the order of >> + * kvm_vcpu_stats_desc[]. Otherwise, vcpu_get_stat() may return incorrect data >> + * because __vcpu_get_stat() uses the enum type as an index to get the >> + * descriptor for a given stat and then uses read_stat_data() to get the stats >> + * from the descriptor. > > This isn't maintainable. Unless I'm missing something, the _order_ of KVM's stats > isn't ABI, and blindly reading an entry and hoping its the right one is doomed to > fail. > > I don't see any reason whatsoever to diverge from the core functionality of > __vm_get_stat(). The only difference should be the origin of the stats file and > header. > > I do see a lot of room for improvement, but that can and should be done for both > VM and vCPU stats. E.g. provide an API (and a container/struct?) to get a direct > pointer to stat so that selftests don't have to walk all descriptors when they're > reading the same stat over and over. > > And to detect typos at compile time, {vcpu,vm}_get_stat() could either play macro > games or use enums and array to detect usage of a stat that doesn't exist. E.g. > > static inline uint64_t vm_get_stat(struct kvm_vm *vm, int stat) > { > uint64_t data; > > __vm_get_stat(vm, kvm_vm_stats[stat], &data, 1); > return data; > } > > or > > #define vm_get_stat(vm, stat) \ > ({ \ > uin64_t __data; \ > \ > <concatenation trickery to trigger compiler error if the stat doesn't exit> > __vm_get_stat(vm, #stat, &data, 1); \ > data; \ > }) > > I'd probably vote for macro games, e.g. so that it's all but impossible to pass > a per-VM stat into vcpu_get_stat(), and vice versa. All the review comments from this patch are taken care in [1]. [1] https://lore.kernel.org/kvm/20241021062226.108657-1-manali.shukla@amd.com/T/#t - Manali
diff --git a/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h b/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h new file mode 100644 index 000000000000..755ff7de53d9 --- /dev/null +++ b/tools/testing/selftests/kvm/include/kvm_arch_vcpu_states.h @@ -0,0 +1,49 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* + * Arch-specific stats are added to the kvm_arch_vcpu_states.h. Sequence + * of arch-specific vcpu_stat_type should be same as they are declared in + * arch-specific kvm_vcpu_stat. + */ +#ifdef __x86_64__ +#define KVM_X86_VCPU_STATE(x) KVM_VCPU_STATE(x) + +KVM_X86_VCPU_STATE(PF_TAKEN) +KVM_X86_VCPU_STATE(PF_FIXED) +KVM_X86_VCPU_STATE(PF_EMULATE) +KVM_X86_VCPU_STATE(PF_SPURIOUS) +KVM_X86_VCPU_STATE(PF_FAST) +KVM_X86_VCPU_STATE(PF_MMIO_SPTE_CREATED) +KVM_X86_VCPU_STATE(PF_GUEST) +KVM_X86_VCPU_STATE(TLB_FLUSH) +KVM_X86_VCPU_STATE(INVLPG) +KVM_X86_VCPU_STATE(EXITS) +KVM_X86_VCPU_STATE(IO_EXITS) +KVM_X86_VCPU_STATE(MMIO_EXITS) +KVM_X86_VCPU_STATE(SIGNAL_EXITS) +KVM_X86_VCPU_STATE(IRQ_WINDOW_EXITS) +KVM_X86_VCPU_STATE(NMI_WINDOW_EXITS) +KVM_X86_VCPU_STATE(LD_FLUSH) +KVM_X86_VCPU_STATE(HALT_EXITS) +KVM_X86_VCPU_STATE(REQUEST_IRQ_EXITS) +KVM_X86_VCPU_STATE(IRQ_EXITS) +KVM_X86_VCPU_STATE(HOST_STATE_RELOAD) +KVM_X86_VCPU_STATE(FPU_RELOAD) +KVM_X86_VCPU_STATE(INSN_EMULATION) +KVM_X86_VCPU_STATE(INSN_EMULATION_FAIL) +KVM_X86_VCPU_STATE(HYPERCALLS) +KVM_X86_VCPU_STATE(IRQ_INJECTIONS) +KVM_X86_VCPU_STATE(NMI_INJECTIONS) +KVM_X86_VCPU_STATE(REQ_EVENT) +KVM_X86_VCPU_STATE(NESTED_RUN) +KVM_X86_VCPU_STATE(DIRECTED_YIELD_ATTEMPTED) +KVM_X86_VCPU_STATE(DIRECTED_YIELD_SUCCESSFUL) +KVM_X86_VCPU_STATE(PREEMPTION_REPORTED) +KVM_X86_VCPU_STATE(PREEMPTION_OTHER) +KVM_X86_VCPU_STATE(GUEST_MODE) +KVM_X86_VCPU_STATE(NOTIFY_WINDOW_EXITS) + +#undef KVM_X86_VCPU_STATE +#undef KVM_VCPU_STATE +#endif + diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index 63c2aaae51f3..fe66b1736060 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -518,6 +518,40 @@ static inline uint64_t vm_get_stat(struct kvm_vm *vm, const char *stat_name) return data; } +/* + * Ensure that the sequence of the enum vcpu_stat_types matches the order of + * kvm_vcpu_stats_desc[]. Otherwise, vcpu_get_stat() may return incorrect data + * because __vcpu_get_stat() uses the enum type as an index to get the + * descriptor for a given stat and then uses read_stat_data() to get the stats + * from the descriptor. + */ +enum vcpu_stat_types { + HALT_SUCCESSFUL_POLL, + HALT_ATTEMPTED_POLL, + HALT_POLL_INVALID, + HALT_WAKEUP, + HALT_POLL_SUCCESS_NS, + HALT_POLL_FAIL_NS, + HALT_WAIT_NS, + HALT_POLL_SUCCESS_HIST, + HALT_POLL_FAIL_HIST, + HALT_WAIT_HIST, + BLOCKING, +#define KVM_VCPU_STATE(x) x, +#include "kvm_arch_vcpu_states.h" +}; + +void __vcpu_get_stat(struct kvm_vcpu *vcpu, enum vcpu_stat_types type, uint64_t *data, + size_t max_elements); + +static inline uint64_t vcpu_get_stat(struct kvm_vcpu *vcpu, enum vcpu_stat_types type) +{ + uint64_t data; + + __vcpu_get_stat(vcpu, type, &data, 1); + return data; +} + void vm_create_irqchip(struct kvm_vm *vm); static inline int __vm_create_guest_memfd(struct kvm_vm *vm, uint64_t size, diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index ad00e4761886..d4b6a352c1ae 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -2264,6 +2264,38 @@ void read_stat_data(int stats_fd, struct kvm_stats_header *header, desc->name, size, ret); } +/* + * Read the data of the named vcpu stat + * + * Input Args: + * vcpu - the vcpu for which the stat should be read + * stat_name - the name of the stat to read + * max_elements - the maximum number of 8-byte values to read into data + * + * Output Args: + * data - the buffer into which stat data should be read + * + * Read the data values of a specified stat from the binary stats interface. + */ +void __vcpu_get_stat(struct kvm_vcpu *vcpu, enum vcpu_stat_types type, uint64_t *data, + size_t max_elements) +{ + int vcpu_stats_fd; + struct kvm_stats_header header; + struct kvm_stats_desc *desc, *t_desc; + size_t size_desc; + + vcpu_stats_fd = vcpu_get_stats_fd(vcpu); + read_stats_header(vcpu_stats_fd, &header); + + desc = read_stats_descriptors(vcpu_stats_fd, &header); + size_desc = get_stats_descriptor_size(&header); + + t_desc = (void *)desc + (type * size_desc); + read_stat_data(vcpu_stats_fd, &header, t_desc, + data, max_elements); +} + /* * Read the data of the named stat *