| 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
| ||
|
Message-ID: <3d8aa022-a73e-aa27-5219-12dcf9f20443@rivosinc.com>
Date: Thu, 3 Nov 2022 21:33:39 -0700
From: Vineet Gupta <vineetg@...osinc.com>
To: Chris Stillson <stillson@...osinc.com>
Cc: Greentime Hu <greentime.hu@...ive.com>,
Guo Ren <guoren@...ux.alibaba.com>,
Vincent Chen <vincent.chen@...ive.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Eric Biederman <ebiederm@...ssion.com>,
Kees Cook <keescook@...omium.org>,
Anup Patel <anup@...infault.org>,
Atish Patra <atishp@...shpatra.org>,
Oleg Nesterov <oleg@...hat.com>, Guo Ren <guoren@...nel.org>,
Heinrich Schuchardt <heinrich.schuchardt@...onical.com>,
Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Arnd Bergmann <arnd@...db.de>,
Heiko Stuebner <heiko@...ech.de>,
Jisheng Zhang <jszhang@...nel.org>,
Dao Lu <daolu@...osinc.com>,
Sunil V L <sunilvl@...tanamicro.com>,
Andy Chiu <andy.chiu@...ive.com>, Guo Ren <guoren@...nel.org>,
lkml <linux-kernel@...r.kernel.org>,
linux-riscv <linux-riscv@...ts.infradead.org>, linux-mm@...ck.org
Subject: Re: [PATCH v12 05/17] riscv: Add has_vector/riscv_vsize to save
vector features.
On 9/21/22 14:43, Chris Stillson wrote:
> From: Greentime Hu <greentime.hu@...ive.com>
>
> This patch is used to detect vector support status of CPU and use
> riscv_vsize to save the size of all the vector registers. It assumes
> all harts has the same capabilities in SMP system.
Patch title is horrible. The meat of patch is vector state save/restore,
but no users of it yet. And then there are random unrelated snippets
thrown in same patch.
> +#ifdef CONFIG_FPU
> +__ro_after_init DEFINE_STATIC_KEY_FALSE(cpu_hwcap_fpu);
> +#endif
This needs to be broken out to a FPU patch which actually uses
cpu_hwcap_fpu.
> +#ifdef CONFIG_VECTOR
> +#include <asm/vector.h>
> +__ro_after_init DEFINE_STATIC_KEY_FALSE(cpu_hwcap_vector);
> +unsigned long riscv_vsize __read_mostly;
> +#endif
I'd move this patch v2/17 as part of detection etc.
> +#ifdef CONFIG_VECTOR
> + if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
> + static_branch_enable(&cpu_hwcap_vector);
Ditto.
> + /* There are 32 vector registers with vlenb length. */
> + rvv_enable();
> + riscv_vsize = csr_read(CSR_VLENB) * 32;
Ditto.
> + rvv_disable();
But guess these needs to be added first as well, see below.
> +#ifdef CONFIG_VECTOR
> +#include <asm/vector.h>
> +EXPORT_SYMBOL(rvv_enable);
> +EXPORT_SYMBOL(rvv_disable);
As suggested in prior review comment, we don't need to EXPORT these, for
now at least.
> diff --git a/arch/riscv/kernel/vector.S b/arch/riscv/kernel/vector.S
> new file mode 100644
> index 000000000000..9f7dc70c4443
> --- /dev/null
> +++ b/arch/riscv/kernel/vector.S
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017 SiFive
> + * Copyright (C) 2019 Alibaba Group Holding Limited
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation, version 2.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
SPDX hdr ...
> +
> +#include <linux/linkage.h>
> +
> +#include <asm/asm.h>
> +#include <asm/csr.h>
> +#include <asm/asm-offsets.h>
> +
> +#define vstatep a0
> +#define datap a1
> +#define x_vstart t0
> +#define x_vtype t1
> +#define x_vl t2
> +#define x_vcsr t3
> +#define incr t4
> +#define status t5
> +
A few words here as to when is this save/restore done.
Perhaps best to add this code in the patch which actually uses these.
> +ENTRY(__vstate_save)
> + li status, SR_VS
> + csrs CSR_STATUS, status
> +
> + csrr x_vstart, CSR_VSTART
> + csrr x_vtype, CSR_VTYPE
> + csrr x_vl, CSR_VL
> + csrr x_vcsr, CSR_VCSR
> + vsetvli incr, x0, e8, m8, ta, ma
> + vse8.v v0, (datap)
> + add datap, datap, incr
> + vse8.v v8, (datap)
> + add datap, datap, incr
> + vse8.v v16, (datap)
> + add datap, datap, incr
> + vse8.v v24, (datap)
> +
> + REG_S x_vstart, RISCV_V_STATE_VSTART(vstatep)
> + REG_S x_vtype, RISCV_V_STATE_VTYPE(vstatep)
> + REG_S x_vl, RISCV_V_STATE_VL(vstatep)
> + REG_S x_vcsr, RISCV_V_STATE_VCSR(vstatep)
> +
> + csrc CSR_STATUS, status
> + ret
> +ENDPROC(__vstate_save)
> +
> +ENTRY(__vstate_restore)
> + li status, SR_VS
> + csrs CSR_STATUS, status
This is rvv_enable code duplicated inline.
> +
> + vsetvli incr, x0, e8, m8, ta, ma
> + vle8.v v0, (datap)
> + add datap, datap, incr
> + vle8.v v8, (datap)
> + add datap, datap, incr
> + vle8.v v16, (datap)
> + add datap, datap, incr
> + vle8.v v24, (datap)
> +
> + REG_L x_vstart, RISCV_V_STATE_VSTART(vstatep)
> + REG_L x_vtype, RISCV_V_STATE_VTYPE(vstatep)
> + REG_L x_vl, RISCV_V_STATE_VL(vstatep)
> + REG_L x_vcsr, RISCV_V_STATE_VCSR(vstatep)
> + vsetvl x0, x_vl, x_vtype
> + csrw CSR_VSTART, x_vstart
> + csrw CSR_VCSR, x_vcsr
> +
> + csrc CSR_STATUS, status
> + ret
> +ENDPROC(__vstate_restore)
> +
> +ENTRY(rvv_enable)
> + li status, SR_VS
> + csrs CSR_STATUS, status
> + ret
> +ENDPROC(rvv_enable)
> +
> +ENTRY(rvv_disable)
> + li status, SR_VS
> + csrc CSR_STATUS, status
> + ret
> +ENDPROC(rvv_disable)
I'd suggest these be made asm macros, to avoid function call overhead
and duplication as in save/restore above.
Powered by blists - more mailing lists