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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <10b6c333-b252-eb9c-db82-91a93232e1a0@intel.com>
Date:   Wed, 23 Sep 2020 08:47:31 -0700
From:   Dave Jiang <dave.jiang@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     vkoul@...nel.org, tglx@...utronix.de, mingo@...hat.com,
        dan.j.williams@...el.com, tony.luck@...el.com, jing.lin@...el.com,
        ashok.raj@...el.com, sanjay.k.kumar@...el.com,
        fenghua.yu@...el.com, kevin.tian@...el.com,
        dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/5] x86/asm: add enqcmds() to support ENQCMDS
 instruction



On 9/23/2020 4:08 AM, Borislav Petkov wrote:
> 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.

Ok will move.

> 
>> 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@...el.com>
>> Reviewed-by: Tony Luck <tony.luck@...el.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.

Ok will add.

> 
>> + * @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?

Function. Will fix.
> 
>  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?

Ok will return 0 for success and -ERETRY for failure.

> 
>> + * 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.

Ok will remove.

> 
>> +		     CC_SET(z)
>> +		     : CC_OUT(z) (retry)
>> +		     : "a" (dst), "d" (src));
> 
> <---- newline here.

Will fix.

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

Thank you very much for reviewing Boris. Very much appreciated!