| Message ID | 5142D010.7060303@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 15, 2013 at 08:38:56AM +0100, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Very old user space (namely qemu-kvm before kvm-49) didn't set the TSS > base before running the VCPU. We always warned about this bug, but no > reports about users actually seeing this are known. Time to finally > remove the workaround that effectively prevented to call vmx_vcpu_reset > while already holding the KVM srcu lock. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Reviewed-by: Gleb Natapov <gleb@redhat.com> > --- > > This obsoletes all other attempts to allow vmx_vcpu_reset to be called > with the srcu lock held. Hope we can settle the topic this way. > > arch/x86/kvm/vmx.c | 30 ++++-------------------------- > 1 files changed, 4 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 958ac3a..310bd00 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -2898,22 +2898,6 @@ static void enter_pmode(struct kvm_vcpu *vcpu) > vmx->cpl = 0; > } > > -static gva_t rmode_tss_base(struct kvm *kvm) > -{ > - if (!kvm->arch.tss_addr) { > - struct kvm_memslots *slots; > - struct kvm_memory_slot *slot; > - gfn_t base_gfn; > - > - slots = kvm_memslots(kvm); > - slot = id_to_memslot(slots, 0); > - base_gfn = slot->base_gfn + slot->npages - 3; > - > - return base_gfn << PAGE_SHIFT; > - } > - return kvm->arch.tss_addr; > -} > - > static void fix_rmode_seg(int seg, struct kvm_segment *save) > { > const struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg]; > @@ -2964,19 +2948,15 @@ static void enter_rmode(struct kvm_vcpu *vcpu) > > /* > * Very old userspace does not call KVM_SET_TSS_ADDR before entering > - * vcpu. Call it here with phys address pointing 16M below 4G. > + * vcpu. Warn the user that an update is overdue. > */ > - if (!vcpu->kvm->arch.tss_addr) { > + if (!vcpu->kvm->arch.tss_addr) > printk_once(KERN_WARNING "kvm: KVM_SET_TSS_ADDR need to be " > "called before entering vcpu\n"); > - srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx); > - vmx_set_tss_addr(vcpu->kvm, 0xfeffd000); > - vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); > - } > > vmx_segment_cache_clear(vmx); > > - vmcs_writel(GUEST_TR_BASE, rmode_tss_base(vcpu->kvm)); > + vmcs_writel(GUEST_TR_BASE, vcpu->kvm->arch.tss_addr); > vmcs_write32(GUEST_TR_LIMIT, RMODE_TSS_SIZE - 1); > vmcs_write32(GUEST_TR_AR_BYTES, 0x008b); > > @@ -3623,7 +3603,7 @@ static int init_rmode_tss(struct kvm *kvm) > int r, idx, ret = 0; > > idx = srcu_read_lock(&kvm->srcu); > - fn = rmode_tss_base(kvm) >> PAGE_SHIFT; > + fn = kvm->arch.tss_addr >> PAGE_SHIFT; > r = kvm_clear_guest_page(kvm, fn, 0, PAGE_SIZE); > if (r < 0) > goto out; > @@ -4190,9 +4170,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) > vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid); > > vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET; > - vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); > vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */ > - srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx); > vmx_set_cr4(&vmx->vcpu, 0); > vmx_set_efer(&vmx->vcpu, 0); > vmx_fpu_activate(&vmx->vcpu); > -- > 1.7.3.4 -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 15, 2013 at 08:38:56AM +0100, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Very old user space (namely qemu-kvm before kvm-49) didn't set the TSS > base before running the VCPU. We always warned about this bug, but no > reports about users actually seeing this are known. Time to finally > remove the workaround that effectively prevented to call vmx_vcpu_reset > while already holding the KVM srcu lock. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > > This obsoletes all other attempts to allow vmx_vcpu_reset to be called > with the srcu lock held. Hope we can settle the topic this way. Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, I just saw a new message after reboot today and searched for it and got https://patchwork.kernel.org/project/kvm/patch/5142D010.7060303@web.de/ and you might be interested that I ran into it. [ 70.747068] kvm: KVM_SET_TSS_ADDR need to be called before entering vcpu Full dmesg attached. This is on reboot, no VMs are running yet. I don’t get any extra weird messages on VM start, and they come up fine. bye, //mirabilos
Dropped all the old maintainers from Cc. This is one of the more impressive displays of thread necromancy I've seen :-) On Wed, Jun 28, 2023, Thorsten Glaser wrote: > Hi, > > I just saw a new message after reboot today and searched for it and got > https://patchwork.kernel.org/project/kvm/patch/5142D010.7060303@web.de/ > and you might be interested that I ran into it. > > [ 70.747068] kvm: KVM_SET_TSS_ADDR need to be called before entering vcpu > > Full dmesg attached. This is on reboot, no VMs are running yet. Heh, there are no VMs that _you_ deliberately created, but that doesn't mean there aren't VMs in the system. IIRC, libvirt (or maybe systemd?) probes KVM by doing modprobe *and* creating a dummy VM. If whatever is creating a VM also creates a vCPU, then the "soft" warning about KVM_SET_TSS_ADDR will trigger. Another possibility is that KVM sefltests are being run during boot. KVM's selftests stuff register state to force the vCPU into 64-bit mode, and so they don't bother setting KVM_SET_TSS_ADDR, e.g. the soft warning can be triggered by doing sudo modprobe kvm_intel unrestricted_guest=0 and running pretty much any KVM selftest. > I don’t get any extra weird messages on VM start, and they come up fine. So long as the VMs you care about don't have issues, the message is completely benign, and expected since you are running on Nehalem, which doesn't support unrestricted guest. > [ 0.000000] DMI: Gigabyte Technology Co., Ltd. X58-USB3/X58-USB3, BIOS F5 09/07/2011 > [ 0.330068] smpboot: CPU0: Intel(R) Core(TM) i7 CPU 950 @ 3.07GHz (family: 0x6, model: 0x1a, stepping: 0x5) > [ 70.747068] kvm: KVM_SET_TSS_ADDR need to be called before entering vcpu
On Wed, 28 Jun 2023, Sean Christopherson wrote: >Dropped all the old maintainers from Cc. This is one of the more impressive >displays of thread necromancy I've seen :-) Well, I found that mail, and it specifically mentioned users not having reported seeing this message, so… >> Full dmesg attached. This is on reboot, no VMs are running yet. > >Heh, there are no VMs that _you_ deliberately created, but that doesn't mean there >aren't VMs in the system. IIRC, libvirt (or maybe systemd?) probes KVM by doing >modprobe *and* creating a dummy VM. If whatever is creating a VM also creates a >vCPU, then the "soft" warning about KVM_SET_TSS_ADDR will trigger. No systemd here, but libvirt might be it. There’s significant time between the last kernel messages and this, that would explain it. >So long as the VMs you care about don't have issues, the message is completely >benign, and expected since you are running on Nehalem, which doesn't support >unrestricted guest. OK, thanks. Might want to make that more well-known… though I don’t have a good idea how. Perhaps the next person to run into that message will, at least, find this thread. bye, //mirabilos
On Wed, Jun 28, 2023, Thorsten Glaser wrote: > On Wed, 28 Jun 2023, Sean Christopherson wrote: > >So long as the VMs you care about don't have issues, the message is completely > >benign, and expected since you are running on Nehalem, which doesn't support > >unrestricted guest. > > OK, thanks. > > Might want to make that more well-known… though I don’t have a > good idea how. I'm leaning toward deleting the message entirely. It was placed there 10+ years ago to alert users that they needed to upgrade QEMU. At this point, if someone files a bug report against a 10+ year old QEMU build, the very first question is going to be if the issue reproduces on a more recent build. The only possible benefit I can think of is for people writing new VMMs from scratch, but any benefit there is also dubious for a variety of reasons.
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 958ac3a..310bd00 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2898,22 +2898,6 @@ static void enter_pmode(struct kvm_vcpu *vcpu) vmx->cpl = 0; } -static gva_t rmode_tss_base(struct kvm *kvm) -{ - if (!kvm->arch.tss_addr) { - struct kvm_memslots *slots; - struct kvm_memory_slot *slot; - gfn_t base_gfn; - - slots = kvm_memslots(kvm); - slot = id_to_memslot(slots, 0); - base_gfn = slot->base_gfn + slot->npages - 3; - - return base_gfn << PAGE_SHIFT; - } - return kvm->arch.tss_addr; -} - static void fix_rmode_seg(int seg, struct kvm_segment *save) { const struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg]; @@ -2964,19 +2948,15 @@ static void enter_rmode(struct kvm_vcpu *vcpu) /* * Very old userspace does not call KVM_SET_TSS_ADDR before entering - * vcpu. Call it here with phys address pointing 16M below 4G. + * vcpu. Warn the user that an update is overdue. */ - if (!vcpu->kvm->arch.tss_addr) { + if (!vcpu->kvm->arch.tss_addr) printk_once(KERN_WARNING "kvm: KVM_SET_TSS_ADDR need to be " "called before entering vcpu\n"); - srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx); - vmx_set_tss_addr(vcpu->kvm, 0xfeffd000); - vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); - } vmx_segment_cache_clear(vmx); - vmcs_writel(GUEST_TR_BASE, rmode_tss_base(vcpu->kvm)); + vmcs_writel(GUEST_TR_BASE, vcpu->kvm->arch.tss_addr); vmcs_write32(GUEST_TR_LIMIT, RMODE_TSS_SIZE - 1); vmcs_write32(GUEST_TR_AR_BYTES, 0x008b); @@ -3623,7 +3603,7 @@ static int init_rmode_tss(struct kvm *kvm) int r, idx, ret = 0; idx = srcu_read_lock(&kvm->srcu); - fn = rmode_tss_base(kvm) >> PAGE_SHIFT; + fn = kvm->arch.tss_addr >> PAGE_SHIFT; r = kvm_clear_guest_page(kvm, fn, 0, PAGE_SIZE); if (r < 0) goto out; @@ -4190,9 +4170,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu) vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid); vmx->vcpu.arch.cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET; - vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); vmx_set_cr0(&vmx->vcpu, kvm_read_cr0(vcpu)); /* enter rmode */ - srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx); vmx_set_cr4(&vmx->vcpu, 0); vmx_set_efer(&vmx->vcpu, 0); vmx_fpu_activate(&vmx->vcpu);