[RFC PATCH v3 0/3] Add segment limit checks to emulator

[RFC PATCH v3 0/3] Add segment limit checks to emulator

[Mohammed Gamal]

fter some conversation with Avi concerning why unreal mode has been seen to work
with KVM on Intel. It clears out the scenario is caused as follows:

- guest enters big real mode
- kvm squashes limit to 64k-1
- guest executes instructions with offset > 64k
- cpu issues #GP due to limit violation
- kvm handle_rmode_exception() -> emulator
- emulator ignores limit, emulates instruction

With these applied I am getting vmentry failures with SeaBIOS and
gPXE. I could still get SeaBIOS to work with emulate_invalid_guest_state=1.
So it's needless to say that these patches are not meant for merging!

--------

Changes from v2:
- Addeded generic segment limit check helpers
- Removed individual segment register segment helpers as they're no longer needed

--------

Mohammed Gamal (3):
  Add helper methods to get segment limits
  x86 emulator: Add segment limit checking helpers
  x86 emulator: Add segment limit checks to emulator

 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/include/asm/kvm_host.h    |    1 +
 arch/x86/kvm/emulate.c             |  112 ++++++++++++++++++++++++++++-------
 arch/x86/kvm/svm.c                 |    8 +++
 arch/x86/kvm/vmx.c                 |    8 +++
 arch/x86/kvm/x86.c                 |   12 ++++
 6 files changed, 119 insertions(+), 23 deletions(-)

[RFC PATCH v3 1/3] Add helper methods to get segment limits

[Mohammed Gamal]

This adds helper methods to get segment limits for kvm_x86_ops and x86_emulate_ops. Hooks are added for SVM and VMX

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/include/asm/kvm_host.h    |    1 +
 arch/x86/kvm/svm.c                 |    8 ++++++++
 arch/x86/kvm/vmx.c                 |    8 ++++++++
 arch/x86/kvm/x86.c                 |   12 ++++++++++++
 5 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 51cfd73..ce90048 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -138,6 +138,7 @@ struct x86_emulate_ops {
 	u16 (*get_segment_selector)(int seg, struct kvm_vcpu *vcpu);
 	void (*set_segment_selector)(u16 sel, int seg, struct kvm_vcpu *vcpu);
 	unsigned long (*get_cached_segment_base)(int seg, struct kvm_vcpu *vcpu);
+	u32 (*get_cached_segment_limit)(int seg, struct kvm_vcpu *vcpu);
 	void (*get_gdt)(struct desc_ptr *dt, struct kvm_vcpu *vcpu);
 	ulong (*get_cr)(int cr, struct kvm_vcpu *vcpu);
 	int (*set_cr)(int cr, ulong val, struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 502e53f..e32efc4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -468,6 +468,7 @@ struct kvm_x86_ops {
 	int (*get_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata);
 	int (*set_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
 	u64 (*get_segment_base)(struct kvm_vcpu *vcpu, int seg);
+	u32 (*get_segment_limit)(struct kvm_vcpu *vcpu, int seg);
 	void (*get_segment)(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg);
 	int (*get_cpl)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 56c9b6b..504761d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1038,6 +1038,13 @@ static u64 svm_get_segment_base(struct kvm_vcpu *vcpu, int seg)
 	return s->base;
 }
 
+static u32 svm_get_segment_limit(struct kvm_vcpu *vcpu, int seg)
+{
+	struct vmcb_seg *s = svm_seg(vcpu, seg);
+
+	return s->limit;
+}
+
 static void svm_get_segment(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg)
 {
@@ -3461,6 +3468,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.get_msr = svm_get_msr,
 	.set_msr = svm_set_msr,
 	.get_segment_base = svm_get_segment_base,
+	.get_segment_limit = svm_get_segment_limit,
 	.get_segment = svm_get_segment,
 	.set_segment = svm_set_segment,
 	.get_cpl = svm_get_cpl,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 2fdcc98..4e2626b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2001,6 +2001,13 @@ static u64 vmx_get_segment_base(struct kvm_vcpu *vcpu, int seg)
 	return vmcs_readl(sf->base);
 }
 
+static u32 vmx_get_segment_limit(struct kvm_vcpu *vcpu, int seg)
+{
+	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
+
+	return vmcs_read32(sf->limit);
+}
+
 static void vmx_get_segment(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg)
 {
@@ -4304,6 +4311,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.get_msr = vmx_get_msr,
 	.set_msr = vmx_set_msr,
 	.get_segment_base = vmx_get_segment_base,
+	.get_segment_limit = vmx_get_segment_limit,
 	.get_segment = vmx_get_segment,
 	.set_segment = vmx_set_segment,
 	.get_cpl = vmx_get_cpl,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6ed3176..23b9d2c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3674,6 +3674,11 @@ static unsigned long get_segment_base(struct kvm_vcpu *vcpu, int seg)
 	return kvm_x86_ops->get_segment_base(vcpu, seg);
 }
 
+static u32 get_segment_limit (struct kvm_vcpu *vcpu, int seg)
+{
+	return kvm_x86_ops->get_segment_limit(vcpu, seg);
+}
+
 int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)
 {
 	kvm_mmu_invlpg(vcpu, address);
@@ -3790,6 +3795,12 @@ static unsigned long emulator_get_cached_segment_base(int seg,
 	return get_segment_base(vcpu, seg);
 }
 
+static u32 emulator_get_cached_segment_limit(int seg,
+					    struct kvm_vcpu *vcpu)
+{
+	return get_segment_limit(vcpu, seg);
+}
+
 static bool emulator_get_cached_descriptor(struct desc_struct *desc, int seg,
 					   struct kvm_vcpu *vcpu)
 {
@@ -3876,6 +3887,7 @@ static struct x86_emulate_ops emulate_ops = {
 	.get_segment_selector = emulator_get_segment_selector,
 	.set_segment_selector = emulator_set_segment_selector,
 	.get_cached_segment_base = emulator_get_cached_segment_base,
+	.get_cached_segment_limit = emulator_get_cached_segment_limit,
 	.get_gdt             = emulator_get_gdt,
 	.get_cr              = emulator_get_cr,
 	.set_cr              = emulator_set_cr,
-- 
1.7.0.4

[RFC PATCH v3 2/3] x86 emulator: Add segment limit checking helpers

[Mohammed Gamal]

Adds segment limit checking helper functions. Also added a cs_base() helper while at it.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/emulate.c |   33 ++++++++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 255473f..07ca28e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -633,6 +633,15 @@ static unsigned long seg_base(struct x86_emulate_ctxt *ctxt,
 	return ops->get_cached_segment_base(seg, ctxt->vcpu);
 }
 
+static u64 seg_limit(struct x86_emulate_ctxt *ctxt,
+		     struct x86_emulate_ops *ops, int seg)
+{
+	if (ctxt->mode == X86EMUL_MODE_PROT64)
+		return -1ULL;
+
+	return ops->get_cached_segment_limit(seg, ctxt->vcpu);
+}
+
 static unsigned long seg_override_base(struct x86_emulate_ctxt *ctxt,
 				       struct x86_emulate_ops *ops,
 				       struct decode_cache *c)
@@ -643,6 +652,12 @@ static unsigned long seg_override_base(struct x86_emulate_ctxt *ctxt,
 	return seg_base(ctxt, ops, c->seg_override);
 }
 
+static unsigned long cs_base(struct x86_emulate_ctxt *ctxt,
+			     struct x86_emulate_ops *ops)
+{
+	return seg_base(ctxt, ops, VCPU_SREG_CS);
+}
+
 static unsigned long es_base(struct x86_emulate_ctxt *ctxt,
 			     struct x86_emulate_ops *ops)
 {
@@ -686,6 +701,22 @@ static void emulate_ts(struct x86_emulate_ctxt *ctxt, int err)
 	emulate_exception(ctxt, TS_VECTOR, err, true);
 }
 
+static int seg_limit_check(struct x86_emulate_ctxt *ctxt,
+			   struct x86_emulate_ops *ops, int seg,
+			   unsigned long addr, unsigned size,
+			   int vec, u32 err)
+{
+	unsigned long base = seg_base(ctxt, ops, seg);
+	u64 limit = seg_limit(ctxt, ops, seg);
+
+	if (addr - base + size - 1 > limit) {
+		emulate_exception(ctxt, vec, err, true);
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
+	return X86EMUL_CONTINUE;
+}
+
 static int do_fetch_insn_byte(struct x86_emulate_ctxt *ctxt,
 			      struct x86_emulate_ops *ops,
 			      unsigned long eip, u8 *dest)
@@ -976,7 +1007,7 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 
 	c->eip = ctxt->eip;
 	c->fetch.start = c->fetch.end = c->eip;
-	ctxt->cs_base = seg_base(ctxt, ops, VCPU_SREG_CS);
+	ctxt->cs_base = cs_base(ctxt, ops);
 
 	switch (mode) {
 	case X86EMUL_MODE_REAL:
-- 
1.7.0.4

[RFC PATCH v3 3/3] x86 emulator: Add segment limit checks to emulator functions

[Mohammed Gamal]

This adds segment limit checks to the emulator. Also changes return value of
emulate_push() and its callers accordingly.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/emulate.c |   79 ++++++++++++++++++++++++++++++++++-------------
 1 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 07ca28e..6cf6bee 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -728,6 +728,10 @@ static int do_fetch_insn_byte(struct x86_emulate_ctxt *ctxt,
 	if (eip == fc->end) {
 		cur_size = fc->end - fc->start;
 		size = min(15UL - cur_size, PAGE_SIZE - offset_in_page(eip));
+		rc = seg_limit_check(ctxt, ops, VCPU_SREGS_CS, ctxt->cs_base + eip,
+				size, GP_VECTOR, 0);
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
 		rc = ops->fetch(ctxt->cs_base + eip, fc->data + cur_size,
 				size, ctxt->vcpu, NULL);
 		if (rc != X86EMUL_CONTINUE)
@@ -1248,6 +1252,8 @@ done_prefixes:
 			register_address(c,  seg_override_base(ctxt, ops, c),
 					 c->regs[VCPU_REGS_RSI]);
 		c->src.val = 0;
+		rc = seg_limit_check(ctxt, ops, c->seg_override, c->src.ptr,
+				c->src.bytes, GP_VECTOR, 0);
 		break;
 	case SrcImmFAddr:
 		c->src.type = OP_IMM;
@@ -1344,6 +1350,8 @@ done_prefixes:
 			register_address(c, es_base(ctxt, ops),
 					 c->regs[VCPU_REGS_RDI]);
 		c->dst.val = 0;
+		rc = seg_limit_check(ctxt, ops, VCPU_SREGS_ES, c->dst.ptr,
+				c->dst.bytes, GP_VECTOR, 0);
 		break;
 	}
 
@@ -1662,7 +1670,7 @@ static inline int writeback(struct x86_emulate_ctxt *ctxt,
 	return X86EMUL_CONTINUE;
 }
 
-static inline void emulate_push(struct x86_emulate_ctxt *ctxt,
+static inline int emulate_push(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
@@ -1673,6 +1681,8 @@ static inline void emulate_push(struct x86_emulate_ctxt *ctxt,
 	register_address_increment(c, &c->regs[VCPU_REGS_RSP], -c->op_bytes);
 	c->dst.ptr = (void *) register_address(c, ss_base(ctxt, ops),
 					       c->regs[VCPU_REGS_RSP]);
+	return seg_limit_check(ctxt, ops, VCPU_SREGS_SS, c->dst.ptr, c->dst.bytes,
+			SS_VECTOR, 0);
 }
 
 static int emulate_pop(struct x86_emulate_ctxt *ctxt,
@@ -1680,11 +1690,16 @@ static int emulate_pop(struct x86_emulate_ctxt *ctxt,
 		       void *dest, int len)
 {
 	struct decode_cache *c = &ctxt->decode;
+	unsigned long reg_addr = register_address(c, ss_base(ctxt, ops),
+						c->regs[VCPU_REGS_RSP]);
 	int rc;
 
-	rc = read_emulated(ctxt, ops, register_address(c, ss_base(ctxt, ops),
-						       c->regs[VCPU_REGS_RSP]),
-			   dest, len);
+
+	rc = read_emulated(ctxt, ops, reg_addr, dest, len);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
+	rc = seg_limit_check(ctxt, ops, VCPU_SREGS_SS, reg_addr, len,
+			SS_VECTOR, 0);
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
 
@@ -1735,14 +1750,14 @@ static int emulate_popf(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
-static void emulate_push_sreg(struct x86_emulate_ctxt *ctxt,
+static int emulate_push_sreg(struct x86_emulate_ctxt *ctxt,
 			      struct x86_emulate_ops *ops, int seg)
 {
 	struct decode_cache *c = &ctxt->decode;
 
 	c->src.val = ops->get_segment_selector(seg, ctxt->vcpu);
 
-	emulate_push(ctxt, ops);
+	return emulate_push(ctxt, ops);
 }
 
 static int emulate_pop_sreg(struct x86_emulate_ctxt *ctxt,
@@ -1772,7 +1787,9 @@ static int emulate_pusha(struct x86_emulate_ctxt *ctxt,
 		(reg == VCPU_REGS_RSP) ?
 		(c->src.val = old_esp) : (c->src.val = c->regs[reg]);
 
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
 
 		rc = writeback(ctxt, ops);
 		if (rc != X86EMUL_CONTINUE)
@@ -1884,15 +1901,13 @@ static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
 		old_eip = c->eip;
 		c->eip = c->src.val;
 		c->src.val = old_eip;
-		emulate_push(ctxt, ops);
-		break;
+		return emulate_push(ctxt, ops);
 	}
 	case 4: /* jmp abs */
 		c->eip = c->src.val;
 		break;
 	case 6:	/* push */
-		emulate_push(ctxt, ops);
-		break;
+		return emulate_push(ctxt, ops);
 	}
 	return X86EMUL_CONTINUE;
 }
@@ -2548,7 +2563,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 		c->op_bytes = c->ad_bytes = (next_tss_desc.type & 8) ? 4 : 2;
 		c->lock_prefix = 0;
 		c->src.val = (unsigned long) error_code;
-		emulate_push(ctxt, ops);
+		return emulate_push(ctxt, ops);
 	}
 
 	return ret;
@@ -2681,7 +2696,9 @@ special_insn:
 		emulate_2op_SrcV("add", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0x06:		/* push es */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_ES);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_ES);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x07:		/* pop es */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_ES);
@@ -2693,14 +2710,18 @@ special_insn:
 		emulate_2op_SrcV("or", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0x0e:		/* push cs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_CS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_CS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x10 ... 0x15:
 	      adc:		/* adc */
 		emulate_2op_SrcV("adc", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0x16:		/* push ss */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_SS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_SS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x17:		/* pop ss */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_SS);
@@ -2712,7 +2733,9 @@ special_insn:
 		emulate_2op_SrcV("sbb", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0x1e:		/* push ds */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_DS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_DS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x1f:		/* pop ds */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_DS);
@@ -2742,7 +2765,9 @@ special_insn:
 		emulate_1op("dec", c->dst, ctxt->eflags);
 		break;
 	case 0x50 ... 0x57:  /* push reg */
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x58 ... 0x5f: /* pop reg */
 	pop_instruction:
@@ -2767,7 +2792,9 @@ special_insn:
 		break;
 	case 0x68: /* push imm */
 	case 0x6a: /* push imm8 */
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x6c:		/* insb */
 	case 0x6d:		/* insw/insd */
@@ -2895,7 +2922,9 @@ special_insn:
 		goto xchg;
 	case 0x9c: /* pushf */
 		c->src.val =  (unsigned long) ctxt->eflags;
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x9d: /* popf */
 		c->dst.type = OP_REG;
@@ -2959,7 +2988,9 @@ special_insn:
 		long int rel = c->src.val;
 		c->src.val = (unsigned long) c->eip;
 		jmp_rel(c, rel);
-		emulate_push(ctxt, ops);
+		rc = emulate_push(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	}
 	case 0xe9: /* jmp rel */
@@ -3286,7 +3317,9 @@ twobyte_insn:
 		c->dst.type = OP_NONE;
 		break;
 	case 0xa0:	  /* push fs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_FS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_FS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0xa1:	 /* pop fs */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_FS);
@@ -3305,7 +3338,9 @@ twobyte_insn:
 		emulate_2op_cl("shld", c->src2, c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xa8:	/* push gs */
-		emulate_push_sreg(ctxt, ops, VCPU_SREG_GS);
+		rc = emulate_push_sreg(ctxt, ops, VCPU_SREG_GS);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0xa9:	/* pop gs */
 		rc = emulate_pop_sreg(ctxt, ops, VCPU_SREG_GS);
-- 
1.7.0.4

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Avi Kivity]

On 07/12/2010 01:56 AM, Mohammed Gamal wrote:
> fter some conversation with Avi concerning why unreal mode has been seen to work
> with KVM on Intel. It clears out the scenario is caused as follows:
>
> - guest enters big real mode
> - kvm squashes limit to 64k-1
> - guest executes instructions with offset>  64k
> - cpu issues #GP due to limit violation
> - kvm handle_rmode_exception() ->  emulator
> - emulator ignores limit, emulates instruction
>
> With these applied I am getting vmentry failures with SeaBIOS and
> gPXE. I could still get SeaBIOS to work with emulate_invalid_guest_state=1.
> So it's needless to say that these patches are not meant for merging!
>    

Well, eventually you need to fix this.

> --------
>
> Changes from v2:
> - Addeded generic segment limit check helpers
> - Removed individual segment register segment helpers as they're no longer needed
>
>    

What about the rest of my comments?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Mohammed Gamal]

On Mon, Jul 12, 2010 at 9:26 AM, Avi Kivity <avi@redhat.com> wrote:
> On 07/12/2010 01:56 AM, Mohammed Gamal wrote:
>>
>> fter some conversation with Avi concerning why unreal mode has been seen
>> to work
>> with KVM on Intel. It clears out the scenario is caused as follows:
>>
>> - guest enters big real mode
>> - kvm squashes limit to 64k-1
>> - guest executes instructions with offset>  64k
>> - cpu issues #GP due to limit violation
>> - kvm handle_rmode_exception() ->  emulator
>> - emulator ignores limit, emulates instruction
>>
>> With these applied I am getting vmentry failures with SeaBIOS and
>> gPXE. I could still get SeaBIOS to work with
>> emulate_invalid_guest_state=1.
>> So it's needless to say that these patches are not meant for merging!
>>
>
> Well, eventually you need to fix this.
What happens is that guests are switched to big real mode so either
gPXE and SeaBIOS need to be modified to work with the way KVM handles
segment limits when switching to real mode, but that'd be only a
temporary solution. The other - and better IMO - option is to get
e_i_g_s=1 completely functional, which is something we want to do
anyway. So we can address all the comments you have on these patches
and eventually merge them along with the rest of e_i_g_s patches.

>
>> --------
>>
>> Changes from v2:
>> - Addeded generic segment limit check helpers
>> - Removed individual segment register segment helpers as they're no longer
>> needed
>>
>>
>
> What about the rest of my comments?
I did change the limit calculations to avoid overflows, and
re-arranged patches as per your suggestion. Sorry for not pointing
this out in the change log. Check the patches I sent out for details.

>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
>

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Avi Kivity]

On 07/12/2010 03:36 PM, Mohammed Gamal wrote:
> On Mon, Jul 12, 2010 at 9:26 AM, Avi Kivity<avi@redhat.com>  wrote:
>    
>> On 07/12/2010 01:56 AM, Mohammed Gamal wrote:
>>      
>>> fter some conversation with Avi concerning why unreal mode has been seen
>>> to work
>>> with KVM on Intel. It clears out the scenario is caused as follows:
>>>
>>> - guest enters big real mode
>>> - kvm squashes limit to 64k-1
>>> - guest executes instructions with offset>    64k
>>> - cpu issues #GP due to limit violation
>>> - kvm handle_rmode_exception() ->    emulator
>>> - emulator ignores limit, emulates instruction
>>>
>>> With these applied I am getting vmentry failures with SeaBIOS and
>>> gPXE. I could still get SeaBIOS to work with
>>> emulate_invalid_guest_state=1.
>>> So it's needless to say that these patches are not meant for merging!
>>>
>>>        
>> Well, eventually you need to fix this.
>>      
> What happens is that guests are switched to big real mode so either
> gPXE and SeaBIOS need to be modified to work with the way KVM handles
> segment limits when switching to real mode, but that'd be only a
> temporary solution. The other - and better IMO - option is to get
> e_i_g_s=1 completely functional, which is something we want to do
> anyway. So we can address all the comments you have on these patches
> and eventually merge them along with the rest of e_i_g_s patches.
>    

Does SeaBIOS use big real mode now?

I think this can work even with e_i_g_s=0.  Simply return 
vmx->rmode.seg.limit instead of GUEST_seg_LIMIT.  In fact we need to do 
this anyway, so live migration migrates the correct limit, not the hack 
that we do for vmx.

>>> --------
>>>
>>> Changes from v2:
>>> - Addeded generic segment limit check helpers
>>> - Removed individual segment register segment helpers as they're no longer
>>> needed
>>>
>>>
>>>        
>> What about the rest of my comments?
>>      
> I did change the limit calculations to avoid overflows, and
> re-arranged patches as per your suggestion. Sorry for not pointing
> this out in the change log. Check the patches I sent out for details.
>    

What about expand-down segments?  and moving the limit check where the 
access is emulated (so we are sure we don't miss a check)?

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

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Avi Kivity]

On 07/12/2010 04:39 PM, Mohammed Gamal wrote:
>
>>> What happens is that guests are switched to big real mode so either
>>> gPXE and SeaBIOS need to be modified to work with the way KVM handles
>>> segment limits when switching to real mode, but that'd be only a
>>> temporary solution. The other - and better IMO - option is to get
>>> e_i_g_s=1 completely functional, which is something we want to do
>>> anyway. So we can address all the comments you have on these patches
>>> and eventually merge them along with the rest of e_i_g_s patches.
>>>
>>>        
>> Does SeaBIOS use big real mode now?
>>      
> I think so, ftrace shows a CR0 access just before the instruction that
> causes the failure. I am not 100% sure though.
>    

Ok, will be good to know.  In any case, I think it can be made to work 
even without e_i_g_s=1.


>> What about expand-down segments?  and moving the limit check where the
>> access is emulated (so we are sure we don't miss a check)?
>>      
> Let me make sure I am understanding this correctly. I added a check in
> do_insn_fetch_byte() checking for CS limit. Similar checks in
> emulate_push() ane emulate_pop() for SS, and checks in
> x86_decode_insn() for SrcSI and DstDI since they causes accesses to
> segment override and ES respectively. Are we on the same page?
>    

You have to do the check wherever you have a read or write that is 
qualified by a segment.  So the best place for them is in 
->read_emulated(), ->write_emulated(), and similar.

A good way to do this is to add a segment variable to 'struct operand', 
and doing all the base adjustment at the end (instead of up front as we 
do now).  That means we'll have the minimum number of places to add 
checks to.


> I haven't looked into expand down segments, but I don't think it's
> much of an effort to add though.
>    

It's needed, since guests will start failing mysteriously if they use 
those segments and the limits are incorrect (though I doubt there are 
any guests which use expand-down segments).

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

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Gleb Natapov]

On Mon, Jul 12, 2010 at 04:51:10PM +0300, Avi Kivity wrote:
> On 07/12/2010 04:39 PM, Mohammed Gamal wrote:
> >
> >>>What happens is that guests are switched to big real mode so either
> >>>gPXE and SeaBIOS need to be modified to work with the way KVM handles
> >>>segment limits when switching to real mode, but that'd be only a
> >>>temporary solution. The other - and better IMO - option is to get
> >>>e_i_g_s=1 completely functional, which is something we want to do
> >>>anyway. So we can address all the comments you have on these patches
> >>>and eventually merge them along with the rest of e_i_g_s patches.
> >>>
> >>Does SeaBIOS use big real mode now?
> >I think so, ftrace shows a CR0 access just before the instruction that
> >causes the failure. I am not 100% sure though.
> 
> Ok, will be good to know.  In any case, I think it can be made to
> work even without e_i_g_s=1.
> 
> 
> >>What about expand-down segments?  and moving the limit check where the
> >>access is emulated (so we are sure we don't miss a check)?
> >Let me make sure I am understanding this correctly. I added a check in
> >do_insn_fetch_byte() checking for CS limit. Similar checks in
> >emulate_push() ane emulate_pop() for SS, and checks in
> >x86_decode_insn() for SrcSI and DstDI since they causes accesses to
> >segment override and ES respectively. Are we on the same page?
> 
> You have to do the check wherever you have a read or write that is
> qualified by a segment.  So the best place for them is in
> ->read_emulated(), ->write_emulated(), and similar.
> 
> A good way to do this is to add a segment variable to 'struct
> operand', and doing all the base adjustment at the end (instead of
> up front as we do now).  That means we'll have the minimum number of
> places to add checks to.
->read_emulated(), ->write_emulated() get liner address as a parameter
and know nothing about 'struct operand'. Luckily emulator.c has only one
call for each one of them, so segment checking can be done there just
before call to the functions. 

> 
> 
> >I haven't looked into expand down segments, but I don't think it's
> >much of an effort to add though.
> 
> It's needed, since guests will start failing mysteriously if they
> use those segments and the limits are incorrect (though I doubt
> there are any guests which use expand-down segments).
> 
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Avi Kivity]

On 07/12/2010 05:41 PM, Gleb Natapov wrote:
>
>> A good way to do this is to add a segment variable to 'struct
>> operand', and doing all the base adjustment at the end (instead of
>> up front as we do now).  That means we'll have the minimum number of
>> places to add checks to.
>>      
> ->read_emulated(), ->write_emulated() get liner address as a parameter
> and know nothing about 'struct operand'. Luckily emulator.c has only one
> call for each one of them, so segment checking can be done there just
> before call to the functions.
>
>    

I'd prefer a new helper (with an additional parameter) so that if a new 
call is added, we don't need to change anything.


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

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Kevin O'Connor]

On Mon, Jul 12, 2010 at 04:13:06PM +0300, Avi Kivity wrote:
> Does SeaBIOS use big real mode now?

SeaBIOS calls option roms in big real mode.  This is required by the
relevant specs.

See the transition16big function in src/romlayout.S.  It briefly jumps
to an address at 0xffxxx during the transition to real-mode.  At a
quick glance, it looks like it could probably be changed to not use a
code address >64K.

-Kevin

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Kevin O'Connor]

On Sat, Jul 24, 2010 at 11:45:22AM -0400, Kevin O'Connor wrote:
> On Mon, Jul 12, 2010 at 04:13:06PM +0300, Avi Kivity wrote:
> > Does SeaBIOS use big real mode now?
> 
> SeaBIOS calls option roms in big real mode.  This is required by the
> relevant specs.
> 
> See the transition16big function in src/romlayout.S.  It briefly jumps
> to an address at 0xffxxx during the transition to real-mode.  At a
> quick glance, it looks like it could probably be changed to not use a
> code address >64K.

I put together a SeaBIOS patch so it does not use code addresses >64K
in big real mode - in case anyone wants to test it.  Note, this only
reduces the use of code addresses >64K - SeaBIOS will still try to use
data addresses >64K (eg, in option rom PMM code).

-Kevin


diff --git a/src/misc.c b/src/misc.c
index 354df87..108c332 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -156,8 +156,8 @@ u64 rombios32_gdt[] VAR16VISIBLE __aligned(8) = {
     GDT_LIMIT(BUILD_BIOS_SIZE-1) | GDT_CODE | GDT_BASE(BUILD_BIOS_ADDR),
     // 16 bit data segment base=0x0 limit=0xffff (SEG32_MODE16_DS)
     GDT_LIMIT(0x0ffff) | GDT_DATA,
-    // 16 bit code segment base=0 limit=0xffffffff (SEG32_MODE16BIG_CS)
-    GDT_LIMIT(0xfffff) | GDT_CODE | GDT_G,
+    // 16 bit code segment base=0xf0000 limit=0xffffffff (SEG32_MODE16BIG_CS)
+    GDT_LIMIT(0xfffff) | GDT_CODE | GDT_G | GDT_BASE(BUILD_BIOS_ADDR),
     // 16 bit data segment base=0 limit=0xffffffff (SEG32_MODE16BIG_DS)
     GDT_LIMIT(0xfffff) | GDT_DATA | GDT_G,
 };
diff --git a/src/romlayout.S b/src/romlayout.S
index 54e5a4d..a469596 100644
--- a/src/romlayout.S
+++ b/src/romlayout.S
@@ -105,7 +105,7 @@ transition16big:
         movw %ax, %fs
         movw %ax, %gs
 
-        ljmpl $SEG32_MODE16BIG_CS, $(BUILD_BIOS_ADDR + 1f)
+        ljmpw $SEG32_MODE16BIG_CS, $1f
 
         .code16gcc
 1:

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Avi Kivity]

  On 07/24/2010 06:45 PM, Kevin O'Connor wrote:
> On Mon, Jul 12, 2010 at 04:13:06PM +0300, Avi Kivity wrote:
>> Does SeaBIOS use big real mode now?
> SeaBIOS calls option roms in big real mode.  This is required by the
> relevant specs.

Can you provide a pointer?

> See the transition16big function in src/romlayout.S.  It briefly jumps
> to an address at 0xffxxx during the transition to real-mode.  At a
> quick glance, it looks like it could probably be changed to not use a
> code address>64K.

Yes please.

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

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Avi Kivity]

  On 07/24/2010 07:16 PM, Kevin O'Connor wrote:
> On Sat, Jul 24, 2010 at 11:45:22AM -0400, Kevin O'Connor wrote:
>> On Mon, Jul 12, 2010 at 04:13:06PM +0300, Avi Kivity wrote:
>>> Does SeaBIOS use big real mode now?
>> SeaBIOS calls option roms in big real mode.  This is required by the
>> relevant specs.
>>
>> See the transition16big function in src/romlayout.S.  It briefly jumps
>> to an address at 0xffxxx during the transition to real-mode.  At a
>> quick glance, it looks like it could probably be changed to not use a
>> code address>64K.
> I put together a SeaBIOS patch so it does not use code addresses>64K
> in big real mode - in case anyone wants to test it.  Note, this only
> reduces the use of code addresses>64K - SeaBIOS will still try to use
> data addresses>64K (eg, in option rom PMM code).
>

What conditions are needed to trigger this path?  This can't occur under 
normal operation, since it will fail badly with kvm on Intel.

> -Kevin
>
>
> diff --git a/src/misc.c b/src/misc.c
> index 354df87..108c332 100644
> --- a/src/misc.c
> +++ b/src/misc.c
> @@ -156,8 +156,8 @@ u64 rombios32_gdt[] VAR16VISIBLE __aligned(8) = {
>       GDT_LIMIT(BUILD_BIOS_SIZE-1) | GDT_CODE | GDT_BASE(BUILD_BIOS_ADDR),
>       // 16 bit data segment base=0x0 limit=0xffff (SEG32_MODE16_DS)
>       GDT_LIMIT(0x0ffff) | GDT_DATA,
> -    // 16 bit code segment base=0 limit=0xffffffff (SEG32_MODE16BIG_CS)
> -    GDT_LIMIT(0xfffff) | GDT_CODE | GDT_G,
> +    // 16 bit code segment base=0xf0000 limit=0xffffffff (SEG32_MODE16BIG_CS)
> +    GDT_LIMIT(0xfffff) | GDT_CODE | GDT_G | GDT_BASE(BUILD_BIOS_ADDR),
>       // 16 bit data segment base=0 limit=0xffffffff (SEG32_MODE16BIG_DS)
>       GDT_LIMIT(0xfffff) | GDT_DATA | GDT_G,
>   };
> diff --git a/src/romlayout.S b/src/romlayout.S
> index 54e5a4d..a469596 100644
> --- a/src/romlayout.S
> +++ b/src/romlayout.S
> @@ -105,7 +105,7 @@ transition16big:
>           movw %ax, %fs
>           movw %ax, %gs
>
> -        ljmpl $SEG32_MODE16BIG_CS, $(BUILD_BIOS_ADDR + 1f)
> +        ljmpw $SEG32_MODE16BIG_CS, $1f
>
>           .code16gcc
>   1:
> --
> 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


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

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Kevin O'Connor]

On Sun, Jul 25, 2010 at 11:54:20AM +0300, Avi Kivity wrote:
>  On 07/24/2010 06:45 PM, Kevin O'Connor wrote:
> >On Mon, Jul 12, 2010 at 04:13:06PM +0300, Avi Kivity wrote:
> >>Does SeaBIOS use big real mode now?
> >SeaBIOS calls option roms in big real mode.  This is required by the
> >relevant specs.
> 
> Can you provide a pointer?

See the PMM spec section 2.2 and section 3.2.4.  (Sadly, I can't find
a link to the PMM spec on the web anymore - hopefully you have a
copy.)  Also see the PCI Firmware Specification v3.0 - section
5.2.1.9.

The specs don't require any code addresses to be >64K, but it does
require data access over 64K.  I doubt there are many systems that use
a code address >64K, because an interrupt in big real mode still only
stores a 16bit return address - thus an irq (or nmi) in that mode will
basically cause a crash.

-Kevin

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Kevin O'Connor]

On Sun, Jul 25, 2010 at 11:55:47AM +0300, Avi Kivity wrote:
>  On 07/24/2010 07:16 PM, Kevin O'Connor wrote:
> >On Sat, Jul 24, 2010 at 11:45:22AM -0400, Kevin O'Connor wrote:
> >>On Mon, Jul 12, 2010 at 04:13:06PM +0300, Avi Kivity wrote:
> >>>Does SeaBIOS use big real mode now?
> >>SeaBIOS calls option roms in big real mode.  This is required by the
> >>relevant specs.
> >>
> >>See the transition16big function in src/romlayout.S.  It briefly jumps
> >>to an address at 0xffxxx during the transition to real-mode.  At a
> >>quick glance, it looks like it could probably be changed to not use a
> >>code address>64K.
> >I put together a SeaBIOS patch so it does not use code addresses>64K
> >in big real mode - in case anyone wants to test it.  Note, this only
> >reduces the use of code addresses>64K - SeaBIOS will still try to use
> >data addresses>64K (eg, in option rom PMM code).
> >
> 
> What conditions are needed to trigger this path?  This can't occur
> under normal operation, since it will fail badly with kvm on Intel.

It's called on every boot.  I've personally only tested kvm on amd,
but I'd have to assume something must be allowing this to work on
intel.

On option rom execution (eg, video rom), there is a call to
optionrom.c:__callrom() which calls util.c:call16big() which calls
romlayout.S:__transition16big.  This has been in place since
SeaBIOS-0.4.0 - well before the integration with kvm.

Is the kvm restriction just on the code address, or is it also for
data accesses?

-Kevin

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Kevin O'Connor]

On Sun, Jul 25, 2010 at 12:42:46PM -0400, Kevin O'Connor wrote:
> On Sun, Jul 25, 2010 at 11:55:47AM +0300, Avi Kivity wrote:
> > What conditions are needed to trigger this path?  This can't occur
> > under normal operation, since it will fail badly with kvm on Intel.
> 
> It's called on every boot.  I've personally only tested kvm on amd,
> but I'd have to assume something must be allowing this to work on
> intel.

BTW, the transition16big code does:

        ljmpl $SEG32_MODE16BIG_CS, $(0xf0000 + 1f)

        .code16gcc
1:
        // Disable protected mode
        movl %cr0, %eax
        andl $~CR0_PE, %eax
        movl %eax, %cr0

        // far jump to flush CPU queue after transition to real mode
        ljmpw $0xf000, $2f
2:

Only the ljmpw is in big real mode with a code address >64K - the
"Disable protected mode" code is technically in 16bit protected mode.
I'm not sure if that helps explain why it works.

-Kevin

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Avi Kivity]

  On 07/25/2010 08:19 PM, Kevin O'Connor wrote:
> On Sun, Jul 25, 2010 at 12:42:46PM -0400, Kevin O'Connor wrote:
>> On Sun, Jul 25, 2010 at 11:55:47AM +0300, Avi Kivity wrote:
>>> What conditions are needed to trigger this path?  This can't occur
>>> under normal operation, since it will fail badly with kvm on Intel.
>> It's called on every boot.  I've personally only tested kvm on amd,
>> but I'd have to assume something must be allowing this to work on
>> intel.
> BTW, the transition16big code does:
>
>          ljmpl $SEG32_MODE16BIG_CS, $(0xf0000 + 1f)
>
>          .code16gcc
> 1:
>          // Disable protected mode
>          movl %cr0, %eax
>          andl $~CR0_PE, %eax
>          movl %eax, %cr0
>
>          // far jump to flush CPU queue after transition to real mode
>          ljmpw $0xf000, $2f
> 2:
>
> Only the ljmpw is in big real mode with a code address>64K - the
> "Disable protected mode" code is technically in 16bit protected mode.
> I'm not sure if that helps explain why it works.

What happens is kvm enters real  mode with cs.limit=0xffff, the guest 
#GPs due to segment limit violation, and enters the emulator, which 
emulates the far jump correctly.

So this works, and will continue to work even after we fix limit 
checking.  It's still cleaner IMO to use normal code segments.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Kevin O'Connor]

On Sun, Jul 25, 2010 at 09:34:38PM +0300, Avi Kivity wrote:
>  On 07/25/2010 08:19 PM, Kevin O'Connor wrote:
> >Only the ljmpw is in big real mode with a code address>64K - the
> >"Disable protected mode" code is technically in 16bit protected mode.
> >I'm not sure if that helps explain why it works.
> 
> What happens is kvm enters real  mode with cs.limit=0xffff, the
> guest #GPs due to segment limit violation, and enters the emulator,
> which emulates the far jump correctly.
> 
> So this works, and will continue to work even after we fix limit
> checking.  It's still cleaner IMO to use normal code segments.

Makes sense.  I committed the patch that avoids this behavior to
SeaBIOS git.

-Kevin

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Avi Kivity]

  On 07/25/2010 07:23 PM, Kevin O'Connor wrote:
> On Sun, Jul 25, 2010 at 11:54:20AM +0300, Avi Kivity wrote:
>>   On 07/24/2010 06:45 PM, Kevin O'Connor wrote:
>>> On Mon, Jul 12, 2010 at 04:13:06PM +0300, Avi Kivity wrote:
>>>> Does SeaBIOS use big real mode now?
>>> SeaBIOS calls option roms in big real mode.  This is required by the
>>> relevant specs.
>> Can you provide a pointer?
> See the PMM spec section 2.2 and section 3.2.4.  (Sadly, I can't find
> a link to the PMM spec on the web anymore - hopefully you have a
> copy.)  Also see the PCI Firmware Specification v3.0 - section
> 5.2.1.9.

Unfortunately, I don't have a copy.

> The specs don't require any code addresses to be>64K, but it does
> require data access over 64K.  I doubt there are many systems that use
> a code address>64K, because an interrupt in big real mode still only
> stores a 16bit return address - thus an irq (or nmi) in that mode will
> basically cause a crash.

Ok.  Flat 4G segments happen to accidentally work in kvm-intel, and we 
can keep it working.

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

Re: [RFC PATCH v3 0/3] Add segment limit checks to emulator

[Stefan Hajnoczi]

On Mon, Jul 26, 2010 at 12:47 PM, Avi Kivity <avi@redhat.com> wrote:
>  On 07/25/2010 07:23 PM, Kevin O'Connor wrote:
>>
>> On Sun, Jul 25, 2010 at 11:54:20AM +0300, Avi Kivity wrote:
>>>
>>>  On 07/24/2010 06:45 PM, Kevin O'Connor wrote:
>>>>
>>>> On Mon, Jul 12, 2010 at 04:13:06PM +0300, Avi Kivity wrote:
>>>>>
>>>>> Does SeaBIOS use big real mode now?
>>>>
>>>> SeaBIOS calls option roms in big real mode.  This is required by the
>>>> relevant specs.
>>>
>>> Can you provide a pointer?
>>
>> See the PMM spec section 2.2 and section 3.2.4.  (Sadly, I can't find
>> a link to the PMM spec on the web anymore - hopefully you have a
>> copy.)  Also see the PCI Firmware Specification v3.0 - section
>> 5.2.1.9.
>
> Unfortunately, I don't have a copy.

There were some links here but they don't work anymore.  The file name
is specspmm101.pdf:

http://lists.xensource.com/archives/html/xen-changelog/2009-01/msg00153.html

>> The specs don't require any code addresses to be>64K, but it does
>> require data access over 64K.  I doubt there are many systems that use
>> a code address>64K, because an interrupt in big real mode still only
>> stores a 16bit return address - thus an irq (or nmi) in that mode will
>> basically cause a crash.
>
> Ok.  Flat 4G segments happen to accidentally work in kvm-intel, and we can
> keep it working.
>
> --
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>