| Message ID | 20230710121543.197250-14-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/21] hw/s390x: Move KVM specific PV from hw/ to target/s390x/kvm/ | expand |
On 7/10/23 13:15, Thomas Huth wrote: > From: Ilya Leoshkevich <iii@linux.ibm.com> > > Add a small test to prevent regressions. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > Reviewed-by: David Hildenbrand <david@redhat.com> > Message-Id: <20230704081506.276055-13-iii@linux.ibm.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > tests/tcg/s390x/mie3-mvcrl.c | 46 ++++++++++++++++++++++++++++-------- > 1 file changed, 36 insertions(+), 10 deletions(-) > > diff --git a/tests/tcg/s390x/mie3-mvcrl.c b/tests/tcg/s390x/mie3-mvcrl.c > index 93c7b0a290..ec78dd1d49 100644 > --- a/tests/tcg/s390x/mie3-mvcrl.c > +++ b/tests/tcg/s390x/mie3-mvcrl.c > @@ -1,29 +1,55 @@ > +#include <stdbool.h> > #include <stdint.h> > +#include <stdlib.h> > #include <string.h> > > - > -static inline void mvcrl_8(const char *dst, const char *src) > +static void mvcrl(const char *dst, const char *src, size_t len) > { > + register long r0 asm("r0") = len; > + > asm volatile ( > - "llill %%r0, 8\n" > ".insn sse, 0xE50A00000000, 0(%[dst]), 0(%[src])" > - : : [dst] "d" (dst), [src] "d" (src) > - : "r0", "memory"); > + : : [dst] "d" (dst), [src] "d" (src), "r" (r0) > + : "memory"); > } > > - > -int main(int argc, char *argv[]) > +static bool test(void) > { > const char *alpha = "abcdefghijklmnop"; > > /* array missing 'i' */ > - char tstr[17] = "abcdefghjklmnop\0" ; > + char tstr[17] = "abcdefghjklmnop\0"; > > /* mvcrl reference use: 'open a hole in an array' */ > - mvcrl_8(tstr + 9, tstr + 8); > + mvcrl(tstr + 9, tstr + 8, 8); > > /* place missing 'i' */ > tstr[8] = 'i'; > > - return strncmp(alpha, tstr, 16ul); > + return strncmp(alpha, tstr, 16ul) == 0; > +} > + > +static bool test_bad_r0(void) > +{ > + char src[256]; > + > + /* > + * PoP says: Bits 32-55 of general register 0 should contain zeros; > + * otherwise, the program may not operate compatibly in the future. > + * > + * Try it anyway in order to check whether this would crash QEMU itself. > + */ > + mvcrl(src, src, (size_t)-1); > + > + return true; > +} gcc 11 doesn't like this: https://gitlab.com/qemu-project/qemu/-/jobs/4623964826#L3921 /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/tests/tcg/s390x/mie3-mvcrl.c: In function ‘test_bad_r0’: /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/tests/tcg/s390x/mie3-mvcrl.c:42:5: error: ‘src’ may be used uninitialized [-Werror=maybe-uninitialized] 42 | mvcrl(src, src, (size_t)-1); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/tests/tcg/s390x/mie3-mvcrl.c:6:13: note: by argument 1 of type ‘const char *’ to ‘mvcrl’ declared here 6 | static void mvcrl(const char *dst, const char *src, size_t len) | ^~~~~ /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/tests/tcg/s390x/mie3-mvcrl.c:34:10: note: ‘src’ declared here 34 | char src[256]; | ^~~ /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/tests/tcg/s390x/mie3-mvcrl.c:42:5: error: ‘src’ may be used uninitialized [-Werror=maybe-uninitialized] 42 | mvcrl(src, src, (size_t)-1); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/tests/tcg/s390x/mie3-mvcrl.c:6:13: note: by argument 2 of type ‘const char *’ to ‘mvcrl’ declared here 6 | static void mvcrl(const char *dst, const char *src, size_t len) | ^~~~~ /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/tests/tcg/s390x/mie3-mvcrl.c:34:10: note: ‘src’ declared here 34 | char src[256]; | ^~~ cc1: all warnings being treated as errors How it sees any use of the structure, initialized or otherwise, I don't know -- it's all hidden within the asm. However, src[256] = { } is enough to silence the error. r~
On Mon, 2023-07-10 at 14:09 +0100, Richard Henderson wrote: > On 7/10/23 13:15, Thomas Huth wrote: > > From: Ilya Leoshkevich <iii@linux.ibm.com> > > > > Add a small test to prevent regressions. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > Reviewed-by: David Hildenbrand <david@redhat.com> > > Message-Id: <20230704081506.276055-13-iii@linux.ibm.com> > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > tests/tcg/s390x/mie3-mvcrl.c | 46 ++++++++++++++++++++++++++++--- > > ----- > > 1 file changed, 36 insertions(+), 10 deletions(-) > > > > diff --git a/tests/tcg/s390x/mie3-mvcrl.c b/tests/tcg/s390x/mie3- > > mvcrl.c > > index 93c7b0a290..ec78dd1d49 100644 > > --- a/tests/tcg/s390x/mie3-mvcrl.c > > +++ b/tests/tcg/s390x/mie3-mvcrl.c > > @@ -1,29 +1,55 @@ > > +#include <stdbool.h> > > #include <stdint.h> > > +#include <stdlib.h> > > #include <string.h> > > > > - > > -static inline void mvcrl_8(const char *dst, const char *src) > > +static void mvcrl(const char *dst, const char *src, size_t len) > > { > > + register long r0 asm("r0") = len; > > + > > asm volatile ( > > - "llill %%r0, 8\n" > > ".insn sse, 0xE50A00000000, 0(%[dst]), 0(%[src])" > > - : : [dst] "d" (dst), [src] "d" (src) > > - : "r0", "memory"); > > + : : [dst] "d" (dst), [src] "d" (src), "r" (r0) > > + : "memory"); > > } > > > > - > > -int main(int argc, char *argv[]) > > +static bool test(void) > > { > > const char *alpha = "abcdefghijklmnop"; > > > > /* array missing 'i' */ > > - char tstr[17] = "abcdefghjklmnop\0" ; > > + char tstr[17] = "abcdefghjklmnop\0"; > > > > /* mvcrl reference use: 'open a hole in an array' */ > > - mvcrl_8(tstr + 9, tstr + 8); > > + mvcrl(tstr + 9, tstr + 8, 8); > > > > /* place missing 'i' */ > > tstr[8] = 'i'; > > > > - return strncmp(alpha, tstr, 16ul); > > + return strncmp(alpha, tstr, 16ul) == 0; > > +} > > + > > +static bool test_bad_r0(void) > > +{ > > + char src[256]; > > + > > + /* > > + * PoP says: Bits 32-55 of general register 0 should contain > > zeros; > > + * otherwise, the program may not operate compatibly in the > > future. > > + * > > + * Try it anyway in order to check whether this would crash > > QEMU itself. > > + */ > > + mvcrl(src, src, (size_t)-1); > > + > > + return true; > > +} > > gcc 11 doesn't like this: > > https://gitlab.com/qemu-project/qemu/-/jobs/4623964826#L3921 > > /home/gitlab-runner/builds/E8PpwMky/0/qemu- > project/qemu/tests/tcg/s390x/mie3-mvcrl.c: In > function ‘test_bad_r0’: > /home/gitlab-runner/builds/E8PpwMky/0/qemu- > project/qemu/tests/tcg/s390x/mie3-mvcrl.c:42:5: > error: ‘src’ may be used uninitialized [-Werror=maybe-uninitialized] > 42 | mvcrl(src, src, (size_t)-1); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > /home/gitlab-runner/builds/E8PpwMky/0/qemu- > project/qemu/tests/tcg/s390x/mie3-mvcrl.c:6:13: > note: by argument 1 of type ‘const char *’ to ‘mvcrl’ declared here > 6 | static void mvcrl(const char *dst, const char *src, size_t > len) > | ^~~~~ > /home/gitlab-runner/builds/E8PpwMky/0/qemu- > project/qemu/tests/tcg/s390x/mie3-mvcrl.c:34:10: > note: ‘src’ declared here > 34 | char src[256]; > | ^~~ > /home/gitlab-runner/builds/E8PpwMky/0/qemu- > project/qemu/tests/tcg/s390x/mie3-mvcrl.c:42:5: > error: ‘src’ may be used uninitialized [-Werror=maybe-uninitialized] > 42 | mvcrl(src, src, (size_t)-1); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > /home/gitlab-runner/builds/E8PpwMky/0/qemu- > project/qemu/tests/tcg/s390x/mie3-mvcrl.c:6:13: > note: by argument 2 of type ‘const char *’ to ‘mvcrl’ declared here > 6 | static void mvcrl(const char *dst, const char *src, size_t > len) > | ^~~~~ > /home/gitlab-runner/builds/E8PpwMky/0/qemu- > project/qemu/tests/tcg/s390x/mie3-mvcrl.c:34:10: > note: ‘src’ declared here > 34 | char src[256]; > | ^~~ > cc1: all warnings being treated as errors > > How it sees any use of the structure, initialized or otherwise, I > don't know -- it's all > hidden within the asm. However, src[256] = { } is enough to silence > the error. > > > r~ Thanks for having a look at this. I assume you applied this fixup, and I don't need to send a follow-up patch?
On 10/07/2023 15.13, Ilya Leoshkevich wrote: > On Mon, 2023-07-10 at 14:09 +0100, Richard Henderson wrote: >> On 7/10/23 13:15, Thomas Huth wrote: >>> From: Ilya Leoshkevich <iii@linux.ibm.com> >>> >>> Add a small test to prevent regressions. >>> >>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> >>> Reviewed-by: David Hildenbrand <david@redhat.com> >>> Message-Id: <20230704081506.276055-13-iii@linux.ibm.com> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> tests/tcg/s390x/mie3-mvcrl.c | 46 ++++++++++++++++++++++++++++--- >>> ----- >>> 1 file changed, 36 insertions(+), 10 deletions(-) >>> >>> diff --git a/tests/tcg/s390x/mie3-mvcrl.c b/tests/tcg/s390x/mie3- >>> mvcrl.c >>> index 93c7b0a290..ec78dd1d49 100644 >>> --- a/tests/tcg/s390x/mie3-mvcrl.c >>> +++ b/tests/tcg/s390x/mie3-mvcrl.c >>> @@ -1,29 +1,55 @@ >>> +#include <stdbool.h> >>> #include <stdint.h> >>> +#include <stdlib.h> >>> #include <string.h> >>> >>> - >>> -static inline void mvcrl_8(const char *dst, const char *src) >>> +static void mvcrl(const char *dst, const char *src, size_t len) >>> { >>> + register long r0 asm("r0") = len; >>> + >>> asm volatile ( >>> - "llill %%r0, 8\n" >>> ".insn sse, 0xE50A00000000, 0(%[dst]), 0(%[src])" >>> - : : [dst] "d" (dst), [src] "d" (src) >>> - : "r0", "memory"); >>> + : : [dst] "d" (dst), [src] "d" (src), "r" (r0) >>> + : "memory"); >>> } >>> >>> - >>> -int main(int argc, char *argv[]) >>> +static bool test(void) >>> { >>> const char *alpha = "abcdefghijklmnop"; >>> >>> /* array missing 'i' */ >>> - char tstr[17] = "abcdefghjklmnop\0" ; >>> + char tstr[17] = "abcdefghjklmnop\0"; >>> >>> /* mvcrl reference use: 'open a hole in an array' */ >>> - mvcrl_8(tstr + 9, tstr + 8); >>> + mvcrl(tstr + 9, tstr + 8, 8); >>> >>> /* place missing 'i' */ >>> tstr[8] = 'i'; >>> >>> - return strncmp(alpha, tstr, 16ul); >>> + return strncmp(alpha, tstr, 16ul) == 0; >>> +} >>> + >>> +static bool test_bad_r0(void) >>> +{ >>> + char src[256]; >>> + >>> + /* >>> + * PoP says: Bits 32-55 of general register 0 should contain >>> zeros; >>> + * otherwise, the program may not operate compatibly in the >>> future. >>> + * >>> + * Try it anyway in order to check whether this would crash >>> QEMU itself. >>> + */ >>> + mvcrl(src, src, (size_t)-1); >>> + >>> + return true; >>> +} >> >> gcc 11 doesn't like this: >> >> https://gitlab.com/qemu-project/qemu/-/jobs/4623964826#L3921 >> >> /home/gitlab-runner/builds/E8PpwMky/0/qemu- >> project/qemu/tests/tcg/s390x/mie3-mvcrl.c: In >> function ‘test_bad_r0’: >> /home/gitlab-runner/builds/E8PpwMky/0/qemu- >> project/qemu/tests/tcg/s390x/mie3-mvcrl.c:42:5: >> error: ‘src’ may be used uninitialized [-Werror=maybe-uninitialized] >> 42 | mvcrl(src, src, (size_t)-1); >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >> /home/gitlab-runner/builds/E8PpwMky/0/qemu- >> project/qemu/tests/tcg/s390x/mie3-mvcrl.c:6:13: >> note: by argument 1 of type ‘const char *’ to ‘mvcrl’ declared here >> 6 | static void mvcrl(const char *dst, const char *src, size_t >> len) >> | ^~~~~ >> /home/gitlab-runner/builds/E8PpwMky/0/qemu- >> project/qemu/tests/tcg/s390x/mie3-mvcrl.c:34:10: >> note: ‘src’ declared here >> 34 | char src[256]; >> | ^~~ >> /home/gitlab-runner/builds/E8PpwMky/0/qemu- >> project/qemu/tests/tcg/s390x/mie3-mvcrl.c:42:5: >> error: ‘src’ may be used uninitialized [-Werror=maybe-uninitialized] >> 42 | mvcrl(src, src, (size_t)-1); >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >> /home/gitlab-runner/builds/E8PpwMky/0/qemu- >> project/qemu/tests/tcg/s390x/mie3-mvcrl.c:6:13: >> note: by argument 2 of type ‘const char *’ to ‘mvcrl’ declared here >> 6 | static void mvcrl(const char *dst, const char *src, size_t >> len) >> | ^~~~~ >> /home/gitlab-runner/builds/E8PpwMky/0/qemu- >> project/qemu/tests/tcg/s390x/mie3-mvcrl.c:34:10: >> note: ‘src’ declared here >> 34 | char src[256]; >> | ^~~ >> cc1: all warnings being treated as errors >> >> How it sees any use of the structure, initialized or otherwise, I >> don't know -- it's all >> hidden within the asm. However, src[256] = { } is enough to silence >> the error. >> >> >> r~ > > Thanks for having a look at this. I assume you applied this fixup, and > I don't need to send a follow-up patch? No need to respin the patch, I'll fix it up and send a v2 pull request. Thomas
diff --git a/tests/tcg/s390x/mie3-mvcrl.c b/tests/tcg/s390x/mie3-mvcrl.c index 93c7b0a290..ec78dd1d49 100644 --- a/tests/tcg/s390x/mie3-mvcrl.c +++ b/tests/tcg/s390x/mie3-mvcrl.c @@ -1,29 +1,55 @@ +#include <stdbool.h> #include <stdint.h> +#include <stdlib.h> #include <string.h> - -static inline void mvcrl_8(const char *dst, const char *src) +static void mvcrl(const char *dst, const char *src, size_t len) { + register long r0 asm("r0") = len; + asm volatile ( - "llill %%r0, 8\n" ".insn sse, 0xE50A00000000, 0(%[dst]), 0(%[src])" - : : [dst] "d" (dst), [src] "d" (src) - : "r0", "memory"); + : : [dst] "d" (dst), [src] "d" (src), "r" (r0) + : "memory"); } - -int main(int argc, char *argv[]) +static bool test(void) { const char *alpha = "abcdefghijklmnop"; /* array missing 'i' */ - char tstr[17] = "abcdefghjklmnop\0" ; + char tstr[17] = "abcdefghjklmnop\0"; /* mvcrl reference use: 'open a hole in an array' */ - mvcrl_8(tstr + 9, tstr + 8); + mvcrl(tstr + 9, tstr + 8, 8); /* place missing 'i' */ tstr[8] = 'i'; - return strncmp(alpha, tstr, 16ul); + return strncmp(alpha, tstr, 16ul) == 0; +} + +static bool test_bad_r0(void) +{ + char src[256]; + + /* + * PoP says: Bits 32-55 of general register 0 should contain zeros; + * otherwise, the program may not operate compatibly in the future. + * + * Try it anyway in order to check whether this would crash QEMU itself. + */ + mvcrl(src, src, (size_t)-1); + + return true; +} + +int main(void) +{ + bool ok = true; + + ok &= test(); + ok &= test_bad_r0(); + + return ok ? EXIT_SUCCESS : EXIT_FAILURE; }