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

2015-11-05 Thread Eduardo Habkost 
On Thu, Nov 05, 2015 at 08:51:24AM +0100, Richard Henderson wrote:
> 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 
> >>>---
> >>>  target-i386/cpu.c | 8 +---
> >>>  target-i386/cpu.h | 3 +++
> >>>  2 files changed, 8 insertions(+), 3 deletions(-)
> >>
> >>Reviewed-by: Richard Henderson 
> >>
> >>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.

I will apply this patch without the TCG_*_FEATURES changes until we
change TCG, then. That's OK?

About the TCG patches I have sent, please let me know if they look good
and appropriate for 2.5. This is the first time I have touched TCG code.

-- 
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-11-04 Thread Richard Henderson 

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 
---
  target-i386/cpu.c | 8 +---
  target-i386/cpu.h | 3 +++
  2 files changed, 8 insertions(+), 3 deletions(-)


Reviewed-by: Richard Henderson 

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~

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-11-04 Thread Eduardo Habkost 
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 
> > ---
> >  target-i386/cpu.c | 8 +---
> >  target-i386/cpu.h | 3 +++
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Richard Henderson 
> 
> 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. Your conclusion seems to be
right for pcommit and clflushopt, if I checked the opcodes and encoding
properly. In the case of pcommit (/7, modrm == 0xf8), we check for SSE;
in the case of clflushopt (/7 with a memory operand, modrm != 0xf8), we
check for CLFLUSH.

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.

-- 
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-10-30 Thread Richard Henderson 
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 
> ---
>  target-i386/cpu.c | 8 +---
>  target-i386/cpu.h | 3 +++
>  2 files changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson 

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?


r~
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html