Hi,
Thanks for the patch and sorry for taking so long to review it.
On Sun, Sep 02, 2018 at 07:46:06PM +0800, Robert Hoo wrote:
> Add kvm_get_supported_feature_msrs() to get supported MSR feature index list.
> Add kvm_arch_get_supported_msr_feature() to get each MSR features value.
>
> Signed-off-by: Robert Hoo <robert...@linux.intel.com>
> ---
> include/sysemu/kvm.h | 2 ++
> target/i386/cpu.c | 7 ++---
> target/i386/kvm.c | 72
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 78 insertions(+), 3 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 0b64b8e..97d8d9d 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -463,6 +463,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int
> extension);
>
> uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> uint32_t index, int reg);
> +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
> +
>
> void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a252c26..0160e97 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3670,7 +3670,7 @@ static uint32_t
> x86_cpu_get_supported_feature_word(FeatureWord w,
> bool migratable_only)
> {
> FeatureWordInfo *wi = &feature_word_info[w];
> - uint32_t r;
> + uint32_t r = 0;
>
> if (kvm_enabled()) {
> switch (wi->type) {
> @@ -3679,8 +3679,9 @@ static uint32_t
> x86_cpu_get_supported_feature_word(FeatureWord w,
> wi->cpuid.ecx,
> wi->cpuid.reg);
> break;
> - default:
> - r = 0;
> + case MSR_FEATURE_WORD:
> + r = kvm_arch_get_supported_msr_feature(kvm_state,
> + wi->msr.index);
If you move this patch before patch 1/3, this hunk could be part
of patch 1/3.
> break;
> }
> } else if (hvf_enabled()) {
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 0b2a07d..bfd8088 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -107,6 +107,7 @@ static int has_pit_state2;
> static bool has_msr_mcg_ext_ctl;
>
> static struct kvm_cpuid2 *cpuid_cache;
> +static struct kvm_msr_list *kvm_feature_msrs;
>
> int kvm_has_pit_state2(void)
> {
> @@ -420,6 +421,33 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
> uint32_t function,
> return ret;
> }
>
> +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index)
> +{
> + struct {
> + struct kvm_msrs info;
> + struct kvm_msr_entry entries[1];
> + } msr_data;
> + uint32_t ret;
> +
> + if (kvm_feature_msrs == NULL) { /*ARCH doesn't support feature MSRs*/
Nit: normally comments have spaces after "/*" and before "*/".
Also: what do you mean by "ARCH"? Do you mean "host kernel"?
> + return 0;
> + }
> +
> + msr_data.info.nmsrs = 1;
> + msr_data.entries[0].index = index;
> +
> + ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data);
> +
> + if (ret != 1) {
If the MSR is not supported by the host kernel, it must not be a
fatal error. We should just return 0 on that case.
Probably the best way to ensure that is to check if the MSR is
listed on kvm_feature_msrs before calling KVM_GET_MSRS (and
return 0 if the MSR is not on the list).
> + fprintf(stderr, "KVM get MSR (index=0x%x) feature failed, %s\n",
> + index, strerror(-ret));
Please use error_report() instead of fprintf(stderr).
> + exit(1);
I'm unsure if exit(1) is the best option here, but at least this
is consistent with error handling kvm_arch_get_supported_cpuid().
> + }
> +
> + return msr_data.entries[0].data;
> +}
> +
> +
> typedef struct HWPoisonPage {
> ram_addr_t ram_addr;
> QLIST_ENTRY(HWPoisonPage) list;
> @@ -1239,6 +1267,45 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu)
> }
> }
>
> +static int kvm_get_supported_feature_msrs(KVMState *s)
> +{
> + int ret = 0;
> +
> + if (kvm_feature_msrs != NULL) {
> + return 0;
> + }
> +
> + if (!kvm_check_extension(s, KVM_CAP_GET_MSR_FEATURES)) {
> + return -1;
There's nothing wrong with not supporting
KVM_CAP_GET_MSR_FEATURES. Why not return 0?
> + }
> +
> + struct kvm_msr_list msr_list;
> +
> + msr_list.nmsrs = 0;
> + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list);
> + if (ret < 0 && ret != -E2BIG) {
You print an error to stderr if (ret < 0) below, but don't print
anything here. Seems inconsistent.
> + return ret;
> + }
> +
> + assert(msr_list.nmsrs > 0);
> + kvm_feature_msrs = (struct kvm_msr_list *) \
> + g_malloc0(sizeof(msr_list) +
> + msr_list.nmsrs * sizeof(msr_list.indices[0]));
> +
> + kvm_feature_msrs->nmsrs = msr_list.nmsrs;
> + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, kvm_feature_msrs);
kvm_arch_get_supported_msr_feature() is only checking if
kvm_feature_msrs is NULL, and nothing else.
What exactly is the point of calling
KVM_GET_MSR_FEATURE_INDEX_LIST and saving data at
kvm_feature_msrs, if no other code is ever looking at the
returned data?
> +
> + if (ret < 0) {
> + fprintf(stderr, "Fetch KVM feature MSRs failed: %s\n",
> + strerror(-ret));
Please use error_report() instead of fprintf(stderr).
> + g_free(kvm_feature_msrs);
> + kvm_feature_msrs = NULL;
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int kvm_get_supported_msrs(KVMState *s)
> {
> static int kvm_supported_msrs;
> @@ -1392,6 +1459,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> return ret;
> }
>
> + ret = kvm_get_supported_feature_msrs(s);
> + if (ret < 0) { /*if MSR based features aren't supported, ignore it.*/
> + warn_report("Get supported feature MSRs failed.");
We must not print a warning only because KVM_CAP_GET_MSR_FEATURES
isn't supported by the host kernel.
If KVM_GET_MSR_FEATURE_INDEX_LIST fails, on the other hand, we
probably should make it a fatal error and not a warning.
> + }
> +
> uname(&utsname);
> lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0;
>
> --
> 1.8.3.1
>
>
--
Eduardo
