| Message ID | 20240406122456.405139-3-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: run script fixes for PV tests | expand |
On Sat, 6 Apr 2024 22:24:54 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > Shellcheck reports "is_pv references arguments, but none are ever > passed." and suggests "use is_pv "$@" if function's $1 should mean > script's $1." > > The is_pv test does not evaluate to true for .pv.bin file names, only > for _PV suffix test names. The arch_cmd_s390x() function appends > .pv.bin to the file name AND _PV to the test name, so this does not > affect run_tests.sh runs, but it might prevent PV tests from being > run directly with the s390x-run command. > > Reported-by: shellcheck SC2119, SC2120 > Reviewed-by: Andrew Jones <andrew.jones@linux.dev> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> Fixes: bcedc5a2 ("s390x: run PV guests with confidential guest enabled") although tbh I would rewrite it to check a variable, something like: IS_PV=no [ "${1: -7}" = ".pv.bin" -o "${TESTNAME: -3}" = "_PV" ] && IS_PV=yes > --- > s390x/run | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/s390x/run b/s390x/run > index e58fa4af9..34552c274 100755 > --- a/s390x/run > +++ b/s390x/run > @@ -21,12 +21,12 @@ is_pv() { > return 1 > } > > -if is_pv && [ "$ACCEL" = "tcg" ]; then > +if is_pv "$@" && [ "$ACCEL" = "tcg" ]; then if [ "$IS_PV" = "yes" -a "$ACCEL" = "tcg" ]; then etc... > echo "Protected Virtualization isn't supported under TCG" > exit 2 > fi > > -if is_pv && [ "$MIGRATION" = "yes" ]; then > +if is_pv "$@" && [ "$MIGRATION" = "yes" ]; then > echo "Migration isn't supported under Protected Virtualization" > exit 2 > fi > @@ -34,12 +34,12 @@ fi > M='-machine s390-ccw-virtio' > M+=",accel=$ACCEL$ACCEL_PROPS" > > -if is_pv; then > +if is_pv "$@"; then > M+=",confidential-guest-support=pv0" > fi > > command="$qemu -nodefaults -nographic $M" > -if is_pv; then > +if is_pv "$@"; then > command+=" -object s390-pv-guest,id=pv0" > fi > command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
On 4/6/24 14:24, Nicholas Piggin wrote: > Shellcheck reports "is_pv references arguments, but none are ever > passed." and suggests "use is_pv "$@" if function's $1 should mean > script's $1." > > The is_pv test does not evaluate to true for .pv.bin file names, only > for _PV suffix test names. The arch_cmd_s390x() function appends > .pv.bin to the file name AND _PV to the test name, so this does not > affect run_tests.sh runs, but it might prevent PV tests from being > run directly with the s390x-run command. > The only thing that changes with this patch is that we get the error message from s390x/run and not from QEMU which complains about the unpack facility (needed for PV) not being available (because TCG does not implement it). And that's likely why we never ran into any problems. Patch looks fine to me: Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
On Mon Apr 8, 2024 at 9:36 PM AEST, Claudio Imbrenda wrote: > On Sat, 6 Apr 2024 22:24:54 +1000 > Nicholas Piggin <npiggin@gmail.com> wrote: > > > Shellcheck reports "is_pv references arguments, but none are ever > > passed." and suggests "use is_pv "$@" if function's $1 should mean > > script's $1." > > > > The is_pv test does not evaluate to true for .pv.bin file names, only > > for _PV suffix test names. The arch_cmd_s390x() function appends > > .pv.bin to the file name AND _PV to the test name, so this does not > > affect run_tests.sh runs, but it might prevent PV tests from being > > run directly with the s390x-run command. > > > > Reported-by: shellcheck SC2119, SC2120 > > Reviewed-by: Andrew Jones <andrew.jones@linux.dev> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > Fixes: bcedc5a2 ("s390x: run PV guests with confidential guest enabled") Thanks. > although tbh I would rewrite it to check a variable, something like: > > IS_PV=no > [ "${1: -7}" = ".pv.bin" -o "${TESTNAME: -3}" = "_PV" ] && IS_PV=yes I don't have a problem if you want to fix it a different way instead. I don't have a good way to test at the moment and this seemed the simplest fix. Shout out if you don't want it going upstream as is. Thanks, Nick > > > --- > > s390x/run | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/s390x/run b/s390x/run > > index e58fa4af9..34552c274 100755 > > --- a/s390x/run > > +++ b/s390x/run > > @@ -21,12 +21,12 @@ is_pv() { > > return 1 > > } > > > > -if is_pv && [ "$ACCEL" = "tcg" ]; then > > +if is_pv "$@" && [ "$ACCEL" = "tcg" ]; then > > if [ "$IS_PV" = "yes" -a "$ACCEL" = "tcg" ]; then > > etc... > > > echo "Protected Virtualization isn't supported under TCG" > > exit 2 > > fi > > > > -if is_pv && [ "$MIGRATION" = "yes" ]; then > > +if is_pv "$@" && [ "$MIGRATION" = "yes" ]; then > > echo "Migration isn't supported under Protected Virtualization" > > exit 2 > > fi > > @@ -34,12 +34,12 @@ fi > > M='-machine s390-ccw-virtio' > > M+=",accel=$ACCEL$ACCEL_PROPS" > > > > -if is_pv; then > > +if is_pv "$@"; then > > M+=",confidential-guest-support=pv0" > > fi > > > > command="$qemu -nodefaults -nographic $M" > > -if is_pv; then > > +if is_pv "$@"; then > > command+=" -object s390-pv-guest,id=pv0" > > fi > > command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
On Mon Apr 8, 2024 at 9:46 PM AEST, Janosch Frank wrote: > On 4/6/24 14:24, Nicholas Piggin wrote: > > Shellcheck reports "is_pv references arguments, but none are ever > > passed." and suggests "use is_pv "$@" if function's $1 should mean > > script's $1." > > > > The is_pv test does not evaluate to true for .pv.bin file names, only > > for _PV suffix test names. The arch_cmd_s390x() function appends > > .pv.bin to the file name AND _PV to the test name, so this does not > > affect run_tests.sh runs, but it might prevent PV tests from being > > run directly with the s390x-run command. > > > > The only thing that changes with this patch is that we get the error > message from s390x/run and not from QEMU which complains about the > unpack facility (needed for PV) not being available (because TCG does > not implement it). > And that's likely why we never ran into any problems. Yeah it did look relatively minor. Thanks for taking a look. Thanks, Nick > > > Patch looks fine to me: > Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
On Wed, 10 Apr 2024 14:34:25 +1000 "Nicholas Piggin" <npiggin@gmail.com> wrote: > On Mon Apr 8, 2024 at 9:36 PM AEST, Claudio Imbrenda wrote: > > On Sat, 6 Apr 2024 22:24:54 +1000 > > Nicholas Piggin <npiggin@gmail.com> wrote: > > > > > Shellcheck reports "is_pv references arguments, but none are ever > > > passed." and suggests "use is_pv "$@" if function's $1 should mean > > > script's $1." > > > > > > The is_pv test does not evaluate to true for .pv.bin file names, only > > > for _PV suffix test names. The arch_cmd_s390x() function appends > > > .pv.bin to the file name AND _PV to the test name, so this does not > > > affect run_tests.sh runs, but it might prevent PV tests from being > > > run directly with the s390x-run command. > > > > > > Reported-by: shellcheck SC2119, SC2120 > > > Reviewed-by: Andrew Jones <andrew.jones@linux.dev> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > > > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > Fixes: bcedc5a2 ("s390x: run PV guests with confidential guest enabled") > > Thanks. > > > although tbh I would rewrite it to check a variable, something like: > > > > IS_PV=no > > [ "${1: -7}" = ".pv.bin" -o "${TESTNAME: -3}" = "_PV" ] && IS_PV=yes > > I don't have a problem if you want to fix it a different way > instead. I don't have a good way to test at the moment and > this seemed the simplest fix. Shout out if you don't want it > going upstream as is. well, I did give you a r-b for the current version of your patch :) it's not so important, as long as it works correctly > > Thanks, > Nick > > > > > > --- > > > s390x/run | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/s390x/run b/s390x/run > > > index e58fa4af9..34552c274 100755 > > > --- a/s390x/run > > > +++ b/s390x/run > > > @@ -21,12 +21,12 @@ is_pv() { > > > return 1 > > > } > > > > > > -if is_pv && [ "$ACCEL" = "tcg" ]; then > > > +if is_pv "$@" && [ "$ACCEL" = "tcg" ]; then > > > > if [ "$IS_PV" = "yes" -a "$ACCEL" = "tcg" ]; then > > > > etc... > > > > > echo "Protected Virtualization isn't supported under TCG" > > > exit 2 > > > fi > > > > > > -if is_pv && [ "$MIGRATION" = "yes" ]; then > > > +if is_pv "$@" && [ "$MIGRATION" = "yes" ]; then > > > echo "Migration isn't supported under Protected Virtualization" > > > exit 2 > > > fi > > > @@ -34,12 +34,12 @@ fi > > > M='-machine s390-ccw-virtio' > > > M+=",accel=$ACCEL$ACCEL_PROPS" > > > > > > -if is_pv; then > > > +if is_pv "$@"; then > > > M+=",confidential-guest-support=pv0" > > > fi > > > > > > command="$qemu -nodefaults -nographic $M" > > > -if is_pv; then > > > +if is_pv "$@"; then > > > command+=" -object s390-pv-guest,id=pv0" > > > fi > > > command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0" >
diff --git a/s390x/run b/s390x/run index e58fa4af9..34552c274 100755 --- a/s390x/run +++ b/s390x/run @@ -21,12 +21,12 @@ is_pv() { return 1 } -if is_pv && [ "$ACCEL" = "tcg" ]; then +if is_pv "$@" && [ "$ACCEL" = "tcg" ]; then echo "Protected Virtualization isn't supported under TCG" exit 2 fi -if is_pv && [ "$MIGRATION" = "yes" ]; then +if is_pv "$@" && [ "$MIGRATION" = "yes" ]; then echo "Migration isn't supported under Protected Virtualization" exit 2 fi @@ -34,12 +34,12 @@ fi M='-machine s390-ccw-virtio' M+=",accel=$ACCEL$ACCEL_PROPS" -if is_pv; then +if is_pv "$@"; then M+=",confidential-guest-support=pv0" fi command="$qemu -nodefaults -nographic $M" -if is_pv; then +if is_pv "$@"; then command+=" -object s390-pv-guest,id=pv0" fi command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"