| Message ID | 20240806-sbi-dbcn-write-test-v1-1-7f198bb55525@berkeley.edu |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [kvm-unit-tests] riscv: sbi: add dbcn write test | expand |
On Tue, Aug 06, 2024 at 10:51:54PM GMT, Cade Richard wrote: > > > --- not sure why this --- above is here, it'll remove the commit text from the commit. Also not sure where the changelog went. We want the changelog, but we want it under the --- below your sign-off. > Added a unit test for the RISC-V SBI debug console write() and write_byte() functions. The output of the tests must be inspected manually to verify that the correct bytes are written. For write(), the expected output is 'DBCN_WRITE_TEST_STRING'. For write_byte(), the expected output is 'a'. Need to wrap lines at 74 chars. > > Signed-off-by: Cade Richard <cade.richard@berkeley.edu> > --- > lib/riscv/asm/sbi.h | 7 ++++++ > riscv/sbi.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 73 insertions(+) > > diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h > index 73ab5438..47e91025 100644 > --- a/lib/riscv/asm/sbi.h > +++ b/lib/riscv/asm/sbi.h > @@ -19,6 +19,7 @@ enum sbi_ext_id { > SBI_EXT_TIME = 0x54494d45, > SBI_EXT_HSM = 0x48534d, > SBI_EXT_SRST = 0x53525354, > + SBI_EXT_DBCN = 0x4442434E, > }; > > enum sbi_ext_base_fid { > @@ -42,6 +43,12 @@ enum sbi_ext_time_fid { > SBI_EXT_TIME_SET_TIMER = 0, > }; > > +enum sbi_ext_dbcn_fid { > + SBI_EXT_DBCN_CONSOLE_WRITE = 0, > + SBI_EXT_DBCN_CONSOLE_READ, > + SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, > +}; > + > struct sbiret { > long error; > long value; > diff --git a/riscv/sbi.c b/riscv/sbi.c > index 2438c497..61993f08 100644 > --- a/riscv/sbi.c > +++ b/riscv/sbi.c > @@ -15,6 +15,10 @@ > #include <asm/sbi.h> > #include <asm/smp.h> > #include <asm/timer.h> > +#include <asm/io.h> The includes are currently in alphabetic order. Let's keep them that way. > + > +#define DBCN_WRITE_TEST_STRING "DBCN_WRITE_TEST_STRING\n" > +#define DBCN_WRITE_BYTE_TEST_BYTE (u8)'a' > > static void help(void) > { > @@ -32,6 +36,11 @@ static struct sbiret __time_sbi_ecall(unsigned long stime_value) > return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0); > } > > +static struct sbiret __dbcn_sbi_ecall(int fid, unsigned long arg0, unsigned long arg1, unsigned long arg2) > +{ > + return sbi_ecall(SBI_EXT_DBCN, fid, arg0, arg1, arg2, 0, 0, 0); > +} > + > static bool env_or_skip(const char *env) > { > if (!getenv(env)) { > @@ -248,6 +257,62 @@ static void check_time(void) > report_prefix_pop(); > } > What happened to the comment about the read test? > +static void check_dbcn(void) > +{ > + > + struct sbiret ret; > + unsigned long num_bytes, base_addr_lo, base_addr_hi; > + int num_calls = 0; > + > + num_bytes = strlen(DBCN_WRITE_TEST_STRING); > + phys_addr_t p = virt_to_phys((void *)&DBCN_WRITE_TEST_STRING); Put p's declaration with the rest above. > + base_addr_lo = (unsigned long)p; > + base_addr_hi = (unsigned long)(p >> __riscv_xlen); > + > + report_prefix_push("dbcn"); > + > + ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_DBCN); > + if (!ret.value) { > + report_skip("DBCN extension unavailable"); > + report_prefix_pop(); > + return; > + } The report skip should be the first thing you do, i.e. the virt_to_phys stuff should be below it. > + > + report_prefix_push("write"); > + > + do { > + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE, num_bytes, base_addr_lo, base_addr_hi); > + num_bytes -= ret.value; > + base_addr_lo += ret.value; We should increment the physical address and then split it again into lo and hi since lo could have been zero (only high addresses) or lo may have started nonzero but then wrapped since we were close the 4G boundary. > + num_calls++; > + } while (num_bytes != 0 && ret.error == SBI_SUCCESS) ; > + report(ret.error == SBI_SUCCESS, "write success"); > + report_info("%d sbi calls made", num_calls); > + > + /* > + Bytes are read from memory and written to the console There should be a '*' on each line of a comment block. Doesn't checkpatch complain about that? > + */ > + if (env_or_skip("INVALID_READ_ADDR")) { > + phys_addr_t p = strtoull(getenv("INVALID_READ_ADDR"), NULL, 0); Don't shadow p, you can use the same one again. > + base_addr_lo = (unsigned long)p; > + base_addr_hi = (unsigned long)(p >> __riscv_xlen); > + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE, 1, base_addr_lo, base_addr_hi); > + report(ret.error == SBI_ERR_INVALID_PARAM, "invalid parameter: address"); > + }; > + > + report_prefix_pop(); > + > + report_prefix_push("write_byte"); > + > + puts("DBCN_WRITE TEST CHAR: "); > + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, (u8)DBCN_WRITE_BYTE_TEST_BYTE, 0, 0); > + puts("\n"); > + report(ret.error == SBI_SUCCESS, "write success"); > + report(ret.value == 0, "expected ret.value"); > + > + report_prefix_pop(); > +} > + > int main(int argc, char **argv) > { > > @@ -259,6 +324,7 @@ int main(int argc, char **argv) > report_prefix_push("sbi"); > check_base(); > check_time(); > + check_dbcn(); > > return report_summary(); > } > > --- > base-commit: 1878b4b663fd50b87de7ba2b1c90614e2703542f > change-id: 20240806-sbi-dbcn-write-test-70d305d511cf > > Best regards, > -- > Cade Richard <cade.richard@berkeley.edu> > Thanks, drew
On Tue, Aug 06, 2024 at 10:51:54PM GMT, Cade Richard wrote: > > > --- > Added a unit test for the RISC-V SBI debug console write() and write_byte() functions. The output of the tests must be inspected manually to verify that the correct bytes are written. For write(), the expected output is 'DBCN_WRITE_TEST_STRING'. For write_byte(), the expected output is 'a'. > > Signed-off-by: Cade Richard <cade.richard@berkeley.edu> > --- > lib/riscv/asm/sbi.h | 7 ++++++ > riscv/sbi.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 73 insertions(+) > > diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h > index 73ab5438..47e91025 100644 > --- a/lib/riscv/asm/sbi.h > +++ b/lib/riscv/asm/sbi.h > @@ -19,6 +19,7 @@ enum sbi_ext_id { > SBI_EXT_TIME = 0x54494d45, > SBI_EXT_HSM = 0x48534d, > SBI_EXT_SRST = 0x53525354, > + SBI_EXT_DBCN = 0x4442434E, > }; > > enum sbi_ext_base_fid { > @@ -42,6 +43,12 @@ enum sbi_ext_time_fid { > SBI_EXT_TIME_SET_TIMER = 0, > }; > > +enum sbi_ext_dbcn_fid { > + SBI_EXT_DBCN_CONSOLE_WRITE = 0, > + SBI_EXT_DBCN_CONSOLE_READ, > + SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, > +}; > + > struct sbiret { > long error; > long value; > diff --git a/riscv/sbi.c b/riscv/sbi.c > index 2438c497..61993f08 100644 > --- a/riscv/sbi.c > +++ b/riscv/sbi.c > @@ -15,6 +15,10 @@ > #include <asm/sbi.h> > #include <asm/smp.h> > #include <asm/timer.h> > +#include <asm/io.h> > + > +#define DBCN_WRITE_TEST_STRING "DBCN_WRITE_TEST_STRING\n" > +#define DBCN_WRITE_BYTE_TEST_BYTE (u8)'a' > > static void help(void) > { > @@ -32,6 +36,11 @@ static struct sbiret __time_sbi_ecall(unsigned long stime_value) > return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0); > } > > +static struct sbiret __dbcn_sbi_ecall(int fid, unsigned long arg0, unsigned long arg1, unsigned long arg2) > +{ > + return sbi_ecall(SBI_EXT_DBCN, fid, arg0, arg1, arg2, 0, 0, 0); > +} > + > static bool env_or_skip(const char *env) > { > if (!getenv(env)) { > @@ -248,6 +257,62 @@ static void check_time(void) > report_prefix_pop(); > } > > +static void check_dbcn(void) > +{ > + I still see whitespace issues, like this blank line here that has spaces, which I pointed out before. And there are several more blank lines with spaces too. > + struct sbiret ret; > + unsigned long num_bytes, base_addr_lo, base_addr_hi; > + int num_calls = 0; > + > + num_bytes = strlen(DBCN_WRITE_TEST_STRING); > + phys_addr_t p = virt_to_phys((void *)&DBCN_WRITE_TEST_STRING); > + base_addr_lo = (unsigned long)p; > + base_addr_hi = (unsigned long)(p >> __riscv_xlen); This doesn't compile for 64-bit. We get riscv/sbi.c: In function 'check_dbcn': riscv/sbi.c:270:42: error: right shift count >= width of type [-Werror=shift-count-overflow] 270 | base_addr_hi = (unsigned long)(p >> __riscv_xlen); | ^~ riscv/sbi.c:298:50: error: right shift count >= width of type [-Werror=shift-count-overflow] 298 | base_addr_hi = (unsigned long)(p >> __riscv_xlen); | ^~ cc1: all warnings being treated as errors make: *** [<builtin>: riscv/sbi.o] Error 1 So I guess you didn't test with 64-bit? You should at least test with both 32-bit and 64-bit on QEMU, and, ideally, also test both on KVM and also 64-bit EFI on QEMU. I just tried 32-bit KVM and see that the DBCN write test fails the 'write success' test. That may be a KVM bug. Thanks, drew
On Wed, Aug 07, 2024 at 01:36:33PM GMT, Andrew Jones wrote: ... > I just tried 32-bit KVM and see that the DBCN write test fails the > 'write success' test. That may be a KVM bug. > We can blame both KVM and kvmtool. KVM sets sbiret.error to a0 and sbiret.value to a1 before exiting to userspace[1]. I think that comes from thinking about how a real ecall would set them. However, as this isn't an ecall, they should get set directly by userspace, not through registers. Also, we should initialize them to some known value before calling userspace, and zero is probably the best choice. kvmtool neglects to set sbiret.error to SBI_SUCCESS on a successful write. QEMU does set it, so this failure shouldn't happen with QEMU, but I haven't tried it. Patching both KVM and kvmtool is best, as it would allow the test to pass when running new KVM with old kvmtool and when running old KVM with new kvmtool. [1] arch/riscv/kvm/vcpu_sbi.c:130 Thanks, drew
On Wed, Aug 07, 2024 at 02:29:31PM GMT, Andrew Jones wrote: > On Wed, Aug 07, 2024 at 01:36:33PM GMT, Andrew Jones wrote: > ... > > I just tried 32-bit KVM and see that the DBCN write test fails the > > 'write success' test. That may be a KVM bug. > > > > We can blame both KVM and kvmtool. > > KVM sets sbiret.error to a0 and sbiret.value to a1 before exiting to > userspace[1]. I think that comes from thinking about how a real ecall > would set them. However, as this isn't an ecall, they should get set > directly by userspace, not through registers. Also, we should initialize > them to some known value before calling userspace, and zero is probably > the best choice. Thinking about this some more and discussing it with Anup, the best choice for sbiret.error is to initialize it to SBI_ERR_NOT_SUPPORTED. Doing that won't allow the test to pass with old kvmtool, but that's OK. I'll send a KVM patch for that and the kvmtool patch. Thanks, drew > > kvmtool neglects to set sbiret.error to SBI_SUCCESS on a successful write. > QEMU does set it, so this failure shouldn't happen with QEMU, but I > haven't tried it. > > Patching both KVM and kvmtool is best, as it would allow the test to pass > when running new KVM with old kvmtool and when running old KVM with new > kvmtool. > > [1] arch/riscv/kvm/vcpu_sbi.c:130 > > Thanks, > drew > > -- > kvm-riscv mailing list > kvm-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kvm-riscv
Yet another comment on this patch is that the subject is missing the 'v4' prefix. Anyway, no need to send a v5. I had 10 free minutes so I just went a head and made all the simple changes myself and queued it. drew On Tue, Aug 06, 2024 at 10:51:54PM GMT, Cade Richard wrote: > > > --- > Added a unit test for the RISC-V SBI debug console write() and write_byte() functions. The output of the tests must be inspected manually to verify that the correct bytes are written. For write(), the expected output is 'DBCN_WRITE_TEST_STRING'. For write_byte(), the expected output is 'a'. > > Signed-off-by: Cade Richard <cade.richard@berkeley.edu> > --- > lib/riscv/asm/sbi.h | 7 ++++++ > riscv/sbi.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 73 insertions(+) > > diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h > index 73ab5438..47e91025 100644 > --- a/lib/riscv/asm/sbi.h > +++ b/lib/riscv/asm/sbi.h > @@ -19,6 +19,7 @@ enum sbi_ext_id { > SBI_EXT_TIME = 0x54494d45, > SBI_EXT_HSM = 0x48534d, > SBI_EXT_SRST = 0x53525354, > + SBI_EXT_DBCN = 0x4442434E, > }; > > enum sbi_ext_base_fid { > @@ -42,6 +43,12 @@ enum sbi_ext_time_fid { > SBI_EXT_TIME_SET_TIMER = 0, > }; > > +enum sbi_ext_dbcn_fid { > + SBI_EXT_DBCN_CONSOLE_WRITE = 0, > + SBI_EXT_DBCN_CONSOLE_READ, > + SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, > +}; > + > struct sbiret { > long error; > long value; > diff --git a/riscv/sbi.c b/riscv/sbi.c > index 2438c497..61993f08 100644 > --- a/riscv/sbi.c > +++ b/riscv/sbi.c > @@ -15,6 +15,10 @@ > #include <asm/sbi.h> > #include <asm/smp.h> > #include <asm/timer.h> > +#include <asm/io.h> > + > +#define DBCN_WRITE_TEST_STRING "DBCN_WRITE_TEST_STRING\n" > +#define DBCN_WRITE_BYTE_TEST_BYTE (u8)'a' > > static void help(void) > { > @@ -32,6 +36,11 @@ static struct sbiret __time_sbi_ecall(unsigned long stime_value) > return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0); > } > > +static struct sbiret __dbcn_sbi_ecall(int fid, unsigned long arg0, unsigned long arg1, unsigned long arg2) > +{ > + return sbi_ecall(SBI_EXT_DBCN, fid, arg0, arg1, arg2, 0, 0, 0); > +} > + > static bool env_or_skip(const char *env) > { > if (!getenv(env)) { > @@ -248,6 +257,62 @@ static void check_time(void) > report_prefix_pop(); > } > > +static void check_dbcn(void) > +{ > + > + struct sbiret ret; > + unsigned long num_bytes, base_addr_lo, base_addr_hi; > + int num_calls = 0; > + > + num_bytes = strlen(DBCN_WRITE_TEST_STRING); > + phys_addr_t p = virt_to_phys((void *)&DBCN_WRITE_TEST_STRING); > + base_addr_lo = (unsigned long)p; > + base_addr_hi = (unsigned long)(p >> __riscv_xlen); > + > + report_prefix_push("dbcn"); > + > + ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_DBCN); > + if (!ret.value) { > + report_skip("DBCN extension unavailable"); > + report_prefix_pop(); > + return; > + } > + > + report_prefix_push("write"); > + > + do { > + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE, num_bytes, base_addr_lo, base_addr_hi); > + num_bytes -= ret.value; > + base_addr_lo += ret.value; > + num_calls++; > + } while (num_bytes != 0 && ret.error == SBI_SUCCESS) ; > + report(ret.error == SBI_SUCCESS, "write success"); > + report_info("%d sbi calls made", num_calls); > + > + /* > + Bytes are read from memory and written to the console > + */ > + if (env_or_skip("INVALID_READ_ADDR")) { > + phys_addr_t p = strtoull(getenv("INVALID_READ_ADDR"), NULL, 0); > + base_addr_lo = (unsigned long)p; > + base_addr_hi = (unsigned long)(p >> __riscv_xlen); > + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE, 1, base_addr_lo, base_addr_hi); > + report(ret.error == SBI_ERR_INVALID_PARAM, "invalid parameter: address"); > + }; > + > + report_prefix_pop(); > + > + report_prefix_push("write_byte"); > + > + puts("DBCN_WRITE TEST CHAR: "); > + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, (u8)DBCN_WRITE_BYTE_TEST_BYTE, 0, 0); > + puts("\n"); > + report(ret.error == SBI_SUCCESS, "write success"); > + report(ret.value == 0, "expected ret.value"); > + > + report_prefix_pop(); > +} > + > int main(int argc, char **argv) > { > > @@ -259,6 +324,7 @@ int main(int argc, char **argv) > report_prefix_push("sbi"); > check_base(); > check_time(); > + check_dbcn(); > > return report_summary(); > } > > --- > base-commit: 1878b4b663fd50b87de7ba2b1c90614e2703542f > change-id: 20240806-sbi-dbcn-write-test-70d305d511cf > > Best regards, > -- > Cade Richard <cade.richard@berkeley.edu> > > > -- > kvm-riscv mailing list > kvm-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kvm-riscv
diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h index 73ab5438..47e91025 100644 --- a/lib/riscv/asm/sbi.h +++ b/lib/riscv/asm/sbi.h @@ -19,6 +19,7 @@ enum sbi_ext_id { SBI_EXT_TIME = 0x54494d45, SBI_EXT_HSM = 0x48534d, SBI_EXT_SRST = 0x53525354, + SBI_EXT_DBCN = 0x4442434E, }; enum sbi_ext_base_fid { @@ -42,6 +43,12 @@ enum sbi_ext_time_fid { SBI_EXT_TIME_SET_TIMER = 0, }; +enum sbi_ext_dbcn_fid { + SBI_EXT_DBCN_CONSOLE_WRITE = 0, + SBI_EXT_DBCN_CONSOLE_READ, + SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, +}; + struct sbiret { long error; long value; diff --git a/riscv/sbi.c b/riscv/sbi.c index 2438c497..61993f08 100644 --- a/riscv/sbi.c +++ b/riscv/sbi.c @@ -15,6 +15,10 @@ #include <asm/sbi.h> #include <asm/smp.h> #include <asm/timer.h> +#include <asm/io.h> + +#define DBCN_WRITE_TEST_STRING "DBCN_WRITE_TEST_STRING\n" +#define DBCN_WRITE_BYTE_TEST_BYTE (u8)'a' static void help(void) { @@ -32,6 +36,11 @@ static struct sbiret __time_sbi_ecall(unsigned long stime_value) return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0); } +static struct sbiret __dbcn_sbi_ecall(int fid, unsigned long arg0, unsigned long arg1, unsigned long arg2) +{ + return sbi_ecall(SBI_EXT_DBCN, fid, arg0, arg1, arg2, 0, 0, 0); +} + static bool env_or_skip(const char *env) { if (!getenv(env)) { @@ -248,6 +257,62 @@ static void check_time(void) report_prefix_pop(); } +static void check_dbcn(void) +{ + + struct sbiret ret; + unsigned long num_bytes, base_addr_lo, base_addr_hi; + int num_calls = 0; + + num_bytes = strlen(DBCN_WRITE_TEST_STRING); + phys_addr_t p = virt_to_phys((void *)&DBCN_WRITE_TEST_STRING); + base_addr_lo = (unsigned long)p; + base_addr_hi = (unsigned long)(p >> __riscv_xlen); + + report_prefix_push("dbcn"); + + ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_DBCN); + if (!ret.value) { + report_skip("DBCN extension unavailable"); + report_prefix_pop(); + return; + } + + report_prefix_push("write"); + + do { + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE, num_bytes, base_addr_lo, base_addr_hi); + num_bytes -= ret.value; + base_addr_lo += ret.value; + num_calls++; + } while (num_bytes != 0 && ret.error == SBI_SUCCESS) ; + report(ret.error == SBI_SUCCESS, "write success"); + report_info("%d sbi calls made", num_calls); + + /* + Bytes are read from memory and written to the console + */ + if (env_or_skip("INVALID_READ_ADDR")) { + phys_addr_t p = strtoull(getenv("INVALID_READ_ADDR"), NULL, 0); + base_addr_lo = (unsigned long)p; + base_addr_hi = (unsigned long)(p >> __riscv_xlen); + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE, 1, base_addr_lo, base_addr_hi); + report(ret.error == SBI_ERR_INVALID_PARAM, "invalid parameter: address"); + }; + + report_prefix_pop(); + + report_prefix_push("write_byte"); + + puts("DBCN_WRITE TEST CHAR: "); + ret = __dbcn_sbi_ecall(SBI_EXT_DBCN_CONSOLE_WRITE_BYTE, (u8)DBCN_WRITE_BYTE_TEST_BYTE, 0, 0); + puts("\n"); + report(ret.error == SBI_SUCCESS, "write success"); + report(ret.value == 0, "expected ret.value"); + + report_prefix_pop(); +} + int main(int argc, char **argv) { @@ -259,6 +324,7 @@ int main(int argc, char **argv) report_prefix_push("sbi"); check_base(); check_time(); + check_dbcn(); return report_summary(); }
--- Added a unit test for the RISC-V SBI debug console write() and write_byte() functions. The output of the tests must be inspected manually to verify that the correct bytes are written. For write(), the expected output is 'DBCN_WRITE_TEST_STRING'. For write_byte(), the expected output is 'a'. Signed-off-by: Cade Richard <cade.richard@berkeley.edu> --- lib/riscv/asm/sbi.h | 7 ++++++ riscv/sbi.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) --- base-commit: 1878b4b663fd50b87de7ba2b1c90614e2703542f change-id: 20240806-sbi-dbcn-write-test-70d305d511cf Best regards,