[PATCH 1/2] KVM: VMX: write new TR selector value into vmcs immediately if it changes during vm86 mode.

[PATCH 1/2] KVM: VMX: write new TR selector value into vmcs immediately if it changes during vm86 mode.

[Gleb Natapov]

When vm86 is active TR descriptor is updated with vm86 task values,
but selector is left intact. vmx_set_segment() makes sure that if TR
register is written into while vm86 is active the new values are saved
for use after vm86 is deactivated, but since selector is not updated on
vm86 activation/deactivation new value is lost. Fix this by writing new
selector into vmcs immediately.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a61ed93..dafb67e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2138,6 +2138,7 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 	u32 ar;
 
 	if (vmx->rmode.vm86_active && seg == VCPU_SREG_TR) {
+		vmcs_write16(sf->selector, var->selector);
 		vmx->rmode.tr.selector = var->selector;
 		vmx->rmode.tr.base = var->base;
 		vmx->rmode.tr.limit = var->limit;
-- 
1.7.2.3

[PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Gleb Natapov]

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 <gleb@redhat.com>
---
 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

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Avi Kivity]

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

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Gleb Natapov]

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.

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Avi Kivity]

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

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Avi Kivity]

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

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Gleb Natapov]

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.

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Avi Kivity]

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

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Gleb Natapov]

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.

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Gleb Natapov]

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.

Re: [PATCH 2/2] KVM: VMX: Initialize vm86 TSS only once.

[Avi Kivity]

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

Re: [PATCH 1/2] KVM: VMX: write new TR selector value into vmcs immediately if it changes during vm86 mode.

[Avi Kivity]

On 02/21/2011 12:07 PM, Gleb Natapov wrote:
> When vm86 is active TR descriptor is updated with vm86 task values,
> but selector is left intact. vmx_set_segment() makes sure that if TR
> register is written into while vm86 is active the new values are saved
> for use after vm86 is deactivated, but since selector is not updated on
> vm86 activation/deactivation new value is lost. Fix this by writing new
> selector into vmcs immediately.

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function