KVM: unbreak userspace that does not sets tss address

PATCH Superseded Gleb Natapov 3 participants 7 messages Mar 7, 2011 – Mar 11, 2011

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Gleb Natapov

author

2011-03-07 15:19 UTC #0 lore raw

PATCH 1/1 KVM: unbreak userspace that does not sets tss address

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 <[email protected]>

arch/x86/kvm/vmx.c +11 -0

index e2b8c6b..280b6ea 100644
@@ -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.

Jan Kiszka

2011-03-07 15:49 UTC #1 lore raw

Quoted 4 lines · 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.

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

Signature · 2 lines

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

Gleb Natapov

author

2011-03-07 16:20 UTC #2 lore raw

Quoted 10 lines · Jan Kiszka

Quoted 4 lines · 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.
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.

Jan Kiszka

2011-03-07 16:23 UTC #3 lore raw

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Jan Kiszka
Quoted 4 lines · 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.
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?

Quoted 5 lines

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

Signature · 2 lines

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

Gleb Natapov

author

2011-03-07 16:26 UTC #4 lore raw

Quoted 12 lines · Jan Kiszka

Quoted 11 lines · Gleb Natapov
Quoted 10 lines · Jan Kiszka
Quoted 4 lines · 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.
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?

Quoted 9 lines

Quoted 5 lines
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.

Marcelo Tosatti

2011-03-11 17:58 UTC #5 lore raw

In reply to Gleb Natapov

Quoted 25 lines · Gleb Natapov

Signed-off-by Gleb Natapov <[email protected]>

arch/x86/kvm/vmx.c +4 -0

index e2b8c6b..280b6ea 100644
@@ -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.

arch/x86/kvm/vmx.c static void enter_rmode(struct kvm_vcpu *vcpu) · line 1778

Quoted 10 lines

arch/x86/kvm/vmx.c +7 -0

+	 */
+	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?

Gleb Natapov

author

2011-03-11 18:22 UTC #6 lore raw

Quoted 23 lines · Marcelo Tosatti

Quoted 22 lines · 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 <[email protected]>
arch/x86/kvm/vmx.c +1 -0
index e2b8c6b..280b6ea 100644
@@ -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;
 

Quoted 16 lines

arch/x86/kvm/vmx.c static void enter_rmode(struct kvm_vcpu *vcpu) · line 1778

Quoted 13 lines

arch/x86/kvm/vmx.c +10 -0

+	/*
+	 * 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.

Quoted 3 lines

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.