[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_
From: |
Robert Hoo |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl |
Date: |
Thu, 20 Sep 2018 15:45:42 +0800 |
On Thu, 2018-09-20 at 00:07 -0300, Eduardo Habkost wrote:
> Hi,
>
> Thanks for the patch and sorry for taking so long to review it.
Never mind. I understand you're really busy. :-)
>
> 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 <address@hidden>
> > ---
> > 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.
>
I'm afraid that if I moved this hunk, because of the dependency, I
would have to move the definition of
kvm_arch_get_supported_msr_feature() to patch 1/3, then in turn, it
used kvm_feature_msrs, I've to put its definition and initialization
function kvm_get_supported_feature_msrs() to patch 1/3 as well. Then
actually, this makes patch 1/3 and 2/3 merged into 1. Would you like me
to do so?
> > 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"?
>
Going to say "Host".
>
> > + 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).
Yes. Will do in this way in v5.
>
>
> > + 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?
>
OK
>
> > + }
> > +
> > + 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.
Going to be consistent.
>
> > + 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?
>
Now above change make it useful for checking requested feature MSR's
validity.
>
> > +
> > + if (ret < 0) {
> > + fprintf(stderr, "Fetch KVM feature MSRs failed: %s\n",
> > + strerror(-ret));
>
> Please use error_report() instead of fprintf(stderr).
OK
>
> > + 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.
>
OK, remove the check.
> > + }
> > +
> > uname(&utsname);
> > lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0;
> >
> > --
> > 1.8.3.1
> >
> >
>
>
[Qemu-devel] [PATCH v4 3/3] x86: define a new MSR based feature word -- FEATURE_WORDS_ARCH_CAPABILITIES, Robert Hoo, 2018/09/02
[Qemu-devel] [PATCH v4 1/3] x86: Data structure changes to support MSR based features, Robert Hoo, 2018/09/02