[kvm-unit-tests PATCH 0/2] s390x: run script fixes for PV tests

[kvm-unit-tests PATCH 0/2] s390x: run script fixes for PV tests

[Nicholas Piggin]

Shellcheck caught what looks like two bugs. May not have been too
noticable because one just seems like it allows TCG to run PV tests
which fails (and might be guarded out later anyway), and the other
might only affect running and individual .pv.bin test with s390x-run
and not unittests runs.

I don't know about running PV tests or have a system with KVM to try,
so these are not tested beyond gitlab CI. That might run some PV tests
with s390x-kvm, but I don't know so please test/review/ack/nack if
possible.

Thanks,
Nick

Nicholas Piggin (2):
  s390x: Fix misspelt variable name in func.bash
  s390x: Fix is_pv check in run script

 s390x/run               | 8 ++++----
 scripts/s390x/func.bash | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.43.0

[kvm-unit-tests PATCH 1/2] s390x: Fix misspelt variable name in func.bash

[Nicholas Piggin]

The if statement is intended to run non-migration tests with PV on KVM.
With the misspelling, they are run on KVM or TCG.

Reported-by: shellcheck SC2153
Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/s390x/func.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
index 6c75e89ae..fa47d0191 100644
--- a/scripts/s390x/func.bash
+++ b/scripts/s390x/func.bash
@@ -21,7 +21,7 @@ function arch_cmd_s390x()
 	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 
 	# run PV test case
-	if [ "$ACCEL" = 'tcg' ] || grep -q "migration" <<< "$groups"; then
+	if [ "$accel" = 'tcg' ] || grep -q "migration" <<< "$groups"; then
 		return
 	fi
 	kernel=${kernel%.elf}.pv.bin
-- 
2.43.0

Re: [kvm-unit-tests PATCH 1/2] s390x: Fix misspelt variable name in func.bash

[Janosch Frank]

On 4/6/24 14:24, Nicholas Piggin wrote:
> The if statement is intended to run non-migration tests with PV on KVM.
> With the misspelling, they are run on KVM or TCG.
> 

It's not misspelt, is it?
It's in the wrong case.


I'm fine with the code though.

Re: [kvm-unit-tests PATCH 1/2] s390x: Fix misspelt variable name in func.bash

[Nicholas Piggin]

On Mon Apr 8, 2024 at 9:59 PM AEST, Janosch Frank wrote:
> On 4/6/24 14:24, Nicholas Piggin wrote:
> > The if statement is intended to run non-migration tests with PV on KVM.
> > With the misspelling, they are run on KVM or TCG.
> > 
>
> It's not misspelt, is it?
> It's in the wrong case.

Yes, that's the right word.

>
>
> I'm fine with the code though.

Thanks, I'll take that as an Acked-by: you

Thanks,
Nick

Re: [kvm-unit-tests PATCH 1/2] s390x: Fix misspelt variable name in func.bash

[Janosch Frank]

On 4/10/24 06:35, Nicholas Piggin wrote:
> On Mon Apr 8, 2024 at 9:59 PM AEST, Janosch Frank wrote:
>> On 4/6/24 14:24, Nicholas Piggin wrote:
>>> The if statement is intended to run non-migration tests with PV on KVM.
>>> With the misspelling, they are run on KVM or TCG.
>>>
>>
>> It's not misspelt, is it?
>> It's in the wrong case.
> 
> Yes, that's the right word.
> 
>>
>>
>> I'm fine with the code though.
> 
> Thanks, I'll take that as an Acked-by: you

Could you send out a fixed version that I can pick or do you want me to 
fix that up?

Re: [kvm-unit-tests PATCH 1/2] s390x: Fix misspelt variable name in func.bash

[Nicholas Piggin]

On Thu Apr 11, 2024 at 7:40 PM AEST, Janosch Frank wrote:
> On 4/10/24 06:35, Nicholas Piggin wrote:
> > On Mon Apr 8, 2024 at 9:59 PM AEST, Janosch Frank wrote:
> >> On 4/6/24 14:24, Nicholas Piggin wrote:
> >>> The if statement is intended to run non-migration tests with PV on KVM.
> >>> With the misspelling, they are run on KVM or TCG.
> >>>
> >>
> >> It's not misspelt, is it?
> >> It's in the wrong case.
> > 
> > Yes, that's the right word.
> > 
> >>
> >>
> >> I'm fine with the code though.
> > 
> > Thanks, I'll take that as an Acked-by: you
>
> Could you send out a fixed version that I can pick or do you want me to 
> fix that up?

I was going to at some point, but was juggling a bunch of other
things and have some travel and vacation. You are welcome to take
over them if you like it would be helpful.

Thanks,
Nick

Re: [kvm-unit-tests PATCH 1/2] s390x: Fix misspelt variable name in func.bash

[Janosch Frank]

On 4/16/24 04:29, Nicholas Piggin wrote:
> On Thu Apr 11, 2024 at 7:40 PM AEST, Janosch Frank wrote:
>> On 4/10/24 06:35, Nicholas Piggin wrote:
>>> On Mon Apr 8, 2024 at 9:59 PM AEST, Janosch Frank wrote:
>>>> On 4/6/24 14:24, Nicholas Piggin wrote:
>>>>> The if statement is intended to run non-migration tests with PV on KVM.
>>>>> With the misspelling, they are run on KVM or TCG.
>>>>>
>>>>
>>>> It's not misspelt, is it?
>>>> It's in the wrong case.
>>>
>>> Yes, that's the right word.
>>>
>>>>
>>>>
>>>> I'm fine with the code though.
>>>
>>> Thanks, I'll take that as an Acked-by: you
>>
>> Could you send out a fixed version that I can pick or do you want me to
>> fix that up?
> 
> I was going to at some point, but was juggling a bunch of other
> things and have some travel and vacation. You are welcome to take
> over them if you like it would be helpful.
> 

Same here, I'll be on vacation starting next week so I've prepared a 
branch with your patches for Nico and Claudio which they can send out 
when I'm on vacation.

https://gitlab.com/frankja/kvm-unit-tests/-/commits/queue/?ref_type=heads

[kvm-unit-tests PATCH 2/2] s390x: Fix is_pv check in run script

[Nicholas Piggin]

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>
---
 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
 	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"
-- 
2.43.0

Re: [kvm-unit-tests PATCH 2/2] s390x: Fix is_pv check in run script

[Claudio Imbrenda]

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"

Re: [kvm-unit-tests PATCH 2/2] s390x: Fix is_pv check in run script

[Nicholas Piggin]

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"

Re: [kvm-unit-tests PATCH 2/2] s390x: Fix is_pv check in run script

[Claudio Imbrenda]

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"  
> 

Re: [kvm-unit-tests PATCH 2/2] s390x: Fix is_pv check in run script

[Janosch Frank]

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>

Re: [kvm-unit-tests PATCH 2/2] s390x: Fix is_pv check in run script

[Nicholas Piggin]

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>

Re: [kvm-unit-tests PATCH 0/2] s390x: run script fixes for PV tests

[Janosch Frank]

On 4/6/24 14:24, Nicholas Piggin wrote:
> Shellcheck caught what looks like two bugs. May not have been too
> noticable because one just seems like it allows TCG to run PV tests
> which fails (and might be guarded out later anyway), and the other
> might only affect running and individual .pv.bin test with s390x-run
> and not unittests runs.
> 
> I don't know about running PV tests or have a system with KVM to try,
> so these are not tested beyond gitlab CI. That might run some PV tests
> with s390x-kvm, but I don't know so please test/review/ack/nack if
> possible.
> 
> Thanks,
> Nick

Hey Nick, thanks for cleaning this up.
The changes work on my machine and I've thrown them into our CI.

Give me a sec to check the patches itself.