[BUG?] can not writeback more then 8 bytes memory

[BUG?] can not writeback more then 8 bytes memory

[Wei Yongjun]

Hi all:

I have the following patch to SIDT emulation instruction, but it
does not work because we can not writeback more then 8 bytes
memory, the SIDT under PROTO64 is 10 bytes.

I change the code to write twice, first time write limit, then write
address, and it does not work too. If I change the c->dst.bytes
to only writeback 8 bytes, it will writeback 8 bytes.

Is this a bug of KVM or it is the IO limit?

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index dc3a648..43a6819 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2213,7 +2213,8 @@ static struct opcode group5[] = {
 };
 
 static struct group_dual group7 = { {
-	N, N, D(ModRM | SrcMem | Priv), D(ModRM | SrcMem | Priv),
+	N, D(DstMem | SrcNone | ModRM | Op3264),
+	D(ModRM | SrcMem | Priv), D(ModRM | SrcMem | Priv),
 	D(SrcNone | ModRM | DstMem | Mov), N,
 	D(SrcMem16 | ModRM | Mov | Priv),
 	D(SrcMem | ModRM | ByteOp | Priv | NoAccess),
@@ -3282,6 +3283,7 @@ twobyte_insn:
 		switch (c->modrm_reg) {
 			u16 size;
 			unsigned long address;
+			struct desc_ptr dt;
 
 		case 0: /* vmcall */
 			if (c->modrm_mod != 3 || c->modrm_rm != 1)
@@ -3296,6 +3298,11 @@ twobyte_insn:
 			/* Disable writeback. */
 			c->dst.type = OP_NONE;
 			break;
+		case 1: /* sidt */
+			ops->get_idt(&dt, ctxt->vcpu);
+			c->dst.bytes = c->op_bytes + 2;
+			memcpy(&c->dst.valptr, &dt, c->dst.bytes);
+			break;
 		case 2: /* lgdt */
 			rc = read_descriptor(ctxt, ops, c->src.addr.mem,
 					     &size, &address, c->op_bytes);
-- 
1.7.0.4

Re: [BUG?] can not writeback more then 8 bytes memory

[Avi Kivity]

 On 08/20/2010 12:12 PM, Wei Yongjun wrote:
> Hi all:
>
> I have the following patch to SIDT emulation instruction, but it
> does not work because we can not writeback more then 8 bytes
> memory, the SIDT under PROTO64 is 10 bytes.
>
> I change the code to write twice, first time write limit, then write
> address, and it does not work too. If I change the c->dst.bytes
> to only writeback 8 bytes, it will writeback 8 bytes.
>
> Is this a bug of KVM or it is the IO limit?

It's complicated... if you write to real memory, then you can do

c->dst.* = ...;
writeback();
c->dst.* = ...;
writeback();

However that will not work for mmio, since writeback() only schedules
mmio writes which are retired later on.

As a temporary fix we can extend writeback() to support 2+N byte writes
(and fail them on mmio). See also the sse-mmio branch on kvm.git on how
to support >8 byte mmio.

Longer term I'd like a queue based approach so we can have any number of
reads and writes for an instruction, and have multiple writes possible
for mmio. It would look something like this:

- every read or write is assigned a position in the queue starting from 0
- the queue is maintained across invocations of x86_emulate_insn()
- if an operation position is already in the queue, we read the value
from the queue, and ignore writes (e.g. a replay after exit to userspace)
- if an operation position is not already in the queue, we add it. If we
cannot satisfy it, we exit to userspace
- need to check read-after-write for collision in the queue.

There's some beginning of that already (see struct read_cache).

For now we can live without SIDT to mmio, that means you can do the
simple fix to writeback() above. But you will only be able to test is
with realmode.flat, not with emulator.flat since that requires mmio.

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

Re: [BUG?] can not writeback more then 8 bytes memory

[Wei Yongjun]

>  On 08/20/2010 12:12 PM, Wei Yongjun wrote:
>   
>> Hi all:
>>
>> I have the following patch to SIDT emulation instruction, but it
>> does not work because we can not writeback more then 8 bytes
>> memory, the SIDT under PROTO64 is 10 bytes.
>>
>> I change the code to write twice, first time write limit, then write
>> address, and it does not work too. If I change the c->dst.bytes
>> to only writeback 8 bytes, it will writeback 8 bytes.
>>
>> Is this a bug of KVM or it is the IO limit?
>>     
> It's complicated... if you write to real memory, then you can do
>
> c->dst.* = ...;
> writeback();
> c->dst.* = ...;
> writeback();
>
> However that will not work for mmio, since writeback() only schedules
> mmio writes which are retired later on.
>
> As a temporary fix we can extend writeback() to support 2+N byte writes
> (and fail them on mmio). See also the sse-mmio branch on kvm.git on how
> to support >8 byte mmio.
>
> Longer term I'd like a queue based approach so we can have any number of
> reads and writes for an instruction, and have multiple writes possible
> for mmio. It would look something like this:
>
> - every read or write is assigned a position in the queue starting from 0
> - the queue is maintained across invocations of x86_emulate_insn()
> - if an operation position is already in the queue, we read the value
> from the queue, and ignore writes (e.g. a replay after exit to userspace)
> - if an operation position is not already in the queue, we add it. If we
> cannot satisfy it, we exit to userspace
> - need to check read-after-write for collision in the queue.
>
> There's some beginning of that already (see struct read_cache).
>
> For now we can live without SIDT to mmio, that means you can do the
> simple fix to writeback() above. But you will only be able to test is
> with realmode.flat, not with emulator.flat since that requires mmio.
>   

If test with realmode.flat, fix to writeback() does not need,
because the size we need to writeback is 2 + 4 in realmode.flat.
I tested used the following patch.

diff --git a/x86/realmode.c b/x86/realmode.c
index 8c771fc..26651a6 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -1242,6 +1242,25 @@ void test_lds_lss()
 		outregs.ebx == desc.sel);
 }
 
+
+static struct {
+	u16 limit;
+	unsigned long base;
+} __attribute__((packed)) idt_descr = {
+	0x1234,
+	0x5678,
+};
+
+void test_sidt()
+{	
+	MK_INSN(sidt, "sidt (%eax)\n\t");
+
+	inregs = (struct regs){ .eax = (unsigned long)&idt_descr };
+
+	exec_in_big_real_mode(&insn_sidt);
+	report("sidt", 0, idt_descr.limit == 1023 && idt_descr.base == 0);
+}
+
 void realmode_start(void)
 {
 	test_null();
@@ -1274,6 +1293,7 @@ void realmode_start(void)
 	test_cwd_cdq();
 	test_das();
 	test_lds_lss();
+	test_sidt();
 
 	exit(0);
 }

Re: [BUG?] can not writeback more then 8 bytes memory

[Avi Kivity]

 On 08/23/2010 11:55 AM, Wei Yongjun wrote:
>
> If test with realmode.flat, fix to writeback() does not need,
> because the size we need to writeback is 2 + 4 in realmode.flat.
> I tested used the following patch.

It will fail in 64-bit mode though.

We can goto cannot_emulate, though it's a bit nasty.

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