| Message ID | 1575449430-23366-1-git-send-email-catherine.hecx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: relax assert when old host kernels don't include msrs | expand |
On 04/12/19 09:50, Catherine Ho wrote: > Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally > add vmx msr entry although older host kernels don't include them. > > But old host kernel + newest qemu will cause a qemu crash as follows: > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0 > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret == > cpu->kvm_msr_buf->nmsrs' failed. > > This fixes it by relaxing the condition. This is intentional. The VMX MSR entries should not have been added. What combination of host kernel/QEMU are you using, and what QEMU command line? Paolo
Hi Paolo [sorry to resend it, seems to reply it incorrectly] On Wed, 4 Dec 2019 at 19:23, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 04/12/19 09:50, Catherine Ho wrote: > > Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally > > add vmx msr entry although older host kernels don't include them. > > > > But old host kernel + newest qemu will cause a qemu crash as follows: > > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0 > > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret == > > cpu->kvm_msr_buf->nmsrs' failed. > > > > This fixes it by relaxing the condition. > > This is intentional. The VMX MSR entries should not have been added. > What combination of host kernel/QEMU are you using, and what QEMU > command line? > > > Host kernel: 4.15.0 (ubuntu 18.04) Qemu: https://gitlab.com/virtio-fs/qemu/tree/virtio-fs-dev cmdline: qemu-system-x86_64 -M pc -cpu host --enable-kvm -smp 8 \ -m 4G,maxmem=4G But before 20a78b02d315, the older kernel + latest qemu can boot guest successfully. Best Regards, Catherine
On 04/12/19 14:33, Catherine Ho wrote: > Hi Paolo > [sorry to resend it, seems to reply it incorrectly] > > On Wed, 4 Dec 2019 at 19:23, Paolo Bonzini <pbonzini@redhat.com > <mailto:pbonzini@redhat.com>> wrote: > > On 04/12/19 09:50, Catherine Ho wrote: > > Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally > > add vmx msr entry although older host kernels don't include them. > > > > But old host kernel + newest qemu will cause a qemu crash as follows: > > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0 > > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret == > > cpu->kvm_msr_buf->nmsrs' failed. > > > > This fixes it by relaxing the condition. > > This is intentional. The VMX MSR entries should not have been added. > What combination of host kernel/QEMU are you using, and what QEMU > command line? > > > Host kernel: 4.15.0 (ubuntu 18.04) > Qemu: https://gitlab.com/virtio-fs/qemu/tree/virtio-fs-dev > cmdline: qemu-system-x86_64 -M pc -cpu host --enable-kvm -smp 8 \ > -m 4G,maxmem=4G > > But before 20a78b02d315, the older kernel + latest qemu can boot guest > successfully. Ok, so the problem is that some MSR didn't exist in that version. Which one it is? Can you make it conditional, similar to MSR_IA32_VMX_VMFUNC? Thanks, Paolo
Hi Paolo On Wed, 4 Dec 2019 at 21:53, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 04/12/19 14:33, Catherine Ho wrote: > > Hi Paolo > > [sorry to resend it, seems to reply it incorrectly] > > > > On Wed, 4 Dec 2019 at 19:23, Paolo Bonzini <pbonzini@redhat.com > > <mailto:pbonzini@redhat.com>> wrote: > > > > On 04/12/19 09:50, Catherine Ho wrote: > > > Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally > > > add vmx msr entry although older host kernels don't include them. > > > > > > But old host kernel + newest qemu will cause a qemu crash as follows: > > > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0 > > > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret == > > > cpu->kvm_msr_buf->nmsrs' failed. > > > > > > This fixes it by relaxing the condition. > > > > This is intentional. The VMX MSR entries should not have been added. > > What combination of host kernel/QEMU are you using, and what QEMU > > command line? > > > > > > Host kernel: 4.15.0 (ubuntu 18.04) > > Qemu: https://gitlab.com/virtio-fs/qemu/tree/virtio-fs-dev > > cmdline: qemu-system-x86_64 -M pc -cpu host --enable-kvm -smp 8 \ > > -m 4G,maxmem=4G > > > > But before 20a78b02d315, the older kernel + latest qemu can boot guest > > successfully. > > Ok, so the problem is that some MSR didn't exist in that version. Which I thought in my platform, the only MSR didn't exist is MSR_IA32_VMX_BASIC (0x480). If I remove this kvm_msr_entry_add(), everything is ok, the guest can be boot up successfully. > one it is? Can you make it conditional, similar to MSR_IA32_VMX_VMFUNC? Ok, I will. Thanks for the suggestion Best regards Catherine
On 04/12/19 16:07, Catherine Ho wrote: >> Ok, so the problem is that some MSR didn't exist in that version. Which > I thought in my platform, the only MSR didn't exist is MSR_IA32_VMX_BASIC > (0x480). If I remove this kvm_msr_entry_add(), everything is ok, the guest can > be boot up successfully. > MSR_IA32_VMX_BASIC was added in kvm-4.10. Maybe the issue is the _value_ that is being written to the VM is not valid? Can you check what's happening in vmx_restore_vmx_basic? Paolo
On Wed, Dec 04, 2019 at 04:34:45PM +0100, Paolo Bonzini wrote: > On 04/12/19 16:07, Catherine Ho wrote: > >> Ok, so the problem is that some MSR didn't exist in that version. Which > > I thought in my platform, the only MSR didn't exist is MSR_IA32_VMX_BASIC > > (0x480). If I remove this kvm_msr_entry_add(), everything is ok, the guest can > > be boot up successfully. > > > > MSR_IA32_VMX_BASIC was added in kvm-4.10. Maybe the issue is the > _value_ that is being written to the VM is not valid? Can you check > what's happening in vmx_restore_vmx_basic? I believe env->features[FEAT_VMX_BASIC] will be initialized to 0 if the host kernel doesn't have KVM_CAP_GET_MSR_FEATURES.
On 04/12/19 16:47, Eduardo Habkost wrote: > On Wed, Dec 04, 2019 at 04:34:45PM +0100, Paolo Bonzini wrote: >> On 04/12/19 16:07, Catherine Ho wrote: >>>> Ok, so the problem is that some MSR didn't exist in that version. Which >>> I thought in my platform, the only MSR didn't exist is MSR_IA32_VMX_BASIC >>> (0x480). If I remove this kvm_msr_entry_add(), everything is ok, the guest can >>> be boot up successfully. >>> >> >> MSR_IA32_VMX_BASIC was added in kvm-4.10. Maybe the issue is the >> _value_ that is being written to the VM is not valid? Can you check >> what's happening in vmx_restore_vmx_basic? > > I believe env->features[FEAT_VMX_BASIC] will be initialized to 0 > if the host kernel doesn't have KVM_CAP_GET_MSR_FEATURES. But the host must have MSR features if the MSRs are added: if (kvm_feature_msrs && cpu_has_vmx(env)) { kvm_msr_entry_add_vmx(cpu, env->features); } Looks like feature MSRs were backported to 4.14, but 1389309c811b0c954bf3b591b761d79b1700283d and the previous commit weren't. Paolo
Hi Paolo and Eduardo I digged into the put msr assertion bug a little more, and seems I found the root cause. Please have a review. Best regards. Catherine On Fri, 6 Dec 2019 at 18:25, Catherine Ho <catherine.hecx@gmail.com> wrote: > > Commit 1389309c811b ("KVM: nVMX: expose VMX capabilities for nested > hypervisors to userspace") expands the msr_based_features with > MSR_IA32_VMX_BASIC and others. Then together with an old kernel before > 1389309c811b, the qemu call KVM_GET_MSR_FEATURE_INDEX_LIST and got the > smaller kvm_feature_msrs. Then in kvm_arch_get_supported_msr_feature(), > searching VMX_BASIC will be failed and return 0. At last kvm_vmx_basic > will be assigned to 0. > > Without this patch, it will cause a qemu crash (host kernel 4.15 > ubuntu 18.04+qemu 4.1): > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0 > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret == > cpu->kvm_msr_buf->nmsrs' failed. > > This fixes it by skipping kvm_msr_entry_add when kvm_vmx_basic is 0 > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > --- > target/i386/kvm.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index a8c44bf..8cf84a2 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -2632,8 +2632,13 @@ static void kvm_msr_entry_add_vmx(X86CPU *cpu, FeatureWordArray f) > f[FEAT_VMX_SECONDARY_CTLS])); > kvm_msr_entry_add(cpu, MSR_IA32_VMX_EPT_VPID_CAP, > f[FEAT_VMX_EPT_VPID_CAPS] | fixed_vmx_ept_vpid); > - kvm_msr_entry_add(cpu, MSR_IA32_VMX_BASIC, > + > + if (kvm_vmx_basic) { > + /* Only add the entry when host supports it */ > + kvm_msr_entry_add(cpu, MSR_IA32_VMX_BASIC, > f[FEAT_VMX_BASIC] | fixed_vmx_basic); > + } > + > kvm_msr_entry_add(cpu, MSR_IA32_VMX_MISC, > f[FEAT_VMX_MISC] | fixed_vmx_misc); > if (has_msr_vmx_vmfunc) { > -- > 1.7.1 >
On 06/12/19 11:23, Catherine Ho wrote: > Commit 1389309c811b ("KVM: nVMX: expose VMX capabilities for nested > hypervisors to userspace") expands the msr_based_features with > MSR_IA32_VMX_BASIC and others. Then together with an old kernel before > 1389309c811b, the qemu call KVM_GET_MSR_FEATURE_INDEX_LIST and got the > smaller kvm_feature_msrs. Then in kvm_arch_get_supported_msr_feature(), > searching VMX_BASIC will be failed and return 0. At last kvm_vmx_basic > will be assigned to 0. > > Without this patch, it will cause a qemu crash (host kernel 4.15 > ubuntu 18.04+qemu 4.1): > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0 > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret == > cpu->kvm_msr_buf->nmsrs' failed. > > This fixes it by skipping kvm_msr_entry_add when kvm_vmx_basic is 0 > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > --- > target/i386/kvm.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index a8c44bf..8cf84a2 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -2632,8 +2632,13 @@ static void kvm_msr_entry_add_vmx(X86CPU *cpu, FeatureWordArray f) > f[FEAT_VMX_SECONDARY_CTLS])); > kvm_msr_entry_add(cpu, MSR_IA32_VMX_EPT_VPID_CAP, > f[FEAT_VMX_EPT_VPID_CAPS] | fixed_vmx_ept_vpid); > - kvm_msr_entry_add(cpu, MSR_IA32_VMX_BASIC, > + > + if (kvm_vmx_basic) { > + /* Only add the entry when host supports it */ > + kvm_msr_entry_add(cpu, MSR_IA32_VMX_BASIC, > f[FEAT_VMX_BASIC] | fixed_vmx_basic); > + } > + > kvm_msr_entry_add(cpu, MSR_IA32_VMX_MISC, > f[FEAT_VMX_MISC] | fixed_vmx_misc); > if (has_msr_vmx_vmfunc) { > Yang Zhong from Intel also sent a similar patch. Thanks very much to both of you. Paolo
diff --git a/target/i386/kvm.c b/target/i386/kvm.c index bf16556..a8c44bf 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -2936,7 +2936,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level) (uint32_t)e->index, (uint64_t)e->data); } - assert(ret == cpu->kvm_msr_buf->nmsrs); + assert(ret <= cpu->kvm_msr_buf->nmsrs); return 0; }
Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally add vmx msr entry although older host kernels don't include them. But old host kernel + newest qemu will cause a qemu crash as follows: qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0 target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed. This fixes it by relaxing the condition. Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> --- target/i386/kvm.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)