Re: [PATCH v3 2/3] x86/emulate: add support of emulating SSE2 instruction {, v}movd mm, r32/m32 and {, v}movq mm, r64

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: "Jan Beulich" <JBeulich@suse.com>,
	"Mihai Donțu" <mdontu@bitdefender.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>, xen-devel@lists.xen.org
Subject: Re: [PATCH v3 2/3] x86/emulate: add support of emulating SSE2 instruction {, v}movd mm, r32/m32 and {, v}movq mm, r64
Date: Mon, 1 Aug 2016 16:01:02 +0100	[thread overview]
Message-ID: <a1a477ed-0954-552f-d0d3-296c908269a1@citrix.com> (raw)
In-Reply-To: <579F7FEA0200007800101615@prv-mh.provo.novell.com>

On 01/08/16 15:59, Jan Beulich wrote:
>>>> On 01.08.16 at 16:55, <mdontu@bitdefender.com> wrote:
>> On Monday 01 August 2016 17:48:33 Mihai Donțu wrote:
>>> On Monday 01 August 2016 07:43:20 Jan Beulich wrote:
>>>>>>> Your suggestion makes sense, but I'm starting to doubt my initial
>>>>>>> patch. :-) I'm testing "movq xmm1, xmm1" and noticing that it takes the
>>>>>>> GPR-handling route and I can't seem to be able to easily prevent it
>>>>>>> with !(rex_prefix & REX_B), as rex_prefix == 0 and vex.b == 1. I need
>>>>>>> to take a harder look at how that class of instructions is coded.      
>>>>>> You obviously need to distinguish the two kinds of register sources/
>>>>>> destinations: GPRs need suitable re-writing of the instruction (without
>>>>>> having looked at the most recent version of the patch yet I btw doubt
>>>>>> converting register to memory operands is the most efficient model),
>>>>>> while MMs, XMMs, and YMMs can retain their register encoding.    
>>>>> Regarding efficiency, I'm not married with the approach I've proposed.
>>>>> If you can give me a few more hints, I can give it a try.    
>>>> I'd rather pick a fixed register and update the regs->... field from that
>>>> after the stub was executed. E.g. using rAX and treating it just like a
>>>> return value of the "call". But maybe I'm imagining this easier than it
>>>> really is; as an alternative I'd then suggest really following what Andrew
>>>> said - use a pointer into regs->, not mmvalp. But (as said in the review
>>>> mail) you'd then have the problem of the missing zero-extension for
>>>> writes to 32-bit GPRs  
>>> I thought that by re-using (hijacking, really) mmvalp, the patch will
>>> look less intrusive and thus not add too much to an already complex
>>> code.
>>>
>>> Assuming I'll just pass to the stub "a"(ea.reg), would it be a good
>>> idea to just zero-out the 64bit register before that? It does not
>>> appear to be any instructions that write just the low dword. Or am I
>>> misunderstanding the zero-extension concept?
>> Just to be sure I'm making myself understood, ea.reg contains the
>> output of decode_register() which, in turn, returns a pointer in regs.
> Hmm, now that you say that maybe I got confused be the expression
> you used: If, rather than doing a cast on the address of mmvalp, you
> could get away with a simply assignment (and maybe a cast on the
> rvalue), then I suppose your code might not be as wrong as it seemed
> to me.

I had to spend quite a long time convincing myself that it was right.

The code would be easier if mmvalp was a plain void* rather than const
mmval_t *.

~Andrew

>
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel