Re: [PATCH v4 2/5] x86/asm: add enqcmds() to support ENQCMDS instruction

From: Borislav Petkov <bp@alien8.de>
To: Dave Jiang <dave.jiang@intel.com>
Cc: vkoul@kernel.org, tglx@linutronix.de, mingo@redhat.com,
	dan.j.williams@intel.com, tony.luck@intel.com,
	jing.lin@intel.com, ashok.raj@intel.com,
	sanjay.k.kumar@intel.com, fenghua.yu@intel.com,
	kevin.tian@intel.com, dmaengine@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/5] x86/asm: add enqcmds() to support ENQCMDS instruction
Date: Wed, 23 Sep 2020 13:08:37 +0200	[thread overview]
Message-ID: <20200923110837.GH28545@zn.tnic> (raw)
In-Reply-To: <160037732334.3777.8083106831110728138.stgit@djiang5-desk3.ch.intel.com>

On Thu, Sep 17, 2020 at 02:15:23PM -0700, Dave Jiang wrote:
> Add enqcmds() in x86 io.h instead of special_insns.h.

Why? It is an asm wrapper for a special instruction.

> MOVDIR64B
> instruction can be used for other purposes. A wrapper was introduced
> in io.h for its command submission usage. ENQCMDS has a single
> purpose of submit 64-byte commands to supported devices and should
> be called directly.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/include/asm/io.h |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index d726459d08e5..b7af0bf8a018 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -424,4 +424,33 @@ static inline void iosubmit_cmds512(void __iomem *dst, const void *src,
>  	}
>  }
>  
> +/**
> + * enqcmds - copy a 512 bits data unit to single MMIO location

Your #319433 doc says

"ENQCMDS — Enqueue Command Supervisor"

Now *how* that enqueueing is done you can explain in the comment below.

> + * @dst: destination, in MMIO space (must be 512-bit aligned)
> + * @src: source
> + *
> + * Submit data from kernel space to MMIO space, in a unit of 512 bits.
> + * Order of data access is not guaranteed, nor is a memory barrier
> + * performed afterwards. The command returns false (0) on failure, and true (1)
> + * on success.

The command or the function?

From what I see below, the instruction sets ZF=1 to denote that it needs
to be retried and ZF=0 means success, as the doc says. And in good UNIX
tradition, 0 means usually success and !0 failure.

So why are you flipping that?

> + * Warning: Do not use this helper unless your driver has checked that the CPU
> + * instruction is supported on the platform.
> + */
> +static inline bool enqcmds(void __iomem *dst, const void *src)
> +{
> +	bool retry;
> +
> +	/* ENQCMDS [rdx], rax */
> +	asm volatile(".byte 0xf3, 0x0f, 0x38, 0xf8, 0x02, 0x66, 0x90\t\n"
								    ^^^^
No need for those last two chars.

> +		     CC_SET(z)
> +		     : CC_OUT(z) (retry)
> +		     : "a" (dst), "d" (src));

<---- newline here.

> +	/* Submission failure is indicated via EFLAGS.ZF=1 */
> +	if (retry)
> +		return false;
> +
> +	return true;
> +}
> +
>  #endif /* _ASM_X86_IO_H */

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette