[PATCH] KVM: unbreak userspace that does not sets tss address

[PATCH] KVM: unbreak userspace that does not sets tss address

[Gleb Natapov]

Commit 6440e5967bc broke old userspaces that do not set tss address
before entering vcpu. Unbreak it by setting tss address to a safe
value on the first vcpu entry. New userspaces should set tss address,
so print warning in case it doesn't.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e2b8c6b..280b6ea 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -180,6 +180,7 @@ static u64 construct_eptp(unsigned long root_hpa);
 static void kvm_cpu_vmxon(u64 addr);
 static void kvm_cpu_vmxoff(void);
 static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
+static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
@@ -1762,6 +1763,16 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
 	vmx->emulation_required = 1;
 	vmx->rmode.vm86_active = 1;
 
+	/*
+	 * Very old userspace does not call KVM_SET_TSS_ADDR before entering
+	 * vcpu. Call it here with phys address pointing 16M below 4G.
+	 */
+	if (!vcpu->kvm->arch.tss_addr) {
+		printk_once(KERN_WARNING "kvm: KVM_SET_TSS_ADDR need to be "
+			     "called before entering vcpu\n");
+		vmx_set_tss_addr(vcpu->kvm, 0xfeffd000);
+	}
+
 	vmx->rmode.tr.selector = vmcs_read16(GUEST_TR_SELECTOR);
 	vmx->rmode.tr.base = vmcs_readl(GUEST_TR_BASE);
 	vmcs_writel(GUEST_TR_BASE, rmode_tss_base(vcpu->kvm));
--
			Gleb.

Re: [PATCH] KVM: unbreak userspace that does not sets tss address

[Jan Kiszka]

On 2011-03-07 16:19, Gleb Natapov wrote:
> Commit 6440e5967bc broke old userspaces that do not set tss address
> before entering vcpu. Unbreak it by setting tss address to a safe
> value on the first vcpu entry. New userspaces should set tss address,
> so print warning in case it doesn't.

Wouldn't allow runtime warnings but wouldn't it be cleaner otherwise to
start vcpus with a default tss address like that, ie. add the logic to
vmx_create_vcpu?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: unbreak userspace that does not sets tss address

[Gleb Natapov]

On Mon, Mar 07, 2011 at 04:49:28PM +0100, Jan Kiszka wrote:
> On 2011-03-07 16:19, Gleb Natapov wrote:
> > Commit 6440e5967bc broke old userspaces that do not set tss address
> > before entering vcpu. Unbreak it by setting tss address to a safe
> > value on the first vcpu entry. New userspaces should set tss address,
> > so print warning in case it doesn't.
> 
> Wouldn't allow runtime warnings but wouldn't it be cleaner otherwise to
> start vcpus with a default tss address like that, ie. add the logic to
> vmx_create_vcpu?
> 
Userspace can create VCPU before setting tss address. We will have to
have logic that removes default tss slot in set_tss_address callback.
Also this default slot may prevent userspace from creating another slot
at this physical address which is perfectly legitimate thing for userspace
to do.

--
			Gleb.

Re: [PATCH] KVM: unbreak userspace that does not sets tss address

[Jan Kiszka]

On 2011-03-07 17:20, Gleb Natapov wrote:
> On Mon, Mar 07, 2011 at 04:49:28PM +0100, Jan Kiszka wrote:
>> On 2011-03-07 16:19, Gleb Natapov wrote:
>>> Commit 6440e5967bc broke old userspaces that do not set tss address
>>> before entering vcpu. Unbreak it by setting tss address to a safe
>>> value on the first vcpu entry. New userspaces should set tss address,
>>> so print warning in case it doesn't.
>>
>> Wouldn't allow runtime warnings but wouldn't it be cleaner otherwise to
>> start vcpus with a default tss address like that, ie. add the logic to
>> vmx_create_vcpu?
>>
> Userspace can create VCPU before setting tss address. We will have to
> have logic that removes default tss slot in set_tss_address callback.
> Also this default slot may prevent userspace from creating another slot
> at this physical address which is perfectly legitimate thing for userspace
> to do.

Ah, ok. I thought you could simple override the tss slot by calling the
corresponding service again.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: unbreak userspace that does not sets tss address

[Gleb Natapov]

On Mon, Mar 07, 2011 at 05:23:02PM +0100, Jan Kiszka wrote:
> On 2011-03-07 17:20, Gleb Natapov wrote:
> > On Mon, Mar 07, 2011 at 04:49:28PM +0100, Jan Kiszka wrote:
> >> On 2011-03-07 16:19, Gleb Natapov wrote:
> >>> Commit 6440e5967bc broke old userspaces that do not set tss address
> >>> before entering vcpu. Unbreak it by setting tss address to a safe
> >>> value on the first vcpu entry. New userspaces should set tss address,
> >>> so print warning in case it doesn't.
> >>
> >> Wouldn't allow runtime warnings but wouldn't it be cleaner otherwise to
> >> start vcpus with a default tss address like that, ie. add the logic to
> >> vmx_create_vcpu?
> >>
> > Userspace can create VCPU before setting tss address. We will have to
> > have logic that removes default tss slot in set_tss_address callback.
> > Also this default slot may prevent userspace from creating another slot
> > at this physical address which is perfectly legitimate thing for userspace
> > to do.
> 
> Ah, ok. I thought you could simple override the tss slot by calling the
> corresponding service again.
> 
I think you need to delete it first, but even if not the second problem
remains.

--
			Gleb.

Re: [PATCH] KVM: unbreak userspace that does not sets tss address

[Marcelo Tosatti]

On Mon, Mar 07, 2011 at 05:19:36PM +0200, Gleb Natapov wrote:
> Commit 6440e5967bc broke old userspaces that do not set tss address
> before entering vcpu. Unbreak it by setting tss address to a safe
> value on the first vcpu entry. New userspaces should set tss address,
> so print warning in case it doesn't.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index e2b8c6b..280b6ea 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -180,6 +180,7 @@ static u64 construct_eptp(unsigned long root_hpa);
>  static void kvm_cpu_vmxon(u64 addr);
>  static void kvm_cpu_vmxoff(void);
>  static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
> +static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
>  
>  static DEFINE_PER_CPU(struct vmcs *, vmxarea);
>  static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
> @@ -1762,6 +1763,16 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
>  	vmx->emulation_required = 1;
>  	vmx->rmode.vm86_active = 1;
>  
> +	/*
> +	 * Very old userspace does not call KVM_SET_TSS_ADDR before entering
> +	 * vcpu. Call it here with phys address pointing 16M below 4G.
> +	 */
> +	if (!vcpu->kvm->arch.tss_addr) {
> +		printk_once(KERN_WARNING "kvm: KVM_SET_TSS_ADDR need to be "
> +			     "called before entering vcpu\n");
> +		vmx_set_tss_addr(vcpu->kvm, 0xfeffd000);
> +	}
> +
>  	vmx->rmode.tr.selector = vmcs_read16(GUEST_TR_SELECTOR);
>  	vmx->rmode.tr.base = vmcs_readl(GUEST_TR_BASE);
>  	vmcs_writel(GUEST_TR_BASE, rmode_tss_base(vcpu->kvm));

Should leave/enter srcu section before/after set_memory_region. 

Actually, for older userspace, isnt the base_gfn + slot[0].npages - 3
address (rmode_tss_base) used? That is, initializing the TSS page at 
that address at vcpu_reset time should be enough?

Re: [PATCH] KVM: unbreak userspace that does not sets tss address

[Gleb Natapov]

On Fri, Mar 11, 2011 at 02:30:32PM -0300, Marcelo Tosatti wrote:
> On Mon, Mar 07, 2011 at 05:19:36PM +0200, Gleb Natapov wrote:
> > Commit 6440e5967bc broke old userspaces that do not set tss address
> > before entering vcpu. Unbreak it by setting tss address to a safe
> > value on the first vcpu entry. New userspaces should set tss address,
> > so print warning in case it doesn't.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index e2b8c6b..280b6ea 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -180,6 +180,7 @@ static u64 construct_eptp(unsigned long root_hpa);
> >  static void kvm_cpu_vmxon(u64 addr);
> >  static void kvm_cpu_vmxoff(void);
> >  static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
> > +static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
> >  
> >  static DEFINE_PER_CPU(struct vmcs *, vmxarea);
> >  static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
> > @@ -1762,6 +1763,16 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
> >  	vmx->emulation_required = 1;
> >  	vmx->rmode.vm86_active = 1;
> >  
> > +	/*
> > +	 * Very old userspace does not call KVM_SET_TSS_ADDR before entering
> > +	 * vcpu. Call it here with phys address pointing 16M below 4G.
> > +	 */
> > +	if (!vcpu->kvm->arch.tss_addr) {
> > +		printk_once(KERN_WARNING "kvm: KVM_SET_TSS_ADDR need to be "
> > +			     "called before entering vcpu\n");
> > +		vmx_set_tss_addr(vcpu->kvm, 0xfeffd000);
> > +	}
> > +
> >  	vmx->rmode.tr.selector = vmcs_read16(GUEST_TR_SELECTOR);
> >  	vmx->rmode.tr.base = vmcs_readl(GUEST_TR_BASE);
> >  	vmcs_writel(GUEST_TR_BASE, rmode_tss_base(vcpu->kvm));
> 
> Should leave/enter srcu section before/after set_memory_region. 
> 
OK.

> Actually, for older userspace, isnt the base_gfn + slot[0].npages - 3
> address (rmode_tss_base) used? That is, initializing the TSS page at 
> that address at vcpu_reset time should be enough?
No, since this points to real memory and nothing prevents guest to write
to this memory at any time (we do not even mark it reserved in e820 map)
it needs to be reinitialized at every rmode entry and this is was we are
trying to avoid. And initialization in vcpu_reset() has the same race
that is fixed by 6440e5967bc007827de2ba57a9928e120c6af86d. When one
cpu resets another may run in rmode and use tss.

--
			Gleb.