Re: [Qemu-devel] [PATCH] target-i386: enable cflushopt/clwb/pcommit instructions

[Richard Henderson <rth@twiddle.net>]

On 11/04/2015 08:35 PM, Eduardo Habkost wrote:
> On Fri, Oct 30, 2015 at 01:54:33PM -0700, Richard Henderson wrote:
>> On 10/29/2015 12:31 AM, Xiao Guangrong wrote:
>>> These instructions are used by NVDIMM drivers and the specification
>>> locates at:
>>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
>>>
>>> There instructions are available on Skylake Server
>>>
>>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>> ---
>>>   target-i386/cpu.c | 8 +++++---
>>>   target-i386/cpu.h | 3 +++
>>>   2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>>
>> Although it would be nice to update the comments in translate.c to include the
>> new insns, since they overlap mfence and sfence.  At present we only check for
>> SSE enabled when accepting these; I suppose it's easiest to consider it invalid
>> to specify +clwb,-sse?
>
> I assume you want to add the extra SSE requirement to TCG code, not to
> generic x86 code, then I have no objections.

I don't really want to add any requirement, just point and laugh at anyone who 
reports an bug for the above condition.

> But in the case of clwb (/6 with a memory operand, modrm != 0xc0), we
> are not just requiring SSE2: we are rejecting the instruction unless
> modrm == 0xc0. That means TCG is rejecting the clwb instruction, so I
> believe we shouldn't add CLWB to TCG_7_0_EBX_FEATURES yet.

Hmm, yes.

I've cleaned up some of this code on a branch, but it didn't get enough testing 
or review this cycle, so it's going to wait for the next.  I see you've posted 
a patch for this, which should be good enough until then.


r~