Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Alexey Dobriyan]

> +static inline void movdir64b(void *dst, const void *src)
> +{
> +	/* movdir64b [rdx], rax */
> +	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> +			: "=m" (*(char *)dst)
                               ^^^^^^^^^^

> +			: "d" (src), "a" (dst));
> +}

Probably needs fake 64-byte type, so that compiler knows what is dirty.

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Luck, Tony]

On Thu, Aug 01, 2019 at 10:43:48PM +0300, Alexey Dobriyan wrote:
> > +static inline void movdir64b(void *dst, const void *src)
> > +{
> > +	/* movdir64b [rdx], rax */
> > +	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > +			: "=m" (*(char *)dst)
>                                ^^^^^^^^^^
> 
> > +			: "d" (src), "a" (dst));
> > +}
> 
> Probably needs fake 64-byte type, so that compiler knows what is dirty.

Would that be something like this?

static inline void movdir64b(void *dst, const void *src)
{
	struct dstbytes {
		char pad[64];
	};

	/* movdir64b [rdx], rax */
	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
		     : "=m" (*(struct dstbytes *)dst)
		     : "d" (src), "a" (dst));
}

Or did you have something else in mind?

-Tony

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Kirill A. Shutemov]

On Thu, Aug 01, 2019 at 12:49:48PM -0700, Luck, Tony wrote:
> On Thu, Aug 01, 2019 at 10:43:48PM +0300, Alexey Dobriyan wrote:
> > > +static inline void movdir64b(void *dst, const void *src)
> > > +{
> > > +	/* movdir64b [rdx], rax */
> > > +	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > > +			: "=m" (*(char *)dst)
> >                                ^^^^^^^^^^
> > 
> > > +			: "d" (src), "a" (dst));
> > > +}
> > 
> > Probably needs fake 64-byte type, so that compiler knows what is dirty.
> 
> Would that be something like this?
> 
> static inline void movdir64b(void *dst, const void *src)
> {
> 	struct dstbytes {
> 		char pad[64];
> 	};
> 
> 	/* movdir64b [rdx], rax */
> 	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> 		     : "=m" (*(struct dstbytes *)dst)
> 		     : "d" (src), "a" (dst));
> }
> 
> Or did you have something else in mind?

Or should we add "memory" clobber instead, like we do for string
operations?

static inline void movdir64b(void *dst, const void *src)
{
	/* movdir64b [rdx], rax */
	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
			: : "d" (src), "a" (dst) : "memory");
}

-- 
 Kirill A. Shutemov

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Borislav Petkov]

On Thu, Aug 01, 2019 at 11:28:08PM +0300, Kirill A. Shutemov wrote:
> On Thu, Aug 01, 2019 at 12:49:48PM -0700, Luck, Tony wrote:
> > On Thu, Aug 01, 2019 at 10:43:48PM +0300, Alexey Dobriyan wrote:
> > > > +static inline void movdir64b(void *dst, const void *src)
> > > > +{
> > > > +	/* movdir64b [rdx], rax */
> > > > +	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > > > +			: "=m" (*(char *)dst)
> > >                                ^^^^^^^^^^
> > > 
> > > > +			: "d" (src), "a" (dst));
> > > > +}
> > > 
> > > Probably needs fake 64-byte type, so that compiler knows what is dirty.
> > 
> > Would that be something like this?
> > 
> > static inline void movdir64b(void *dst, const void *src)
> > {
> > 	struct dstbytes {
> > 		char pad[64];
> > 	};
> > 
> > 	/* movdir64b [rdx], rax */
> > 	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > 		     : "=m" (*(struct dstbytes *)dst)
> > 		     : "d" (src), "a" (dst));
> > }
> > 
> > Or did you have something else in mind?
> 
> Or should we add "memory" clobber instead, like we do for string
> operations?

I think Tony's in the right direction. We already do dst "sizing" like
that for the compiler in clwb().

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Alexey Dobriyan]

On Thu, Aug 01, 2019 at 12:49:48PM -0700, Luck, Tony wrote:
> On Thu, Aug 01, 2019 at 10:43:48PM +0300, Alexey Dobriyan wrote:
> > > +static inline void movdir64b(void *dst, const void *src)
> > > +{
> > > +	/* movdir64b [rdx], rax */
> > > +	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > > +			: "=m" (*(char *)dst)
> >                                ^^^^^^^^^^
> > 
> > > +			: "d" (src), "a" (dst));
> > > +}
> > 
> > Probably needs fake 64-byte type, so that compiler knows what is dirty.
> 
> Would that be something like this?
> 
> static inline void movdir64b(void *dst, const void *src)
> {
> 	struct dstbytes {
> 		char pad[64];
> 	};
> 
> 	/* movdir64b [rdx], rax */
> 	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> 		     : "=m" (*(struct dstbytes *)dst)
> 		     : "d" (src), "a" (dst));
> }
> 
> Or did you have something else in mind?

Well, it doesn't need a name:

	: "=m" (*(struct{char _[64];}*)dst)

But yes, something like that.

Can't cast to "char[64]" :-(

RE: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Luck, Tony]

> I think Tony's in the right direction. We already do dst "sizing" like
> that for the compiler in clwb().

The clwb case does look like what we want for movdir64b().

But is it right for clwb() ... that doesn't modify anything, just pushes
things from cache to memory. So why is it using "+m"?

-Tony

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 12:49:48PM -0700, Luck, Tony wrote:
> On Thu, Aug 01, 2019 at 10:43:48PM +0300, Alexey Dobriyan wrote:
> > > +static inline void movdir64b(void *dst, const void *src)
> > > +{
> > > +	/* movdir64b [rdx], rax */
> > > +	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > > +			: "=m" (*(char *)dst)
> >                                ^^^^^^^^^^
> > 
> > > +			: "d" (src), "a" (dst));
> > > +}
> > 
> > Probably needs fake 64-byte type, so that compiler knows what is dirty.
> 
> Would that be something like this?
> 
> static inline void movdir64b(void *dst, const void *src)
> {
> 	struct dstbytes {
> 		char pad[64];
> 	};
> 
> 	/* movdir64b [rdx], rax */
> 	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> 		     : "=m" (*(struct dstbytes *)dst)
> 		     : "d" (src), "a" (dst));
> }

Can the source and destination overlap? The SDM doesn't seem to mention
this.

Also, it bugs me something fierce that this provides a
single-copy-atomic 64b store, but there is no matching load operation.

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Kirill A. Shutemov]

On Fri, Aug 02, 2019 at 10:15:33AM +0200, Peter Zijlstra wrote:
> On Thu, Aug 01, 2019 at 12:49:48PM -0700, Luck, Tony wrote:
> > On Thu, Aug 01, 2019 at 10:43:48PM +0300, Alexey Dobriyan wrote:
> > > > +static inline void movdir64b(void *dst, const void *src)
> > > > +{
> > > > +	/* movdir64b [rdx], rax */
> > > > +	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > > > +			: "=m" (*(char *)dst)
> > >                                ^^^^^^^^^^
> > > 
> > > > +			: "d" (src), "a" (dst));
> > > > +}
> > > 
> > > Probably needs fake 64-byte type, so that compiler knows what is dirty.
> > 
> > Would that be something like this?
> > 
> > static inline void movdir64b(void *dst, const void *src)
> > {
> > 	struct dstbytes {
> > 		char pad[64];
> > 	};
> > 
> > 	/* movdir64b [rdx], rax */
> > 	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > 		     : "=m" (*(struct dstbytes *)dst)
> > 		     : "d" (src), "a" (dst));
> > }
> 
> Can the source and destination overlap? The SDM doesn't seem to mention
> this.

Good question. I'll ask to clarify this.

-- 
 Kirill A. Shutemov

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Borislav Petkov]

On Thu, Aug 01, 2019 at 10:06:27PM +0000, Luck, Tony wrote:
> > I think Tony's in the right direction. We already do dst "sizing" like
> > that for the compiler in clwb().
> 
> The clwb case does look like what we want for movdir64b().
> 
> But is it right for clwb() ... that doesn't modify anything, just pushes
> things from cache to memory. So why is it using "+m"?

Here some hints from to my notes, if you want to know more detail, I can
ping my gcc guy.

It needs to be an input and an output operand so that it prevents gcc
from reordering accesses to it after the insn happens, i.e., you don't
want to touch it after CLFLUSH has executed.

And also, you want to make sure it works with all gcc versions and this
is, I was told, the right way to do it. For example, some gcc versions
consider it not limited to 64 bytes of memory being touched but a full
memory clobber.

HTH.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Lin, Jing]

+Dave, who is the DSA developer. 

Thanks,
Jing 

-----Original Message-----
From: Borislav Petkov <bp@alien8.de> 
Sent: Friday, August 2, 2019 7:41 AM
To: Luck, Tony <tony.luck@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>; Alexey Dobriyan <adobriyan@gmail.com>; kirill.shutemov@linux.intel.com; linux-kernel@vger.kernel.org; Lin, Jing <jing.lin@intel.com>; x86@kernel.org
Subject: Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

On Thu, Aug 01, 2019 at 10:06:27PM +0000, Luck, Tony wrote:
> > I think Tony's in the right direction. We already do dst "sizing" 
> > like that for the compiler in clwb().
> 
> The clwb case does look like what we want for movdir64b().
> 
> But is it right for clwb() ... that doesn't modify anything, just 
> pushes things from cache to memory. So why is it using "+m"?

Here some hints from to my notes, if you want to know more detail, I can ping my gcc guy.

It needs to be an input and an output operand so that it prevents gcc from reordering accesses to it after the insn happens, i.e., you don't want to touch it after CLFLUSH has executed.

And also, you want to make sure it works with all gcc versions and this is, I was told, the right way to do it. For example, some gcc versions consider it not limited to 64 bytes of memory being touched but a full memory clobber.

HTH.

--
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Borislav Petkov]

On Thu, Aug 01, 2019 at 12:20:30PM -0700, Luck, Tony wrote:
> Just to get another of the non-controversial bits out of the
> way before the main course arrives.

Let's submit functions together with their respective users pls. Like we
always do.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Luck, Tony]

On Thu, Aug 01, 2019 at 12:03:41PM +0200, Borislav Petkov wrote:
> On Wed, Jul 31, 2019 at 02:05:54AM +0300, Kirill A. Shutemov wrote:
> > Several upcoming patchsets will make use of the helper.
> 
> ... so why aren't you sending it together with its first user?

Just to get another of the non-controversial bits out of the
way before the main course arrives.

Note that the CPUFEATURE bit for MOVDIR64B already went upstream
in v4.20:

 ace6485a0326 ("x86/cpufeatures: Enumerate MOVDIR64B instruction")

-Tony

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Kirill A. Shutemov]

On Thu, Aug 01, 2019 at 10:03:41AM +0000, Borislav Petkov wrote:
> On Wed, Jul 31, 2019 at 02:05:54AM +0300, Kirill A. Shutemov wrote:
> > Add support for a new instruction MOVDIR64B. The instruction moves
> > 64-bytes as direct-store with 64-byte write atomicity from source memory
> > address to destination memory address.
> > 
> > MOVDIR64B requires the destination address to be 64-byte aligned. No
> > alignment restriction is enforced for source operand.
> > 
> > See Intel Software Developer’s Manual for more information on the
> > instruction.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> > 
> > Several upcoming patchsets will make use of the helper.
> 
> ... so why aren't you sending it together with its first user?

We are not yet sure which patchset will hit upstream first. I thought it
would be logistically easier to get the patch upstream on its own.

But if you prefer the patch to be submitted along with the first user, we
can definately do this.

Jing, are you okay with this?

-- 
 Kirill A. Shutemov

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[Borislav Petkov]

On Wed, Jul 31, 2019 at 02:05:54AM +0300, Kirill A. Shutemov wrote:
> Add support for a new instruction MOVDIR64B. The instruction moves
> 64-bytes as direct-store with 64-byte write atomicity from source memory
> address to destination memory address.
> 
> MOVDIR64B requires the destination address to be 64-byte aligned. No
> alignment restriction is enforced for source operand.
> 
> See Intel Software Developer’s Manual for more information on the
> instruction.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
> 
> Several upcoming patchsets will make use of the helper.

... so why aren't you sending it together with its first user?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86/asm: Add support for MOVDIR64B instruction

[jinglin]

On 7/30/19 4:05 PM, Kirill A. Shutemov wrote:
> Add support for a new instruction MOVDIR64B. The instruction moves
> 64-bytes as direct-store with 64-byte write atomicity from source memory
> address to destination memory address.
>
> MOVDIR64B requires the destination address to be 64-byte aligned. No
> alignment restriction is enforced for source operand.
>
> See Intel Software Developer’s Manual for more information on the
> instruction.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reviewed-by: Jing Lin <jing.lin@intel.com>
> ---
>
> Several upcoming patchsets will make use of the helper.
>
> ---
>   arch/x86/include/asm/special_insns.h | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 219be88a59d2..059e7bd331d2 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -248,6 +248,13 @@ static inline void clwb(volatile void *__p)
>   
>   #define nop() asm volatile ("nop")
>   
> +static inline void movdir64b(void *dst, const void *src)
> +{
> +	/* movdir64b [rdx], rax */
> +	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> +			: "=m" (*(char *)dst)
> +			: "d" (src), "a" (dst));
> +}
>   
>   #endif /* __KERNEL__ */
>   

[PATCH] x86/asm: Add support for MOVDIR64B instruction

[Kirill A. Shutemov]

Add support for a new instruction MOVDIR64B. The instruction moves
64-bytes as direct-store with 64-byte write atomicity from source memory
address to destination memory address.

MOVDIR64B requires the destination address to be 64-byte aligned. No
alignment restriction is enforced for source operand.

See Intel Software Developer’s Manual for more information on the
instruction.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---

Several upcoming patchsets will make use of the helper.

---
 arch/x86/include/asm/special_insns.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 219be88a59d2..059e7bd331d2 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -248,6 +248,13 @@ static inline void clwb(volatile void *__p)
 
 #define nop() asm volatile ("nop")
 
+static inline void movdir64b(void *dst, const void *src)
+{
+	/* movdir64b [rdx], rax */
+	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
+			: "=m" (*(char *)dst)
+			: "d" (src), "a" (dst));
+}
 
 #endif /* __KERNEL__ */
 
-- 
2.21.0