| 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: <1b0ca747-e6cb-e53f-f5cf-b1bbfc1ed839@xen0n.name>
Date: Sat, 26 Aug 2023 19:29:19 +0800
From: WANG Xuerui <kernel@...0n.name>
To: Tianrui Zhao <zhaotianrui@...ngson.cn>,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc: Paolo Bonzini <pbonzini@...hat.com>,
Huacai Chen <chenhuacai@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
loongarch@...ts.linux.dev, Jens Axboe <axboe@...nel.dk>,
Mark Brown <broonie@...nel.org>,
Alex Deucher <alexander.deucher@....com>,
Oliver Upton <oliver.upton@...ux.dev>, maobibo@...ngson.cn,
Xi Ruoyao <xry111@...111.site>
Subject: Re: [PATCH v19 05/30] LoongArch: KVM: Add vcpu related header files
Hi,
On 8/17/23 20:59, Tianrui Zhao wrote:
> Add LoongArch vcpu related header files, including vcpu csr
> information, irq number defines, and some vcpu interfaces.
>
> Reviewed-by: Bibo Mao <maobibo@...ngson.cn>
> Signed-off-by: Tianrui Zhao <zhaotianrui@...ngson.cn>
> ---
> arch/loongarch/include/asm/insn-def.h | 55 ++++++
> arch/loongarch/include/asm/kvm_csr.h | 252 +++++++++++++++++++++++++
> arch/loongarch/include/asm/kvm_vcpu.h | 95 ++++++++++
> arch/loongarch/include/asm/loongarch.h | 20 +-
> arch/loongarch/kvm/trace.h | 168 +++++++++++++++++
> 5 files changed, 585 insertions(+), 5 deletions(-)
> create mode 100644 arch/loongarch/include/asm/insn-def.h
> create mode 100644 arch/loongarch/include/asm/kvm_csr.h
> create mode 100644 arch/loongarch/include/asm/kvm_vcpu.h
> create mode 100644 arch/loongarch/kvm/trace.h
>
> diff --git a/arch/loongarch/include/asm/insn-def.h b/arch/loongarch/include/asm/insn-def.h
> new file mode 100644
> index 000000000000..e285ee108fb0
> --- /dev/null
> +++ b/arch/loongarch/include/asm/insn-def.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_INSN_DEF_H
> +#define __ASM_INSN_DEF_H
> +
> +#include <linux/stringify.h>
> +#include <asm/gpr-num.h>
> +#include <asm/asm.h>
> +
> +#define INSN_STR(x) __stringify(x)
> +#define CSR_RD_SHIFT 0
> +#define CSR_RJ_SHIFT 5
> +#define CSR_SIMM14_SHIFT 10
> +#define CSR_OPCODE_SHIFT 24
> +
> +#define DEFINE_INSN_CSR \
> + __DEFINE_ASM_GPR_NUMS \
> +" .macro insn_csr, opcode, rj, rd, simm14\n" \
> +" .4byte ((\\opcode << " INSN_STR(CSR_OPCODE_SHIFT) ") |" \
> +" (.L__gpr_num_\\rj << " INSN_STR(CSR_RJ_SHIFT) ") |" \
> +" (.L__gpr_num_\\rd << " INSN_STR(CSR_RD_SHIFT) ") |" \
> +" (\\simm14 << " INSN_STR(CSR_SIMM14_SHIFT) "))\n" \
> +" .endm\n"
Okay so the previous request (just removing this file and related
machinery, with detailed analysis [1] as to why this is doable across
the whole LoongArch ecosystem, and agreed by at least one downstream
commercial distro [2]) still isn't being honored...
Maybe give your arguments as to what edge case the previous analysis
wasn't covering, or *at the very least* re-use the existing "parse_r"
helper and don't re-invent it, should it happen that such usage is
actually deemed acceptable in the kvm subsystem as previously pointed
out by Ruoyao [3].
[1]:
https://lore.kernel.org/loongarch/81270b55-37c4-d566-8cd7-acc90b490c10@xen0n.name/
[2]:
https://lore.kernel.org/loongarch/367239C38ED9D19C+9ef16061-9057-482c-bd8c-0b9ede71cfa7@uniontech.com/
[3]:
https://lore.kernel.org/loongarch/6a5ed2266138cc61cbe27577424bb53cda72378d.camel@xry111.site/
> +
> +#define UNDEFINE_INSN_CSR \
> +" .purgem insn_csr\n"
> +
> +#define __INSN_CSR(opcode, rj, rd, simm14) \
> + DEFINE_INSN_CSR \
> + "insn_csr " opcode ", " rj ", " rd ", " simm14 "\n" \
> + UNDEFINE_INSN_CSR
> +
> +
> +#define INSN_CSR(opcode, rj, rd, simm14) \
> + __INSN_CSR(LARCH_##opcode, LARCH_##rj, LARCH_##rd, \
> + LARCH_##simm14)
> +
> +#define __ASM_STR(x) #x
> +#define LARCH_OPCODE(v) __ASM_STR(v)
> +#define LARCH_SIMM14(v) __ASM_STR(v)
> +#define __LARCH_REG(v) __ASM_STR(v)
> +#define LARCH___RD(v) __LARCH_REG(v)
> +#define LARCH___RJ(v) __LARCH_REG(v)
> +#define LARCH_OPCODE_GCSR LARCH_OPCODE(5)
> +
> +#define GCSR_read(csr, rd) \
> + INSN_CSR(OPCODE_GCSR, __RJ(zero), __RD(rd), SIMM14(csr))
> +
> +#define GCSR_write(csr, rd) \
> + INSN_CSR(OPCODE_GCSR, __RJ($r1), __RD(rd), SIMM14(csr))
> +
> +#define GCSR_xchg(csr, rj, rd) \
> + INSN_CSR(OPCODE_GCSR, __RJ(rj), __RD(rd), SIMM14(csr))
> +
> +#endif /* __ASM_INSN_DEF_H */
> diff --git a/arch/loongarch/include/asm/kvm_csr.h b/arch/loongarch/include/asm/kvm_csr.h
> new file mode 100644
> index 000000000000..34483bbaec15
> --- /dev/null
> +++ b/arch/loongarch/include/asm/kvm_csr.h
> @@ -0,0 +1,252 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020-2023 Loongson Technology Corporation Limited
> + */
> +
> +#ifndef __ASM_LOONGARCH_KVM_CSR_H__
> +#define __ASM_LOONGARCH_KVM_CSR_H__
> +#include <asm/loongarch.h>
> +#include <asm/kvm_vcpu.h>
> +#include <linux/uaccess.h>
> +#include <linux/kvm_host.h>
> +
> +#ifdef CONFIG_AS_HAS_LVZ_EXTENSION
> +/* binutils support virtualization instructions */
It may also be LLVM IAS, so I'd suggest removing this comment as the
config symbol name is pretty self-documenting.
But this Kconfig flag isn't being defined until Patch 28, which feels a
bit reversed. I'd suggest splitting the definition for this symbol out
for a small patch before this one, or merging that definition into this
patch.
> +#define gcsr_read(csr) \
> +({ \
> + register unsigned long __v; \
> + __asm__ __volatile__( \
> + " gcsrrd %[val], %[reg]\n\t" \
> + : [val] "=r" (__v) \
> + : [reg] "i" (csr) \
> + : "memory"); \
> + __v; \
> +})
> +
> +#define gcsr_write(v, csr) \
> +({ \
> + register unsigned long __v = v; \
> + __asm__ __volatile__ ( \
> + " gcsrwr %[val], %[reg]\n\t" \
> + : [val] "+r" (__v) \
> + : [reg] "i" (csr) \
> + : "memory"); \
> +})
> +
> +#define gcsr_xchg(v, m, csr) \
> +({ \
> + register unsigned long __v = v; \
> + __asm__ __volatile__( \
> + " gcsrxchg %[val], %[mask], %[reg]\n\t" \
> + : [val] "+r" (__v) \
> + : [mask] "r" (m), [reg] "i" (csr) \
> + : "memory"); \
> + __v; \
> +})
> +#else
> +/* binutils do not support virtualization instructions */
Similarly this comment could be removed. Or better, instead hint at the
symbol being checked so readers don't lose context:
#else /* CONFIG_AS_HAS_LVZ_EXTENSION */
> +#define gcsr_read(csr) \
> +({ \
> + register unsigned long __v; \
> + __asm__ __volatile__ (GCSR_read(csr, %0) \
> + : "=r" (__v) : \
> + : "memory"); \
> + __v; \
> +})
> +
> +#define gcsr_write(val, csr) \
> +({ \
> + register unsigned long __v = val; \
> + __asm__ __volatile__ (GCSR_write(csr, %0) \
> + : "+r" (__v) : \
> + : "memory"); \
> +})
> +
> +#define gcsr_xchg(val, mask, csr) \
> +({ \
> + register unsigned long __v = val; \
> + __asm__ __volatile__ (GCSR_xchg(csr, %1, %0) \
> + : "+r" (__v) \
> + : "r" (mask) \
> + : "memory"); \
> + __v; \
> +})
> +#endif
> +
>
> [snip]
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
Powered by blists - more mailing lists