On Mon, Apr 22, 2013 at 11:38:10AM +0200, Borislav Petkov wrote:
> On Mon, Apr 22, 2013 at 10:53:42AM +0200, Paolo Bonzini wrote:
> > Il 21/04/2013 14:23, Borislav Petkov ha scritto:
> > > On Sun, Apr 21, 2013 at 01:46:50PM +0200, Borislav Petkov wrote:
> > >> We probably need something with copying values to a temp variable or so.
> > >
> > > Basically something like that:
> > >
> > > case 2:
> > > /*
> > > * From MOVBE definition: "...When the operand size is 16 bits,
> > > * the upper word of the destination register remains unchanged
> > > * ..."
> > > *
> > > * Both casting ->valptr and ->val to u16 breaks strict aliasing
> > > * rules so we have to do the operation almost per hand.
> > > */
> > > tmp = (u16)ctxt->src.val;
> > > ctxt->dst.val &= ~0xffffUL;
> > > ctxt->dst.val |= (unsigned long)swab16(tmp);
> > > break;
> > >
> > > This passes all gcc checks, even the stricter ones when building with W=3.
> >
> > I thought the valptr one was ok.
>
> Yep, it looked like that too. And, it could actually really be ok and
> the gcc's warning here is bogus. I'll try to talk to gcc people about
> it.
>
> > I find this one more readable, too. How does the generated code look
> > like?
>
> Well, so so:
>
> movzwl 112(%rdi), %eax # ctxt_5(D)->src.D.27823.val, tmp87
> movq 240(%rdi), %rdx # ctxt_5(D)->dst.D.27823.val, tmp89
> xorw %dx, %dx # tmp89
> rolw $8, %ax #, tmp87
> movzwl %ax, %eax # tmp87, tmp91
>
> I have hard time understanding why it is adding this insn here - it can
> simply drop it and continue with the 64-bit OR. It's not like it changes
> anything...
>
> orq %rdx, %rax # tmp89, tmp91
> movq %rax, 240(%rdi) # tmp91, ctxt_5(D)->dst.D.27823.val
>
> Btw, I wanted to ask: when kvm commits the results, does it look at
> ctxt->op_bytes to know exactly how many bytes to write to the guest?
> Because if it does, we can save ourselves the trouble here.
>
> Or does it simply write both the full sizeof(unsigned long) bytes of
> ->src.val and ->dst.val to the guest?
>
No, it does this in case of register operand:
static void write_register_operand(struct operand *op)
{
/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
switch (op->bytes) {
case 1:
*(u8 *)op->addr.reg = (u8)op->val;
break;
case 2:
*(u16 *)op->addr.reg = (u16)op->val;
break;
case 4:
*op->addr.reg = (u32)op->val;
break; /* 64b: zero-extend */
case 8:
*op->addr.reg = op->val;
break;
}
}
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
