| Message ID | 20241016193140.2206352-4-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg/riscv: Add support for vector | expand |
On Thu, Oct 17, 2024 at 5:35 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > From: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com> > > Add support for probing RISC-V vector extension availability in > the backend. This information will be used when deciding whether > to use vector instructions in code generation. > > Cache lg2(vlenb) for the backend. The storing of lg2(vlenb) means > we can convert all of the division into subtraction. > > While the compiler doesn't support RISCV_HWPROBE_EXT_ZVE64X, > we use RISCV_HWPROBE_IMA_V instead. RISCV_HWPROBE_IMA_V is more > strictly constrainted than RISCV_HWPROBE_EXT_ZVE64X. At least in > current QEMU implemenation, the V vector extension depends on the > zve64d extension. > > Signed-off-by: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com> > Reviewed-by: Liu Zhiwei <zhiwei_liu@linux.alibaba.com> > Message-ID: <20241007025700.47259-2-zhiwei_liu@linux.alibaba.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Acked-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > host/include/riscv/host/cpuinfo.h | 2 ++ > util/cpuinfo-riscv.c | 24 ++++++++++++++++++++++-- > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/host/include/riscv/host/cpuinfo.h b/host/include/riscv/host/cpuinfo.h > index 2b00660e36..cdc784e7b6 100644 > --- a/host/include/riscv/host/cpuinfo.h > +++ b/host/include/riscv/host/cpuinfo.h > @@ -10,9 +10,11 @@ > #define CPUINFO_ZBA (1u << 1) > #define CPUINFO_ZBB (1u << 2) > #define CPUINFO_ZICOND (1u << 3) > +#define CPUINFO_ZVE64X (1u << 4) > > /* Initialized with a constructor. */ > extern unsigned cpuinfo; > +extern unsigned riscv_lg2_vlenb; > > /* > * We cannot rely on constructor ordering, so other constructors must > diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c > index 8cacc67645..16114ffd32 100644 > --- a/util/cpuinfo-riscv.c > +++ b/util/cpuinfo-riscv.c > @@ -4,6 +4,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/host-utils.h" > #include "host/cpuinfo.h" > > #ifdef CONFIG_ASM_HWPROBE_H > @@ -13,6 +14,7 @@ > #endif > > unsigned cpuinfo; > +unsigned riscv_lg2_vlenb; > static volatile sig_atomic_t got_sigill; > > static void sigill_handler(int signo, siginfo_t *si, void *data) > @@ -34,7 +36,7 @@ static void sigill_handler(int signo, siginfo_t *si, void *data) > /* Called both as constructor and (possibly) via other constructors. */ > unsigned __attribute__((constructor)) cpuinfo_init(void) > { > - unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND; > + unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND | CPUINFO_ZVE64X; > unsigned info = cpuinfo; > > if (info) { > @@ -50,6 +52,9 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) > #endif > #if defined(__riscv_arch_test) && defined(__riscv_zicond) > info |= CPUINFO_ZICOND; > +#endif > +#if defined(__riscv_arch_test) && defined(__riscv_zve64x) > + info |= CPUINFO_ZVE64X; > #endif > left &= ~info; > > @@ -65,7 +70,8 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) > && pair.key >= 0) { > info |= pair.value & RISCV_HWPROBE_EXT_ZBA ? CPUINFO_ZBA : 0; > info |= pair.value & RISCV_HWPROBE_EXT_ZBB ? CPUINFO_ZBB : 0; > - left &= ~(CPUINFO_ZBA | CPUINFO_ZBB); > + info |= pair.value & RISCV_HWPROBE_IMA_V ? CPUINFO_ZVE64X : 0; > + left &= ~(CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZVE64X); > #ifdef RISCV_HWPROBE_EXT_ZICOND > info |= pair.value & RISCV_HWPROBE_EXT_ZICOND ? CPUINFO_ZICOND : 0; > left &= ~CPUINFO_ZICOND; > @@ -113,6 +119,20 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) > assert(left == 0); > } > > + if (info & CPUINFO_ZVE64X) { > + /* > + * We are guaranteed by RVV-1.0 that VLEN is a power of 2. > + * We are guaranteed by Zve64x that VLEN >= 64, and that > + * EEW of {8,16,32,64} are supported. > + * > + * Cache VLEN in a convenient form. > + */ > + unsigned long vlenb; > + /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ > + asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); > + riscv_lg2_vlenb = ctz32(vlenb); > + } > + > info |= CPUINFO_ALWAYS; > cpuinfo = info; > return info; > -- > 2.43.0 > >
Hi, This patch is breaking a KVM guest that runs with '-cpu host' in an emulated Risc-V host. The break happens regardless of the RVV support in the emulated host: $ qemu-system-riscv64 \ -machine virt,accel=kvm -m 2G -smp 1 \ -cpu host \ -nographic -snapshot \ -kernel ./guest_imgs/Image \ -initrd ./guest_imgs/rootfs_kvm_riscv64.img \ -append "root=/dev/ram rw console=ttyS0 earlycon=sbi" qemu-system-riscv64: ../util/cpuinfo-riscv.c:119: cpuinfo_init: Assertion `left == 0' failed. Aborted In a quick debug: On 10/16/24 4:31 PM, Richard Henderson wrote: > From: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com> > > Add support for probing RISC-V vector extension availability in > the backend. This information will be used when deciding whether > to use vector instructions in code generation. > > Cache lg2(vlenb) for the backend. The storing of lg2(vlenb) means > we can convert all of the division into subtraction. > > While the compiler doesn't support RISCV_HWPROBE_EXT_ZVE64X, > we use RISCV_HWPROBE_IMA_V instead. RISCV_HWPROBE_IMA_V is more > strictly constrainted than RISCV_HWPROBE_EXT_ZVE64X. At least in > current QEMU implemenation, the V vector extension depends on the > zve64d extension. > > Signed-off-by: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com> > Reviewed-by: Liu Zhiwei <zhiwei_liu@linux.alibaba.com> > Message-ID: <20241007025700.47259-2-zhiwei_liu@linux.alibaba.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > host/include/riscv/host/cpuinfo.h | 2 ++ > util/cpuinfo-riscv.c | 24 ++++++++++++++++++++++-- > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/host/include/riscv/host/cpuinfo.h b/host/include/riscv/host/cpuinfo.h > index 2b00660e36..cdc784e7b6 100644 > --- a/host/include/riscv/host/cpuinfo.h > +++ b/host/include/riscv/host/cpuinfo.h > @@ -10,9 +10,11 @@ > #define CPUINFO_ZBA (1u << 1) > #define CPUINFO_ZBB (1u << 2) > #define CPUINFO_ZICOND (1u << 3) > +#define CPUINFO_ZVE64X (1u << 4) > > /* Initialized with a constructor. */ > extern unsigned cpuinfo; > +extern unsigned riscv_lg2_vlenb; > > /* > * We cannot rely on constructor ordering, so other constructors must > diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c > index 8cacc67645..16114ffd32 100644 > --- a/util/cpuinfo-riscv.c > +++ b/util/cpuinfo-riscv.c > @@ -4,6 +4,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/host-utils.h" > #include "host/cpuinfo.h" > > #ifdef CONFIG_ASM_HWPROBE_H > @@ -13,6 +14,7 @@ > #endif > > unsigned cpuinfo; > +unsigned riscv_lg2_vlenb; > static volatile sig_atomic_t got_sigill; > > static void sigill_handler(int signo, siginfo_t *si, void *data) > @@ -34,7 +36,7 @@ static void sigill_handler(int signo, siginfo_t *si, void *data) > /* Called both as constructor and (possibly) via other constructors. */ > unsigned __attribute__((constructor)) cpuinfo_init(void) > { > - unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND; > + unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND | CPUINFO_ZVE64X; This will init 'left' with 30 (2 + 4 + 8 + 16) > unsigned info = cpuinfo; > > if (info) { > @@ -50,6 +52,9 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) > #endif > #if defined(__riscv_arch_test) && defined(__riscv_zicond) > info |= CPUINFO_ZICOND; > +#endif > +#if defined(__riscv_arch_test) && defined(__riscv_zve64x) > + info |= CPUINFO_ZVE64X; > #endif > left &= ~info; > > @@ -65,7 +70,8 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) > && pair.key >= 0) { > info |= pair.value & RISCV_HWPROBE_EXT_ZBA ? CPUINFO_ZBA : 0; > info |= pair.value & RISCV_HWPROBE_EXT_ZBB ? CPUINFO_ZBB : 0; > - left &= ~(CPUINFO_ZBA | CPUINFO_ZBB); > + info |= pair.value & RISCV_HWPROBE_IMA_V ? CPUINFO_ZVE64X : 0; > + left &= ~(CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZVE64X); > #ifdef RISCV_HWPROBE_EXT_ZICOND > info |= pair.value & RISCV_HWPROBE_EXT_ZICOND ? CPUINFO_ZICOND : 0; > left &= ~CPUINFO_ZICOND; > @@ -113,6 +119,20 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) > assert(left == 0); To better understand, this is the 'if' block that contains this assert: if (left) { struct sigaction sa_old, sa_new; memset(&sa_new, 0, sizeof(sa_new)); sa_new.sa_flags = SA_SIGINFO; sa_new.sa_sigaction = sigill_handler; sigaction(SIGILL, &sa_new, &sa_old); if (left & CPUINFO_ZBA) { /* Probe for Zba: add.uw zero,zero,zero. */ got_sigill = 0; asm volatile(".insn r 0x3b, 0, 0x04, zero, zero, zero" : : : "memory"); info |= got_sigill ? 0 : CPUINFO_ZBA; left &= ~CPUINFO_ZBA; } if (left & CPUINFO_ZBB) { /* Probe for Zbb: andn zero,zero,zero. */ got_sigill = 0; asm volatile(".insn r 0x33, 7, 0x20, zero, zero, zero" : : : "memory"); info |= got_sigill ? 0 : CPUINFO_ZBB; left &= ~CPUINFO_ZBB; } if (left & CPUINFO_ZICOND) { /* Probe for Zicond: czero.eqz zero,zero,zero. */ got_sigill = 0; asm volatile(".insn r 0x33, 5, 0x07, zero, zero, zero" : : : "memory"); info |= got_sigill ? 0 : CPUINFO_ZICOND; left &= ~CPUINFO_ZICOND; } sigaction(SIGILL, &sa_old, NULL); assert(left == 0); } The 'assert' is hit at this point because left is 16, i.e. left = CPUINFO_ZVE64X. I did a fix based on what seems to be the usual flow of how 'left' is being calculated: $ git diff diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c index 16114ffd32..25a98a75ad 100644 --- a/util/cpuinfo-riscv.c +++ b/util/cpuinfo-riscv.c @@ -115,24 +115,27 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) left &= ~CPUINFO_ZICOND; } + if (left & CPUINFO_ZVE64X) { + /* + * We are guaranteed by RVV-1.0 that VLEN is a power of 2. + * We are guaranteed by Zve64x that VLEN >= 64, and that + * EEW of {8,16,32,64} are supported. + * + * Cache VLEN in a convenient form. + */ + unsigned long vlenb; + got_sigill = 0; + /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ + asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); + info |= got_sigill ? 0 : CPUINFO_ZVE64X; + left &= ~CPUINFO_ZVE64X; + riscv_lg2_vlenb = ctz32(vlenb); + } + sigaction(SIGILL, &sa_old, NULL); assert(left == 0); } - if (info & CPUINFO_ZVE64X) { - /* - * We are guaranteed by RVV-1.0 that VLEN is a power of 2. - * We are guaranteed by Zve64x that VLEN >= 64, and that - * EEW of {8,16,32,64} are supported. - * - * Cache VLEN in a convenient form. - */ - unsigned long vlenb; - /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ - asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); - riscv_lg2_vlenb = ctz32(vlenb); - } - info |= CPUINFO_ALWAYS; cpuinfo = info; return info; i.e. I moved the CPUINFO_ZVE64X inside the 'if (left)' block, then update both 'info' and 'left' depending on if we found the vlenb CSR. Note that this fixes the issue I'm seeing (KVM guest boot), but I can't say if the original intent of the patch is preserved. If this is a good fix feel free to squash this diff into the patch. Thanks, Daniel > }> > + if (info & CPUINFO_ZVE64X) { > + /* > + * We are guaranteed by RVV-1.0 that VLEN is a power of 2. > + * We are guaranteed by Zve64x that VLEN >= 64, and that > + * EEW of {8,16,32,64} are supported. > + * > + * Cache VLEN in a convenient form. > + */ > + unsigned long vlenb; > + /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ > + asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); > + riscv_lg2_vlenb = ctz32(vlenb); > + } > + > info |= CPUINFO_ALWAYS; > cpuinfo = info; > return info;
On 10/21/24 11:25, Daniel Henrique Barboza wrote: > Hi, > > This patch is breaking a KVM guest that runs with '-cpu host' in an emulated > Risc-V host. The break happens regardless of the RVV support in the emulated > host: > > > $ qemu-system-riscv64 \ > -machine virt,accel=kvm -m 2G -smp 1 \ > -cpu host \ > -nographic -snapshot \ > -kernel ./guest_imgs/Image \ > -initrd ./guest_imgs/rootfs_kvm_riscv64.img \ > -append "root=/dev/ram rw console=ttyS0 earlycon=sbi" > > qemu-system-riscv64: ../util/cpuinfo-riscv.c:119: cpuinfo_init: Assertion `left == 0' failed. > Aborted > > > In a quick debug: > > > On 10/16/24 4:31 PM, Richard Henderson wrote: >> From: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com> >> >> Add support for probing RISC-V vector extension availability in >> the backend. This information will be used when deciding whether >> to use vector instructions in code generation. >> >> Cache lg2(vlenb) for the backend. The storing of lg2(vlenb) means >> we can convert all of the division into subtraction. >> >> While the compiler doesn't support RISCV_HWPROBE_EXT_ZVE64X, >> we use RISCV_HWPROBE_IMA_V instead. RISCV_HWPROBE_IMA_V is more >> strictly constrainted than RISCV_HWPROBE_EXT_ZVE64X. At least in >> current QEMU implemenation, the V vector extension depends on the >> zve64d extension. >> >> Signed-off-by: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com> >> Reviewed-by: Liu Zhiwei <zhiwei_liu@linux.alibaba.com> >> Message-ID: <20241007025700.47259-2-zhiwei_liu@linux.alibaba.com> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> host/include/riscv/host/cpuinfo.h | 2 ++ >> util/cpuinfo-riscv.c | 24 ++++++++++++++++++++++-- >> 2 files changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/host/include/riscv/host/cpuinfo.h b/host/include/riscv/host/cpuinfo.h >> index 2b00660e36..cdc784e7b6 100644 >> --- a/host/include/riscv/host/cpuinfo.h >> +++ b/host/include/riscv/host/cpuinfo.h >> @@ -10,9 +10,11 @@ >> #define CPUINFO_ZBA (1u << 1) >> #define CPUINFO_ZBB (1u << 2) >> #define CPUINFO_ZICOND (1u << 3) >> +#define CPUINFO_ZVE64X (1u << 4) >> /* Initialized with a constructor. */ >> extern unsigned cpuinfo; >> +extern unsigned riscv_lg2_vlenb; >> /* >> * We cannot rely on constructor ordering, so other constructors must >> diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c >> index 8cacc67645..16114ffd32 100644 >> --- a/util/cpuinfo-riscv.c >> +++ b/util/cpuinfo-riscv.c >> @@ -4,6 +4,7 @@ >> */ >> #include "qemu/osdep.h" >> +#include "qemu/host-utils.h" >> #include "host/cpuinfo.h" >> #ifdef CONFIG_ASM_HWPROBE_H >> @@ -13,6 +14,7 @@ >> #endif >> unsigned cpuinfo; >> +unsigned riscv_lg2_vlenb; >> static volatile sig_atomic_t got_sigill; >> static void sigill_handler(int signo, siginfo_t *si, void *data) >> @@ -34,7 +36,7 @@ static void sigill_handler(int signo, siginfo_t *si, void *data) >> /* Called both as constructor and (possibly) via other constructors. */ >> unsigned __attribute__((constructor)) cpuinfo_init(void) >> { >> - unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND; >> + unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND | CPUINFO_ZVE64X; > > > This will init 'left' with 30 (2 + 4 + 8 + 16) > > >> unsigned info = cpuinfo; >> if (info) { >> @@ -50,6 +52,9 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) >> #endif >> #if defined(__riscv_arch_test) && defined(__riscv_zicond) >> info |= CPUINFO_ZICOND; >> +#endif >> +#if defined(__riscv_arch_test) && defined(__riscv_zve64x) >> + info |= CPUINFO_ZVE64X; >> #endif >> left &= ~info; >> @@ -65,7 +70,8 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) >> && pair.key >= 0) { >> info |= pair.value & RISCV_HWPROBE_EXT_ZBA ? CPUINFO_ZBA : 0; >> info |= pair.value & RISCV_HWPROBE_EXT_ZBB ? CPUINFO_ZBB : 0; >> - left &= ~(CPUINFO_ZBA | CPUINFO_ZBB); >> + info |= pair.value & RISCV_HWPROBE_IMA_V ? CPUINFO_ZVE64X : 0; >> + left &= ~(CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZVE64X); >> #ifdef RISCV_HWPROBE_EXT_ZICOND >> info |= pair.value & RISCV_HWPROBE_EXT_ZICOND ? CPUINFO_ZICOND : 0; >> left &= ~CPUINFO_ZICOND; >> @@ -113,6 +119,20 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) >> assert(left == 0); > > To better understand, this is the 'if' block that contains this assert: > > if (left) { > struct sigaction sa_old, sa_new; > > memset(&sa_new, 0, sizeof(sa_new)); > sa_new.sa_flags = SA_SIGINFO; > sa_new.sa_sigaction = sigill_handler; > sigaction(SIGILL, &sa_new, &sa_old); > > if (left & CPUINFO_ZBA) { > /* Probe for Zba: add.uw zero,zero,zero. */ > got_sigill = 0; > asm volatile(".insn r 0x3b, 0, 0x04, zero, zero, zero" > : : : "memory"); > info |= got_sigill ? 0 : CPUINFO_ZBA; > left &= ~CPUINFO_ZBA; > } > > if (left & CPUINFO_ZBB) { > /* Probe for Zbb: andn zero,zero,zero. */ > got_sigill = 0; > asm volatile(".insn r 0x33, 7, 0x20, zero, zero, zero" > : : : "memory"); > info |= got_sigill ? 0 : CPUINFO_ZBB; > left &= ~CPUINFO_ZBB; > } > > if (left & CPUINFO_ZICOND) { > /* Probe for Zicond: czero.eqz zero,zero,zero. */ > got_sigill = 0; > asm volatile(".insn r 0x33, 5, 0x07, zero, zero, zero" > : : : "memory"); > info |= got_sigill ? 0 : CPUINFO_ZICOND; > left &= ~CPUINFO_ZICOND; > } > > sigaction(SIGILL, &sa_old, NULL); > assert(left == 0); > } > > > The 'assert' is hit at this point because left is 16, i.e. left = CPUINFO_ZVE64X. > > I did a fix based on what seems to be the usual flow of how 'left' is being calculated: > > > $ git diff > diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c > index 16114ffd32..25a98a75ad 100644 > --- a/util/cpuinfo-riscv.c > +++ b/util/cpuinfo-riscv.c > @@ -115,24 +115,27 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) > left &= ~CPUINFO_ZICOND; > } > > + if (left & CPUINFO_ZVE64X) { > + /* > + * We are guaranteed by RVV-1.0 that VLEN is a power of 2. > + * We are guaranteed by Zve64x that VLEN >= 64, and that > + * EEW of {8,16,32,64} are supported. > + * > + * Cache VLEN in a convenient form. > + */ > + unsigned long vlenb; > + got_sigill = 0; > + /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ > + asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); > + info |= got_sigill ? 0 : CPUINFO_ZVE64X; > + left &= ~CPUINFO_ZVE64X; > + riscv_lg2_vlenb = ctz32(vlenb); > + } > + > sigaction(SIGILL, &sa_old, NULL); > assert(left == 0); > } > > - if (info & CPUINFO_ZVE64X) { > - /* > - * We are guaranteed by RVV-1.0 that VLEN is a power of 2. > - * We are guaranteed by Zve64x that VLEN >= 64, and that > - * EEW of {8,16,32,64} are supported. > - * > - * Cache VLEN in a convenient form. > - */ > - unsigned long vlenb; > - /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ > - asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); > - riscv_lg2_vlenb = ctz32(vlenb); > - } > - > info |= CPUINFO_ALWAYS; > cpuinfo = info; > return info; > > > > i.e. I moved the CPUINFO_ZVE64X inside the 'if (left)' block, then update both 'info' and > 'left' depending on if we found the vlenb CSR. > > Note that this fixes the issue I'm seeing (KVM guest boot), but I can't say if the original > intent of the patch is preserved. If this is a good fix feel free to squash this diff into > the patch. This is not a good fix. The vlenb probe must happen whenever vector support is detected -- it is not for detection itself. It is my understanding that kernel support for riscv_hwprobe pre-dates kernel support for vectors in userspace. I believe that we really don't care about old kernel header installs. Therefore we should simply left &= ~CPUINFO_ZVE64X; with a comment after the CONFIG_ASM_HWPROBE_H block. I'll post a v7 for testing. r~
On 10/21/24 3:52 PM, Richard Henderson wrote: > On 10/21/24 11:25, Daniel Henrique Barboza wrote: >> Hi, >> >> This patch is breaking a KVM guest that runs with '-cpu host' in an emulated >> Risc-V host. The break happens regardless of the RVV support in the emulated >> host: >> >> >> $ qemu-system-riscv64 \ >> -machine virt,accel=kvm -m 2G -smp 1 \ >> -cpu host \ >> -nographic -snapshot \ >> -kernel ./guest_imgs/Image \ >> -initrd ./guest_imgs/rootfs_kvm_riscv64.img \ >> -append "root=/dev/ram rw console=ttyS0 earlycon=sbi" >> >> qemu-system-riscv64: ../util/cpuinfo-riscv.c:119: cpuinfo_init: Assertion `left == 0' failed. >> Aborted >> >> >> In a quick debug: >> >> >> On 10/16/24 4:31 PM, Richard Henderson wrote: >>> From: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com> >>> >>> Add support for probing RISC-V vector extension availability in >>> the backend. This information will be used when deciding whether >>> to use vector instructions in code generation. >>> >>> Cache lg2(vlenb) for the backend. The storing of lg2(vlenb) means >>> we can convert all of the division into subtraction. >>> >>> While the compiler doesn't support RISCV_HWPROBE_EXT_ZVE64X, >>> we use RISCV_HWPROBE_IMA_V instead. RISCV_HWPROBE_IMA_V is more >>> strictly constrainted than RISCV_HWPROBE_EXT_ZVE64X. At least in >>> current QEMU implemenation, the V vector extension depends on the >>> zve64d extension. >>> >>> Signed-off-by: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com> >>> Reviewed-by: Liu Zhiwei <zhiwei_liu@linux.alibaba.com> >>> Message-ID: <20241007025700.47259-2-zhiwei_liu@linux.alibaba.com> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> host/include/riscv/host/cpuinfo.h | 2 ++ >>> util/cpuinfo-riscv.c | 24 ++++++++++++++++++++++-- >>> 2 files changed, 24 insertions(+), 2 deletions(-) >>> >>> diff --git a/host/include/riscv/host/cpuinfo.h b/host/include/riscv/host/cpuinfo.h >>> index 2b00660e36..cdc784e7b6 100644 >>> --- a/host/include/riscv/host/cpuinfo.h >>> +++ b/host/include/riscv/host/cpuinfo.h >>> @@ -10,9 +10,11 @@ >>> #define CPUINFO_ZBA (1u << 1) >>> #define CPUINFO_ZBB (1u << 2) >>> #define CPUINFO_ZICOND (1u << 3) >>> +#define CPUINFO_ZVE64X (1u << 4) >>> /* Initialized with a constructor. */ >>> extern unsigned cpuinfo; >>> +extern unsigned riscv_lg2_vlenb; >>> /* >>> * We cannot rely on constructor ordering, so other constructors must >>> diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c >>> index 8cacc67645..16114ffd32 100644 >>> --- a/util/cpuinfo-riscv.c >>> +++ b/util/cpuinfo-riscv.c >>> @@ -4,6 +4,7 @@ >>> */ >>> #include "qemu/osdep.h" >>> +#include "qemu/host-utils.h" >>> #include "host/cpuinfo.h" >>> #ifdef CONFIG_ASM_HWPROBE_H >>> @@ -13,6 +14,7 @@ >>> #endif >>> unsigned cpuinfo; >>> +unsigned riscv_lg2_vlenb; >>> static volatile sig_atomic_t got_sigill; >>> static void sigill_handler(int signo, siginfo_t *si, void *data) >>> @@ -34,7 +36,7 @@ static void sigill_handler(int signo, siginfo_t *si, void *data) >>> /* Called both as constructor and (possibly) via other constructors. */ >>> unsigned __attribute__((constructor)) cpuinfo_init(void) >>> { >>> - unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND; >>> + unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND | CPUINFO_ZVE64X; >> >> >> This will init 'left' with 30 (2 + 4 + 8 + 16) >> >> >>> unsigned info = cpuinfo; >>> if (info) { >>> @@ -50,6 +52,9 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) >>> #endif >>> #if defined(__riscv_arch_test) && defined(__riscv_zicond) >>> info |= CPUINFO_ZICOND; >>> +#endif >>> +#if defined(__riscv_arch_test) && defined(__riscv_zve64x) >>> + info |= CPUINFO_ZVE64X; >>> #endif >>> left &= ~info; >>> @@ -65,7 +70,8 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) >>> && pair.key >= 0) { >>> info |= pair.value & RISCV_HWPROBE_EXT_ZBA ? CPUINFO_ZBA : 0; >>> info |= pair.value & RISCV_HWPROBE_EXT_ZBB ? CPUINFO_ZBB : 0; >>> - left &= ~(CPUINFO_ZBA | CPUINFO_ZBB); >>> + info |= pair.value & RISCV_HWPROBE_IMA_V ? CPUINFO_ZVE64X : 0; >>> + left &= ~(CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZVE64X); >>> #ifdef RISCV_HWPROBE_EXT_ZICOND >>> info |= pair.value & RISCV_HWPROBE_EXT_ZICOND ? CPUINFO_ZICOND : 0; >>> left &= ~CPUINFO_ZICOND; >>> @@ -113,6 +119,20 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) >>> assert(left == 0); >> >> To better understand, this is the 'if' block that contains this assert: >> >> if (left) { >> struct sigaction sa_old, sa_new; >> >> memset(&sa_new, 0, sizeof(sa_new)); >> sa_new.sa_flags = SA_SIGINFO; >> sa_new.sa_sigaction = sigill_handler; >> sigaction(SIGILL, &sa_new, &sa_old); >> >> if (left & CPUINFO_ZBA) { >> /* Probe for Zba: add.uw zero,zero,zero. */ >> got_sigill = 0; >> asm volatile(".insn r 0x3b, 0, 0x04, zero, zero, zero" >> : : : "memory"); >> info |= got_sigill ? 0 : CPUINFO_ZBA; >> left &= ~CPUINFO_ZBA; >> } >> >> if (left & CPUINFO_ZBB) { >> /* Probe for Zbb: andn zero,zero,zero. */ >> got_sigill = 0; >> asm volatile(".insn r 0x33, 7, 0x20, zero, zero, zero" >> : : : "memory"); >> info |= got_sigill ? 0 : CPUINFO_ZBB; >> left &= ~CPUINFO_ZBB; >> } >> >> if (left & CPUINFO_ZICOND) { >> /* Probe for Zicond: czero.eqz zero,zero,zero. */ >> got_sigill = 0; >> asm volatile(".insn r 0x33, 5, 0x07, zero, zero, zero" >> : : : "memory"); >> info |= got_sigill ? 0 : CPUINFO_ZICOND; >> left &= ~CPUINFO_ZICOND; >> } >> >> sigaction(SIGILL, &sa_old, NULL); >> assert(left == 0); >> } >> >> >> The 'assert' is hit at this point because left is 16, i.e. left = CPUINFO_ZVE64X. >> >> I did a fix based on what seems to be the usual flow of how 'left' is being calculated: >> >> >> $ git diff >> diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c >> index 16114ffd32..25a98a75ad 100644 >> --- a/util/cpuinfo-riscv.c >> +++ b/util/cpuinfo-riscv.c >> @@ -115,24 +115,27 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) >> left &= ~CPUINFO_ZICOND; >> } >> >> + if (left & CPUINFO_ZVE64X) { >> + /* >> + * We are guaranteed by RVV-1.0 that VLEN is a power of 2. >> + * We are guaranteed by Zve64x that VLEN >= 64, and that >> + * EEW of {8,16,32,64} are supported. >> + * >> + * Cache VLEN in a convenient form. >> + */ >> + unsigned long vlenb; >> + got_sigill = 0; >> + /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ >> + asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); >> + info |= got_sigill ? 0 : CPUINFO_ZVE64X; >> + left &= ~CPUINFO_ZVE64X; >> + riscv_lg2_vlenb = ctz32(vlenb); >> + } >> + >> sigaction(SIGILL, &sa_old, NULL); >> assert(left == 0); >> } >> >> - if (info & CPUINFO_ZVE64X) { >> - /* >> - * We are guaranteed by RVV-1.0 that VLEN is a power of 2. >> - * We are guaranteed by Zve64x that VLEN >= 64, and that >> - * EEW of {8,16,32,64} are supported. >> - * >> - * Cache VLEN in a convenient form. >> - */ >> - unsigned long vlenb; >> - /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ >> - asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); >> - riscv_lg2_vlenb = ctz32(vlenb); >> - } >> - >> info |= CPUINFO_ALWAYS; >> cpuinfo = info; >> return info; >> >> >> >> i.e. I moved the CPUINFO_ZVE64X inside the 'if (left)' block, then update both 'info' and >> 'left' depending on if we found the vlenb CSR. >> >> Note that this fixes the issue I'm seeing (KVM guest boot), but I can't say if the original >> intent of the patch is preserved. If this is a good fix feel free to squash this diff into >> the patch. > > This is not a good fix. The vlenb probe must happen whenever vector support is detected -- it is not for detection itself. > > It is my understanding that kernel support for riscv_hwprobe pre-dates kernel support for vectors in userspace. I believe that we really don't care about old kernel header installs. > > Therefore we should simply > > left &= ~CPUINFO_ZVE64X; > > with a comment after the CONFIG_ASM_HWPROBE_H block. > > I'll post a v7 for testing. Put me in Cc for v7 and I'll give it a go. Thanks, Daniel > > > r~
diff --git a/host/include/riscv/host/cpuinfo.h b/host/include/riscv/host/cpuinfo.h index 2b00660e36..cdc784e7b6 100644 --- a/host/include/riscv/host/cpuinfo.h +++ b/host/include/riscv/host/cpuinfo.h @@ -10,9 +10,11 @@ #define CPUINFO_ZBA (1u << 1) #define CPUINFO_ZBB (1u << 2) #define CPUINFO_ZICOND (1u << 3) +#define CPUINFO_ZVE64X (1u << 4) /* Initialized with a constructor. */ extern unsigned cpuinfo; +extern unsigned riscv_lg2_vlenb; /* * We cannot rely on constructor ordering, so other constructors must diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c index 8cacc67645..16114ffd32 100644 --- a/util/cpuinfo-riscv.c +++ b/util/cpuinfo-riscv.c @@ -4,6 +4,7 @@ */ #include "qemu/osdep.h" +#include "qemu/host-utils.h" #include "host/cpuinfo.h" #ifdef CONFIG_ASM_HWPROBE_H @@ -13,6 +14,7 @@ #endif unsigned cpuinfo; +unsigned riscv_lg2_vlenb; static volatile sig_atomic_t got_sigill; static void sigill_handler(int signo, siginfo_t *si, void *data) @@ -34,7 +36,7 @@ static void sigill_handler(int signo, siginfo_t *si, void *data) /* Called both as constructor and (possibly) via other constructors. */ unsigned __attribute__((constructor)) cpuinfo_init(void) { - unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND; + unsigned left = CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZICOND | CPUINFO_ZVE64X; unsigned info = cpuinfo; if (info) { @@ -50,6 +52,9 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) #endif #if defined(__riscv_arch_test) && defined(__riscv_zicond) info |= CPUINFO_ZICOND; +#endif +#if defined(__riscv_arch_test) && defined(__riscv_zve64x) + info |= CPUINFO_ZVE64X; #endif left &= ~info; @@ -65,7 +70,8 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) && pair.key >= 0) { info |= pair.value & RISCV_HWPROBE_EXT_ZBA ? CPUINFO_ZBA : 0; info |= pair.value & RISCV_HWPROBE_EXT_ZBB ? CPUINFO_ZBB : 0; - left &= ~(CPUINFO_ZBA | CPUINFO_ZBB); + info |= pair.value & RISCV_HWPROBE_IMA_V ? CPUINFO_ZVE64X : 0; + left &= ~(CPUINFO_ZBA | CPUINFO_ZBB | CPUINFO_ZVE64X); #ifdef RISCV_HWPROBE_EXT_ZICOND info |= pair.value & RISCV_HWPROBE_EXT_ZICOND ? CPUINFO_ZICOND : 0; left &= ~CPUINFO_ZICOND; @@ -113,6 +119,20 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) assert(left == 0); } + if (info & CPUINFO_ZVE64X) { + /* + * We are guaranteed by RVV-1.0 that VLEN is a power of 2. + * We are guaranteed by Zve64x that VLEN >= 64, and that + * EEW of {8,16,32,64} are supported. + * + * Cache VLEN in a convenient form. + */ + unsigned long vlenb; + /* Read csr "vlenb" with "csrr %0, vlenb" : "=r"(vlenb) */ + asm volatile(".insn i 0x73, 0x2, %0, zero, -990" : "=r"(vlenb)); + riscv_lg2_vlenb = ctz32(vlenb); + } + info |= CPUINFO_ALWAYS; cpuinfo = info; return info;