Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On Sun, Feb 27, 2011 at 06:31:13PM +0200, Avi Kivity wrote: > On 02/27/2011 06:27 PM, Gleb Natapov wrote: > >> > >> Or we can keep the old behaviour. If KVM_SET_TSS_ADDR hasn't been > >> called by the time of the first entry into real mode (the first > >> KVM_CREATE_VCPU?), use the top of the first slot. > >> > >Do we require that KVM_SET_TSS_ADDR is called before first KVM_CREATE_VCPU? > >We may still break some theoretical userspaces this way. > > The documentation doesn't say, but I think it's unlikely that > userspace will start a guest and then configure it. > KVM_CREATE_VCPU is done before guest is started. Starting of a guest happens when KVM_RUN is done for the first time. > > > >> We can avoid the SMP problem by initializing the memory in a single > >> pass, writing each byte exactly once with its final value. This way > >> concurrent initialization doesn't corrupt an in-use TSS. > >> > >Sounds hackish, but may work. Doing so will make entering pmode much > >more slow. > > Why? do it at most once per vcpu. > The reboot may fail since guest may have used the memory while running, so memory should be reinited on reboot, but since KVM doesn't always know about reboot it should be done at each protected mode entry. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On 02/27/2011 06:27 PM, Gleb Natapov wrote: > > Or we can keep the old behaviour. If KVM_SET_TSS_ADDR hasn't been > called by the time of the first entry into real mode (the first > KVM_CREATE_VCPU?), use the top of the first slot. > Do we require that KVM_SET_TSS_ADDR is called before first KVM_CREATE_VCPU? We may still break some theoretical userspaces this way. The documentation doesn't say, but I think it's unlikely that userspace will start a guest and then configure it. > We can avoid the SMP problem by initializing the memory in a single > pass, writing each byte exactly once with its final value. This way > concurrent initialization doesn't corrupt an in-use TSS. > Sounds hackish, but may work. Doing so will make entering pmode much more slow. Why? do it at most once per vcpu. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On Sun, Feb 27, 2011 at 06:04:16PM +0200, Avi Kivity wrote: > On 02/27/2011 05:58 PM, Avi Kivity wrote: > > > >>The problem with using top of slot > >>zero is that this memory is available for guest use and we do not even > >>put it into e820 map as far as I see. Also there are patches floating > >>around that re-arrange memslots or even put them in a tree. They will > >>break old guests too. > > > >Well, slot 0 still exists even if it is moved somewhere else. > > > >Something we can do is put the tss slot just below the highest > >slot that is still below 4G, and hope there is no mmio there. > >Once the user issues KVM_SET_TSS_ADDR, use that. We'll have to > >keep juggling that slot as the user creates more slots, icky. > > > > Or we can keep the old behaviour. If KVM_SET_TSS_ADDR hasn't been > called by the time of the first entry into real mode (the first > KVM_CREATE_VCPU?), use the top of the first slot. > Do we require that KVM_SET_TSS_ADDR is called before first KVM_CREATE_VCPU? We may still break some theoretical userspaces this way. > We can avoid the SMP problem by initializing the memory in a single > pass, writing each byte exactly once with its final value. This way > concurrent initialization doesn't corrupt an in-use TSS. > Sounds hackish, but may work. Doing so will make entering pmode much more slow. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On 02/27/2011 06:12 PM, Gleb Natapov wrote: On Sun, Feb 27, 2011 at 05:58:54PM +0200, Avi Kivity wrote: > On 02/27/2011 05:52 PM, Gleb Natapov wrote: > >> > >> According to my reading of the code, if KVM_SET_TSS_ADDR is not > >> invoked, the guest would fail both before and after the patch, yes? > >> > >Hmmm. Actually no. Before the patch guest that doesn't use KVM_SET_TSS_ADDR > >will use the top of slot zero. Should I fix that (how?), or should we > >drop support for those old guests? > > I don't think we have a problem with older qemus, but perhaps we do > with non-qemu users. The API clearly requires the ioctl to be > called, but I don't think we can blame anyone for forgetting to do > so, especially if it worked silently. > It may have worked as in "no error returned from KVM_RUN", but if userspace does not call to KVM_SET_TSS_ADDR kvm silently uses part of a guest memory to store its data which may cause guest to fail long after it was started. It is true that for that to happen guest needs to enter protected mode during its lifetime and not many guests do that usually. The only cases I can think of are during some guests installation and S3 suspend/resume. Right. I prefer to keep this partially working state if users didn't have a problem with it. > >The problem with using top of slot > >zero is that this memory is available for guest use and we do not even > >put it into e820 map as far as I see. Also there are patches floating > >around that re-arrange memslots or even put them in a tree. They will > >break old guests too. > > Well, slot 0 still exists even if it is moved somewhere else. > > Something we can do is put the tss slot just below the highest slot > that is still below 4G, and hope there is no mmio there. Once the > user issues KVM_SET_TSS_ADDR, use that. We'll have to keep juggling > that slot as the user creates more slots, icky. > I have a question about our current placement of tss addr. Qemu-kvm places it at 4G-16M and comment says that this is just below BIOS ROM, but BIOS ROM takes only upper 128K. Are you surprised that a comment is inaccurate? Likely it was moved to make room for larger bioses. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On Sun, Feb 27, 2011 at 05:58:54PM +0200, Avi Kivity wrote: > On 02/27/2011 05:52 PM, Gleb Natapov wrote: > >> > >> According to my reading of the code, if KVM_SET_TSS_ADDR is not > >> invoked, the guest would fail both before and after the patch, yes? > >> > >Hmmm. Actually no. Before the patch guest that doesn't use KVM_SET_TSS_ADDR > >will use the top of slot zero. Should I fix that (how?), or should we > >drop support for those old guests? > > I don't think we have a problem with older qemus, but perhaps we do > with non-qemu users. The API clearly requires the ioctl to be > called, but I don't think we can blame anyone for forgetting to do > so, especially if it worked silently. > It may have worked as in "no error returned from KVM_RUN", but if userspace does not call to KVM_SET_TSS_ADDR kvm silently uses part of a guest memory to store its data which may cause guest to fail long after it was started. It is true that for that to happen guest needs to enter protected mode during its lifetime and not many guests do that usually. The only cases I can think of are during some guests installation and S3 suspend/resume. > >The problem with using top of slot > >zero is that this memory is available for guest use and we do not even > >put it into e820 map as far as I see. Also there are patches floating > >around that re-arrange memslots or even put them in a tree. They will > >break old guests too. > > Well, slot 0 still exists even if it is moved somewhere else. > > Something we can do is put the tss slot just below the highest slot > that is still below 4G, and hope there is no mmio there. Once the > user issues KVM_SET_TSS_ADDR, use that. We'll have to keep juggling > that slot as the user creates more slots, icky. > I have a question about our current placement of tss addr. Qemu-kvm places it at 4G-16M and comment says that this is just below BIOS ROM, but BIOS ROM takes only upper 128K. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On 02/27/2011 05:58 PM, Avi Kivity wrote: The problem with using top of slot zero is that this memory is available for guest use and we do not even put it into e820 map as far as I see. Also there are patches floating around that re-arrange memslots or even put them in a tree. They will break old guests too. Well, slot 0 still exists even if it is moved somewhere else. Something we can do is put the tss slot just below the highest slot that is still below 4G, and hope there is no mmio there. Once the user issues KVM_SET_TSS_ADDR, use that. We'll have to keep juggling that slot as the user creates more slots, icky. Or we can keep the old behaviour. If KVM_SET_TSS_ADDR hasn't been called by the time of the first entry into real mode (the first KVM_CREATE_VCPU?), use the top of the first slot. We can avoid the SMP problem by initializing the memory in a single pass, writing each byte exactly once with its final value. This way concurrent initialization doesn't corrupt an in-use TSS. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On 02/27/2011 05:52 PM, Gleb Natapov wrote: > > According to my reading of the code, if KVM_SET_TSS_ADDR is not > invoked, the guest would fail both before and after the patch, yes? > Hmmm. Actually no. Before the patch guest that doesn't use KVM_SET_TSS_ADDR will use the top of slot zero. Should I fix that (how?), or should we drop support for those old guests? I don't think we have a problem with older qemus, but perhaps we do with non-qemu users. The API clearly requires the ioctl to be called, but I don't think we can blame anyone for forgetting to do so, especially if it worked silently. The problem with using top of slot zero is that this memory is available for guest use and we do not even put it into e820 map as far as I see. Also there are patches floating around that re-arrange memslots or even put them in a tree. They will break old guests too. Well, slot 0 still exists even if it is moved somewhere else. Something we can do is put the tss slot just below the highest slot that is still below 4G, and hope there is no mmio there. Once the user issues KVM_SET_TSS_ADDR, use that. We'll have to keep juggling that slot as the user creates more slots, icky. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On Sun, Feb 27, 2011 at 05:43:07PM +0200, Avi Kivity wrote: > On 02/21/2011 12:07 PM, Gleb Natapov wrote: > >Currently vm86 task is initialized on each real mode entry and vcpu > >reset. Initialization is done by zeroing TSS and updating relevant > >fields. But since all vcpus are using the same TSS there is a race where > >one vcpu may use TSS while other vcpu is initializing it, so the vcpu > >that uses TSS will see wrong TSS content and will behave incorrectly. > >Fix that by initializing TSS only once. > > Applied, thanks. > > According to my reading of the code, if KVM_SET_TSS_ADDR is not > invoked, the guest would fail both before and after the patch, yes? > Hmmm. Actually no. Before the patch guest that doesn't use KVM_SET_TSS_ADDR will use the top of slot zero. Should I fix that (how?), or should we drop support for those old guests? The problem with using top of slot zero is that this memory is available for guest use and we do not even put it into e820 map as far as I see. Also there are patches floating around that re-arrange memslots or even put them in a tree. They will break old guests too. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
On 02/21/2011 12:07 PM, Gleb Natapov wrote: Currently vm86 task is initialized on each real mode entry and vcpu reset. Initialization is done by zeroing TSS and updating relevant fields. But since all vcpus are using the same TSS there is a race where one vcpu may use TSS while other vcpu is initializing it, so the vcpu that uses TSS will see wrong TSS content and will behave incorrectly. Fix that by initializing TSS only once. Applied, thanks. According to my reading of the code, if KVM_SET_TSS_ADDR is not invoked, the guest would fail both before and after the patch, yes? -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.
Currently vm86 task is initialized on each real mode entry and vcpu
reset. Initialization is done by zeroing TSS and updating relevant
fields. But since all vcpus are using the same TSS there is a race where
one vcpu may use TSS while other vcpu is initializing it, so the vcpu
that uses TSS will see wrong TSS content and will behave incorrectly.
Fix that by initializing TSS only once.
Signed-off-by: Gleb Natapov
---
arch/x86/kvm/vmx.c | 28 ++--
1 files changed, 6 insertions(+), 22 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index dafb67e..e2b8c6b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -176,7 +176,6 @@ static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)
return container_of(vcpu, struct vcpu_vmx, vcpu);
}
-static int init_rmode(struct kvm *kvm);
static u64 construct_eptp(unsigned long root_hpa);
static void kvm_cpu_vmxon(u64 addr);
static void kvm_cpu_vmxoff(void);
@@ -1802,7 +1801,6 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
continue_rmode:
kvm_mmu_reset_context(vcpu);
- init_rmode(vcpu->kvm);
}
static void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
@@ -2737,22 +2735,6 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
return 0;
}
-static int init_rmode(struct kvm *kvm)
-{
- int idx, ret = 0;
-
- idx = srcu_read_lock(&kvm->srcu);
- if (!init_rmode_tss(kvm))
- goto exit;
- if (!init_rmode_identity_map(kvm))
- goto exit;
-
- ret = 1;
-exit:
- srcu_read_unlock(&kvm->srcu, idx);
- return ret;
-}
-
static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -2760,10 +2742,6 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
int ret;
vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP));
- if (!init_rmode(vmx->vcpu.kvm)) {
- ret = -ENOMEM;
- goto out;
- }
vmx->rmode.vm86_active = 0;
@@ -3009,6 +2987,9 @@ static int vmx_set_tss_addr(struct kvm *kvm, unsigned int
addr)
if (ret)
return ret;
kvm->arch.tss_addr = addr;
+ if (!init_rmode_tss(kvm))
+ return -ENOMEM;
+
return 0;
}
@@ -4224,8 +4205,11 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm,
unsigned int id)
if (!kvm->arch.ept_identity_map_addr)
kvm->arch.ept_identity_map_addr =
VMX_EPT_IDENTITY_PAGETABLE_ADDR;
+ err = -ENOMEM;
if (alloc_identity_pagetable(kvm) != 0)
goto free_vmcs;
+ if (!init_rmode_identity_map(kvm))
+ goto free_vmcs;
}
return &vmx->vcpu;
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
