Re: [PATCH v15 05/30] LoongArch: KVM: Add vcpu related header files

[bibo mao]

在 2023/6/27 16:19, WANG Xuerui 写道:
> On 2023/6/26 16:47, 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@xxxxxxxxxxx>
>> Signed-off-by: Tianrui Zhao <zhaotianrui@xxxxxxxxxxx>
>> ---
>>   arch/loongarch/include/asm/insn-def.h  |  55 ++++++
>>   arch/loongarch/include/asm/kvm_csr.h   | 231 +++++++++++++++++++++++++
>>   arch/loongarch/include/asm/kvm_vcpu.h  |  97 +++++++++++
>>   arch/loongarch/include/asm/loongarch.h |  20 ++-
>>   arch/loongarch/kvm/trace.h             | 168 ++++++++++++++++++
>>   5 files changed, 566 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"
>> +
>> +#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 */
> 
> I still find this unnecessarily complex. First of all this is reinventing infra that's already available as the "parse_r" helper (check out include/asm/tlb.h in v6.4), but the only usage of the helper has just been removed, so it's probably a signal saying this practice may not last for long -- people are no longer in a situation like back in the MIPS era when toolchain support are not guaranteed (or even allowed upstream).
> 
> Secondly, while support for older compilers is nice-to-have, but users of upstream kernels also already effectively depend on very recent toolchains (if not bleeding-edge). So we can just probe for support and just use proper mnemonics and automatically get support soon, because we can expect most of them to pick up upstream changes very quickly. That's to say, if we have something like:
> 
> # arch/loongarch/Kconfig
> config LOONGARCH
>     # ...
>     select HAVE_KVM if AS_HAS_LVZ_EXTENSION
No, I do not think so. It is a bad idea, by this way kvm module will be disabled until there is proper gcc version.
The popular gcc12/gcc13 does not support AS_HAS_LVZ_EXTENSION, kvm modules will be disabled with general gcc, may be useful for future gcc. what is detailed date for future gcc?

The existing patch can work on all gcc version. If the piece of code does not support generic gcc, why we post the patch now? It is just post a bunch of usefulness code with your method.

Regards
Bibo Mao
> 
> config AS_HAS_LVZ_EXTENSION
>     def_bool $(as-instr,gcsrrd \$t0$(comma)\$t1$(comma)123)
> 
> Then support is guaranteed for all KVM code and this cruft can go away, and then the feature will likely be available in a few months.
> 
> FYI, support for LSX and LASX instructions are already posted by your fellow toolchain folks [1], so it's 100% doable for them to add support despite the manuals not being available yet. Just coordinate with them a bit...
> 
> [1]: https://sourceware.org/pipermail/binutils/2023-June/127990.html
>