linux-kernel - Re: [PATCH RFC 01/14] x86/asm: add iosubmit_cmds512() based on movdir64b CPU 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: <20191120215338.GN2634@zn.tnic>
Date:   Wed, 20 Nov 2019 22:53:39 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Dave Jiang <dave.jiang@...el.com>
Cc:     dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
        vkoul@...nel.org, dan.j.williams@...el.com, tony.luck@...el.com,
        jing.lin@...el.com, ashok.raj@...el.com, sanjay.k.kumar@...el.com,
        megha.dey@...el.com, jacob.jun.pan@...el.com, yi.l.liu@...el.com,
        axboe@...nel.dk, akpm@...ux-foundation.org, tglx@...utronix.de,
        mingo@...hat.com, fenghua.yu@...el.com, hpa@...or.com
Subject: Re: [PATCH RFC 01/14] x86/asm: add iosubmit_cmds512() based on
 movdir64b CPU instruction

On Wed, Nov 20, 2019 at 02:23:49PM -0700, Dave Jiang wrote:
> +/**
> + * iosubmit_cmds512 - copy data to single MMIO location, in 512-bit units

Where is the alignment check on that data before doing the copying?

> + * @dst: destination, in MMIO space (must be 512-bit aligned)
> + * @src: source
> + * @count: number of 512 bits quantities to submit

Where's that check on the data?

> + *
> + * Submit data from kernel space to MMIO space, in units of 512 bits at a
> + * time.  Order of access is not guaranteed, nor is a memory barrier
> + * performed afterwards.
> + */
> +static inline void iosubmit_cmds512(void __iomem *dst, const void *src,
> +				    size_t count)

An iosubmit function which returns void and doesn't tell its callers
whether it succeeded or not? That looks non-optimal to say the least.

Why isn't there a fallback function which to call when the CPU doesn't
support movdir64b?

Because then you can use alternative_call() and have the thing work
regardless of hardware support for MOVDIR*.

> +{
> +	const u8 *from = src;
> +	const u8 *end = from + count * 64;
> +
> +	if (!cpu_has_write512())

If anything, that thing needs to go and you should use

  static_cpu_has(X86_FEATURE_MOVDIR64B)

as it looks to me like you would care about speed on this fast path?
Yes, no?

-- 
Regards/Gruss,
    Boris.

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