[kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Gavin Shan]

There are extra properties for accelerators to enable the specific
features. For example, the dirty ring for KVM accelerator can be
enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
extra properties for the accelerators aren't supported. It makes
it's impossible to test the combination of KVM and dirty ring
as the following error message indicates.

  # cd /home/gavin/sandbox/kvm-unit-tests/tests
  # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
    ACCEL=kvm,dirty-ring-size=65536 ./its-migration
     :
  BUILD_HEAD=2fffb37e
  timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
  -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
  -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
  -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
  -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
  qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument

Allow to specify extra properties for accelerators. With this, the
"its-migration" can be tested for the combination of KVM and dirty
ring.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v3: Split $ACCEL to $ACCEL and $ACCEL_PROPS in get_qemu_accelerator()
    and don't print them as output, suggested by Drew.
---
 arm/run               | 12 ++++--------
 powerpc/run           |  5 ++---
 s390x/run             |  5 ++---
 scripts/arch-run.bash | 21 +++++++++++++--------
 x86/run               |  5 ++---
 5 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/arm/run b/arm/run
index c6f25b8..d9ebe59 100755
--- a/arm/run
+++ b/arm/run
@@ -10,10 +10,8 @@ if [ -z "$KUT_STANDALONE" ]; then
 fi
 processor="$PROCESSOR"
 
-accel=$(get_qemu_accelerator) ||
-	exit $?
-
-if [ "$accel" = "kvm" ]; then
+get_qemu_accelerator || exit $?
+if [ "$ACCEL" = "kvm" ]; then
 	QEMU_ARCH=$HOST
 fi
 
@@ -23,11 +21,9 @@ qemu=$(search_qemu_binary) ||
 if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
    [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
    [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
-	accel=tcg
+	ACCEL="tcg"
 fi
 
-ACCEL=$accel
-
 if ! $qemu -machine '?' | grep -q 'ARM Virtual Machine'; then
 	echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
 	exit 2
@@ -72,7 +68,7 @@ if $qemu $M -device '?' | grep -q pci-testdev; then
 	pci_testdev="-device pci-testdev"
 fi
 
-A="-accel $ACCEL"
+A="-accel $ACCEL$ACCEL_PROPS"
 command="$qemu -nodefaults $M $A -cpu $processor $chr_testdev $pci_testdev"
 command+=" -display none -serial stdio -kernel"
 command="$(migration_cmd) $(timeout_cmd) $command"
diff --git a/powerpc/run b/powerpc/run
index ee38e07..3988e72 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -9,8 +9,7 @@ if [ -z "$KUT_STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_qemu_accelerator) ||
-	exit $?
+get_qemu_accelerator || exit $?
 
 qemu=$(search_qemu_binary) ||
 	exit $?
@@ -21,7 +20,7 @@ if ! $qemu -machine '?' 2>&1 | grep 'pseries' > /dev/null; then
 fi
 
 M='-machine pseries'
-M+=",accel=$ACCEL"
+M+=",accel=$ACCEL$ACCEL_PROPS"
 command="$qemu -nodefaults $M -bios $FIRMWARE"
 command+=" -display none -serial stdio -kernel"
 command="$(migration_cmd) $(timeout_cmd) $command"
diff --git a/s390x/run b/s390x/run
index f1111db..c57862f 100755
--- a/s390x/run
+++ b/s390x/run
@@ -9,8 +9,7 @@ if [ -z "$KUT_STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_qemu_accelerator) ||
-	exit $?
+get_qemu_accelerator || exit $?
 
 qemu=$(search_qemu_binary) ||
 	exit $?
@@ -26,7 +25,7 @@ if [ "${1: -7}" = ".pv.bin" ] || [ "${TESTNAME: -3}" = "_PV" ] && [ "$MIGRATION"
 fi
 
 M='-machine s390-ccw-virtio'
-M+=",accel=$ACCEL"
+M+=",accel=$ACCEL$ACCEL_PROPS"
 command="$qemu -nodefaults -nographic $M"
 command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
 command+=" -kernel"
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 51e4b97..19c16c1 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -412,6 +412,9 @@ hvf_available ()
 
 get_qemu_accelerator ()
 {
+	ACCEL_PROPS=${ACCEL#"${ACCEL%%,*}"}
+	ACCEL=${ACCEL%%,*}
+
 	if [ "$ACCEL" = "kvm" ] && ! kvm_available; then
 		echo "KVM is needed, but not available on this host" >&2
 		return 2
@@ -421,13 +424,15 @@ get_qemu_accelerator ()
 		return 2
 	fi
 
-	if [ "$ACCEL" ]; then
-		echo $ACCEL
-	elif kvm_available; then
-		echo kvm
-	elif hvf_available; then
-		echo hvf
-	else
-		echo tcg
+	if [ -z "$ACCEL" ]; then
+		if kvm_available; then
+			ACCEL="kvm"
+		elif hvf_available; then
+			ACCEL="hvf"
+		else
+			ACCEL="tcg"
+		fi
 	fi
+
+	return 0
 }
diff --git a/x86/run b/x86/run
index 4d53b72..9a10703 100755
--- a/x86/run
+++ b/x86/run
@@ -9,8 +9,7 @@ if [ -z "$KUT_STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_qemu_accelerator) ||
-	exit $?
+get_qemu_accelerator || exit $?
 
 qemu=$(search_qemu_binary) ||
 	exit $?
@@ -38,7 +37,7 @@ else
 fi
 
 command="${qemu} --no-reboot -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev"
-command+=" -machine accel=$ACCEL"
+command+=" -machine accel=$ACCEL$ACCEL_PROPS"
 if [ "${CONFIG_EFI}" != y ]; then
 	command+=" -kernel"
 fi
-- 
2.40.1

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Nico Boehr]

Quoting Gavin Shan (2023-06-15 08:21:48)
> There are extra properties for accelerators to enable the specific
> features. For example, the dirty ring for KVM accelerator can be
> enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
> extra properties for the accelerators aren't supported. It makes
> it's impossible to test the combination of KVM and dirty ring
> as the following error message indicates.
> 
>   # cd /home/gavin/sandbox/kvm-unit-tests/tests
>   # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>     ACCEL=kvm,dirty-ring-size=65536 ./its-migration
>      :
>   BUILD_HEAD=2fffb37e
>   timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>   -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
>   -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
>   -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
>   -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
>   qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
> 
> Allow to specify extra properties for accelerators. With this, the
> "its-migration" can be tested for the combination of KVM and dirty
> ring.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Maybe get_qemu_accelerator could be renamed now, since it doesn't actually "get"
anything, so maybe check_qemu_accelerator?

In any case, I gave it a quick run on s390x with kvm and tcg and nothing seems
to break, hence for the changes in s390x:

Tested-by: Nico Boehr <nrb@linux.ibm.com>
Acked-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Gavin Shan]

Hi Nico,

On 6/15/23 23:39, Nico Boehr wrote:
> Quoting Gavin Shan (2023-06-15 08:21:48)
>> There are extra properties for accelerators to enable the specific
>> features. For example, the dirty ring for KVM accelerator can be
>> enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
>> extra properties for the accelerators aren't supported. It makes
>> it's impossible to test the combination of KVM and dirty ring
>> as the following error message indicates.
>>
>>    # cd /home/gavin/sandbox/kvm-unit-tests/tests
>>    # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>      ACCEL=kvm,dirty-ring-size=65536 ./its-migration
>>       :
>>    BUILD_HEAD=2fffb37e
>>    timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>    -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
>>    -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
>>    -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
>>    -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
>>    qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
>>
>> Allow to specify extra properties for accelerators. With this, the
>> "its-migration" can be tested for the combination of KVM and dirty
>> ring.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> Maybe get_qemu_accelerator could be renamed now, since it doesn't actually "get"
> anything, so maybe check_qemu_accelerator?
> 
> In any case, I gave it a quick run on s390x with kvm and tcg and nothing seems
> to break, hence for the changes in s390x:
> 
> Tested-by: Nico Boehr <nrb@linux.ibm.com>
> Acked-by: Nico Boehr <nrb@linux.ibm.com>
> 

Thanks for a quick try and comment for this. I guess it's fine to keep 
the function name as get_qemu_accelator() because $ACCEL is split into 
$ACCEL and $ACCEL_PROPS inside it, even it don't print the accelerator 
name at return. However, I'm also fine with check_qemu_accelerator(). 
Lets see what's Drew's comment on this and I can post v4 to have the 
modified function name, or an followup patch to modify the function name.

Thanks,
Gavin

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Andrew Jones]

On Fri, Jun 16, 2023 at 10:41:29AM +1000, Gavin Shan wrote:
> Hi Nico,
> 
> On 6/15/23 23:39, Nico Boehr wrote:
> > Quoting Gavin Shan (2023-06-15 08:21:48)
> > > There are extra properties for accelerators to enable the specific
> > > features. For example, the dirty ring for KVM accelerator can be
> > > enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
> > > extra properties for the accelerators aren't supported. It makes
> > > it's impossible to test the combination of KVM and dirty ring
> > > as the following error message indicates.
> > > 
> > >    # cd /home/gavin/sandbox/kvm-unit-tests/tests
> > >    # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> > >      ACCEL=kvm,dirty-ring-size=65536 ./its-migration
> > >       :
> > >    BUILD_HEAD=2fffb37e
> > >    timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> > >    -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
> > >    -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
> > >    -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
> > >    -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
> > >    qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
> > > 
> > > Allow to specify extra properties for accelerators. With this, the
> > > "its-migration" can be tested for the combination of KVM and dirty
> > > ring.
> > > 
> > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > 
> > Maybe get_qemu_accelerator could be renamed now, since it doesn't actually "get"
> > anything, so maybe check_qemu_accelerator?
> > 
> > In any case, I gave it a quick run on s390x with kvm and tcg and nothing seems
> > to break, hence for the changes in s390x:
> > 
> > Tested-by: Nico Boehr <nrb@linux.ibm.com>
> > Acked-by: Nico Boehr <nrb@linux.ibm.com>
> > 
> 
> Thanks for a quick try and comment for this. I guess it's fine to keep the
> function name as get_qemu_accelator() because $ACCEL is split into $ACCEL
> and $ACCEL_PROPS inside it, even it don't print the accelerator name at
> return. However, I'm also fine with check_qemu_accelerator(). Lets see
> what's Drew's comment on this and I can post v4 to have the modified
> function name, or an followup patch to modify the function name.

I suggested naming it set_qemu_accelerator() in the v2 review.

Thanks,
drew

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Andrew Jones]

On Thu, Jun 15, 2023 at 04:21:48PM +1000, Gavin Shan wrote:
> There are extra properties for accelerators to enable the specific
> features. For example, the dirty ring for KVM accelerator can be
> enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
> extra properties for the accelerators aren't supported. It makes
> it's impossible to test the combination of KVM and dirty ring
> as the following error message indicates.
> 
>   # cd /home/gavin/sandbox/kvm-unit-tests/tests
>   # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>     ACCEL=kvm,dirty-ring-size=65536 ./its-migration
>      :
>   BUILD_HEAD=2fffb37e
>   timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>   -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
>   -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
>   -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
>   -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
>   qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
> 
> Allow to specify extra properties for accelerators. With this, the
> "its-migration" can be tested for the combination of KVM and dirty
> ring.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> v3: Split $ACCEL to $ACCEL and $ACCEL_PROPS in get_qemu_accelerator()
>     and don't print them as output, suggested by Drew.
> ---
>  arm/run               | 12 ++++--------
>  powerpc/run           |  5 ++---
>  s390x/run             |  5 ++---
>  scripts/arch-run.bash | 21 +++++++++++++--------
>  x86/run               |  5 ++---
>  5 files changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/arm/run b/arm/run
> index c6f25b8..d9ebe59 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -10,10 +10,8 @@ if [ -z "$KUT_STANDALONE" ]; then
>  fi
>  processor="$PROCESSOR"
>  
> -accel=$(get_qemu_accelerator) ||
> -	exit $?
> -
> -if [ "$accel" = "kvm" ]; then
> +get_qemu_accelerator || exit $?
> +if [ "$ACCEL" = "kvm" ]; then
>  	QEMU_ARCH=$HOST
>  fi
>  
> @@ -23,11 +21,9 @@ qemu=$(search_qemu_binary) ||
>  if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
>     [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
>     [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
> -	accel=tcg
> +	ACCEL="tcg"
>  fi
>  

As I pointed out in the v2 review we can't just s/accel/ACCEL/ without
other changes. Now ACCEL will also be set when the above condition
is checked, making it useless. Please ensure the test case that commit
c7d6c7f00e7c ("arm/run: Use TCG with qemu-system-arm on arm64 systems")
fixed still works with your patch.

Thanks,
drew

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Gavin Shan]

Hi Drew,

On 6/19/23 18:45, Andrew Jones wrote:
> On Thu, Jun 15, 2023 at 04:21:48PM +1000, Gavin Shan wrote:
>> There are extra properties for accelerators to enable the specific
>> features. For example, the dirty ring for KVM accelerator can be
>> enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
>> extra properties for the accelerators aren't supported. It makes
>> it's impossible to test the combination of KVM and dirty ring
>> as the following error message indicates.
>>
>>    # cd /home/gavin/sandbox/kvm-unit-tests/tests
>>    # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>      ACCEL=kvm,dirty-ring-size=65536 ./its-migration
>>       :
>>    BUILD_HEAD=2fffb37e
>>    timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>    -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
>>    -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
>>    -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
>>    -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
>>    qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
>>
>> Allow to specify extra properties for accelerators. With this, the
>> "its-migration" can be tested for the combination of KVM and dirty
>> ring.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> v3: Split $ACCEL to $ACCEL and $ACCEL_PROPS in get_qemu_accelerator()
>>      and don't print them as output, suggested by Drew.
>> ---
>>   arm/run               | 12 ++++--------
>>   powerpc/run           |  5 ++---
>>   s390x/run             |  5 ++---
>>   scripts/arch-run.bash | 21 +++++++++++++--------
>>   x86/run               |  5 ++---
>>   5 files changed, 23 insertions(+), 25 deletions(-)
>>
>> diff --git a/arm/run b/arm/run
>> index c6f25b8..d9ebe59 100755
>> --- a/arm/run
>> +++ b/arm/run
>> @@ -10,10 +10,8 @@ if [ -z "$KUT_STANDALONE" ]; then
>>   fi
>>   processor="$PROCESSOR"
>>   
>> -accel=$(get_qemu_accelerator) ||
>> -	exit $?
>> -
>> -if [ "$accel" = "kvm" ]; then
>> +get_qemu_accelerator || exit $?
>> +if [ "$ACCEL" = "kvm" ]; then
>>   	QEMU_ARCH=$HOST
>>   fi
>>   
>> @@ -23,11 +21,9 @@ qemu=$(search_qemu_binary) ||
>>   if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
>>      [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
>>      [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
>> -	accel=tcg
>> +	ACCEL="tcg"
>>   fi
>>   
> 
> As I pointed out in the v2 review we can't just s/accel/ACCEL/ without
> other changes. Now ACCEL will also be set when the above condition
> is checked, making it useless. Please ensure the test case that commit
> c7d6c7f00e7c ("arm/run: Use TCG with qemu-system-arm on arm64 systems")
> fixed still works with your patch.
> 

Sorry that I missed your comments for v2. In order to make the test case
in c7d6c7f00e7c working, we just need to call set_qemu_accelerator() after
the chunk of code, like below. When $ACCEL is set to "tcg" by the conditional
code, it won't be changed in the following set_qemu_accelerator().

Could you Please confirm if it looks good to you so that I can integrate
the changes to v4 and post it.

arm/run
--------

processor="$PROCESSOR"

if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
    [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
    [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
         ACCEL="tcg"
fi

set_qemu_accelerator || exit $?
if [ "$ACCEL" = "kvm" ]; then
         QEMU_ARCH=$HOST
fi


Thanks,
Gavin

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Gavin Shan]

Hi Drew,

On 6/19/23 18:44, Andrew Jones wrote:
> On Fri, Jun 16, 2023 at 10:41:29AM +1000, Gavin Shan wrote:
>>
>> On 6/15/23 23:39, Nico Boehr wrote:
>>> Quoting Gavin Shan (2023-06-15 08:21:48)
>>>> There are extra properties for accelerators to enable the specific
>>>> features. For example, the dirty ring for KVM accelerator can be
>>>> enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
>>>> extra properties for the accelerators aren't supported. It makes
>>>> it's impossible to test the combination of KVM and dirty ring
>>>> as the following error message indicates.
>>>>
>>>>     # cd /home/gavin/sandbox/kvm-unit-tests/tests
>>>>     # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>>>       ACCEL=kvm,dirty-ring-size=65536 ./its-migration
>>>>        :
>>>>     BUILD_HEAD=2fffb37e
>>>>     timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>>>     -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
>>>>     -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
>>>>     -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
>>>>     -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
>>>>     qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
>>>>
>>>> Allow to specify extra properties for accelerators. With this, the
>>>> "its-migration" can be tested for the combination of KVM and dirty
>>>> ring.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>
>>> Maybe get_qemu_accelerator could be renamed now, since it doesn't actually "get"
>>> anything, so maybe check_qemu_accelerator?
>>>
>>> In any case, I gave it a quick run on s390x with kvm and tcg and nothing seems
>>> to break, hence for the changes in s390x:
>>>
>>> Tested-by: Nico Boehr <nrb@linux.ibm.com>
>>> Acked-by: Nico Boehr <nrb@linux.ibm.com>
>>>
>>
>> Thanks for a quick try and comment for this. I guess it's fine to keep the
>> function name as get_qemu_accelator() because $ACCEL is split into $ACCEL
>> and $ACCEL_PROPS inside it, even it don't print the accelerator name at
>> return. However, I'm also fine with check_qemu_accelerator(). Lets see
>> what's Drew's comment on this and I can post v4 to have the modified
>> function name, or an followup patch to modify the function name.
> 
> I suggested naming it set_qemu_accelerator() in the v2 review.
> 

My bad, it will be renamed to set_qemu_accelerator() in v4 :)

Thanks,
Gavin

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Andrew Jones]

On Tue, Jun 20, 2023 at 02:13:22PM +1000, Gavin Shan wrote:
> Hi Drew,
> 
> On 6/19/23 18:45, Andrew Jones wrote:
> > On Thu, Jun 15, 2023 at 04:21:48PM +1000, Gavin Shan wrote:
> > > There are extra properties for accelerators to enable the specific
> > > features. For example, the dirty ring for KVM accelerator can be
> > > enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
> > > extra properties for the accelerators aren't supported. It makes
> > > it's impossible to test the combination of KVM and dirty ring
> > > as the following error message indicates.
> > > 
> > >    # cd /home/gavin/sandbox/kvm-unit-tests/tests
> > >    # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> > >      ACCEL=kvm,dirty-ring-size=65536 ./its-migration
> > >       :
> > >    BUILD_HEAD=2fffb37e
> > >    timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> > >    -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
> > >    -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
> > >    -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
> > >    -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
> > >    qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
> > > 
> > > Allow to specify extra properties for accelerators. With this, the
> > > "its-migration" can be tested for the combination of KVM and dirty
> > > ring.
> > > 
> > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > ---
> > > v3: Split $ACCEL to $ACCEL and $ACCEL_PROPS in get_qemu_accelerator()
> > >      and don't print them as output, suggested by Drew.
> > > ---
> > >   arm/run               | 12 ++++--------
> > >   powerpc/run           |  5 ++---
> > >   s390x/run             |  5 ++---
> > >   scripts/arch-run.bash | 21 +++++++++++++--------
> > >   x86/run               |  5 ++---
> > >   5 files changed, 23 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/arm/run b/arm/run
> > > index c6f25b8..d9ebe59 100755
> > > --- a/arm/run
> > > +++ b/arm/run
> > > @@ -10,10 +10,8 @@ if [ -z "$KUT_STANDALONE" ]; then
> > >   fi
> > >   processor="$PROCESSOR"
> > > -accel=$(get_qemu_accelerator) ||
> > > -	exit $?
> > > -
> > > -if [ "$accel" = "kvm" ]; then
> > > +get_qemu_accelerator || exit $?
> > > +if [ "$ACCEL" = "kvm" ]; then
> > >   	QEMU_ARCH=$HOST
> > >   fi
> > > @@ -23,11 +21,9 @@ qemu=$(search_qemu_binary) ||
> > >   if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
> > >      [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
> > >      [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
> > > -	accel=tcg
> > > +	ACCEL="tcg"
> > >   fi
> > 
> > As I pointed out in the v2 review we can't just s/accel/ACCEL/ without
> > other changes. Now ACCEL will also be set when the above condition
> > is checked, making it useless. Please ensure the test case that commit
> > c7d6c7f00e7c ("arm/run: Use TCG with qemu-system-arm on arm64 systems")
> > fixed still works with your patch.
> > 
> 
> Sorry that I missed your comments for v2. In order to make the test case
> in c7d6c7f00e7c working, we just need to call set_qemu_accelerator() after
> the chunk of code, like below. When $ACCEL is set to "tcg" by the conditional
> code, it won't be changed in the following set_qemu_accelerator().
> 
> Could you Please confirm if it looks good to you so that I can integrate
> the changes to v4 and post it.
> 
> arm/run
> --------
> 
> processor="$PROCESSOR"
> 
> if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
>    [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
>    [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
>         ACCEL="tcg"
> fi
> 
> set_qemu_accelerator || exit $?
> if [ "$ACCEL" = "kvm" ]; then
>         QEMU_ARCH=$HOST
> fi
>

Looks fine, but please give it a test run.

Thanks,
drew

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Gavin Shan]

Hi Drew,

On 6/20/23 19:06, Andrew Jones wrote:
> On Tue, Jun 20, 2023 at 02:13:22PM +1000, Gavin Shan wrote:
>> On 6/19/23 18:45, Andrew Jones wrote:
>>> On Thu, Jun 15, 2023 at 04:21:48PM +1000, Gavin Shan wrote:
>>>> There are extra properties for accelerators to enable the specific
>>>> features. For example, the dirty ring for KVM accelerator can be
>>>> enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
>>>> extra properties for the accelerators aren't supported. It makes
>>>> it's impossible to test the combination of KVM and dirty ring
>>>> as the following error message indicates.
>>>>
>>>>     # cd /home/gavin/sandbox/kvm-unit-tests/tests
>>>>     # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>>>       ACCEL=kvm,dirty-ring-size=65536 ./its-migration
>>>>        :
>>>>     BUILD_HEAD=2fffb37e
>>>>     timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>>>     -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
>>>>     -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
>>>>     -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
>>>>     -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
>>>>     qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
>>>>
>>>> Allow to specify extra properties for accelerators. With this, the
>>>> "its-migration" can be tested for the combination of KVM and dirty
>>>> ring.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>> v3: Split $ACCEL to $ACCEL and $ACCEL_PROPS in get_qemu_accelerator()
>>>>       and don't print them as output, suggested by Drew.
>>>> ---
>>>>    arm/run               | 12 ++++--------
>>>>    powerpc/run           |  5 ++---
>>>>    s390x/run             |  5 ++---
>>>>    scripts/arch-run.bash | 21 +++++++++++++--------
>>>>    x86/run               |  5 ++---
>>>>    5 files changed, 23 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/arm/run b/arm/run
>>>> index c6f25b8..d9ebe59 100755
>>>> --- a/arm/run
>>>> +++ b/arm/run
>>>> @@ -10,10 +10,8 @@ if [ -z "$KUT_STANDALONE" ]; then
>>>>    fi
>>>>    processor="$PROCESSOR"
>>>> -accel=$(get_qemu_accelerator) ||
>>>> -	exit $?
>>>> -
>>>> -if [ "$accel" = "kvm" ]; then
>>>> +get_qemu_accelerator || exit $?
>>>> +if [ "$ACCEL" = "kvm" ]; then
>>>>    	QEMU_ARCH=$HOST
>>>>    fi
>>>> @@ -23,11 +21,9 @@ qemu=$(search_qemu_binary) ||
>>>>    if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
>>>>       [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
>>>>       [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
>>>> -	accel=tcg
>>>> +	ACCEL="tcg"
>>>>    fi
>>>
>>> As I pointed out in the v2 review we can't just s/accel/ACCEL/ without
>>> other changes. Now ACCEL will also be set when the above condition
>>> is checked, making it useless. Please ensure the test case that commit
>>> c7d6c7f00e7c ("arm/run: Use TCG with qemu-system-arm on arm64 systems")
>>> fixed still works with your patch.
>>>
>>
>> Sorry that I missed your comments for v2. In order to make the test case
>> in c7d6c7f00e7c working, we just need to call set_qemu_accelerator() after
>> the chunk of code, like below. When $ACCEL is set to "tcg" by the conditional
>> code, it won't be changed in the following set_qemu_accelerator().
>>
>> Could you Please confirm if it looks good to you so that I can integrate
>> the changes to v4 and post it.
>>
>> arm/run
>> --------
>>
>> processor="$PROCESSOR"
>>
>> if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
>>     [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
>>     [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
>>          ACCEL="tcg"
>> fi
>>
>> set_qemu_accelerator || exit $?
>> if [ "$ACCEL" = "kvm" ]; then
>>          QEMU_ARCH=$HOST
>> fi
>>
> 
> Looks fine, but please give it a test run.
> 

Thanks, Drew. v4 has been posted for further review. Since I don't have a 'arm'
host around, I adjust $ARCH to "arm64" for a simulation and the test case included
in commit c7d6c7f00e7c should be working fine: We eventually have "tcg" instead of
"kvm" accelerator for the combination, which is expected by commit c7d6c7f00e7c

v4: https://lore.kernel.org/kvmarm/20230623035750.312679-1-gshan@redhat.com/T/#u

Thanks,
Gavin

Re: [kvm-unit-tests PATCH v3] runtime: Allow to specify properties for accelerator

[Andrew Jones]

On Fri, Jun 23, 2023 at 02:22:42PM +1000, Gavin Shan wrote:
> Hi Drew,
> 
> On 6/20/23 19:06, Andrew Jones wrote:
> > On Tue, Jun 20, 2023 at 02:13:22PM +1000, Gavin Shan wrote:
> > > On 6/19/23 18:45, Andrew Jones wrote:
> > > > On Thu, Jun 15, 2023 at 04:21:48PM +1000, Gavin Shan wrote:
> > > > > There are extra properties for accelerators to enable the specific
> > > > > features. For example, the dirty ring for KVM accelerator can be
> > > > > enabled by "-accel kvm,dirty-ring-size=65536". Unfortuntely, the
> > > > > extra properties for the accelerators aren't supported. It makes
> > > > > it's impossible to test the combination of KVM and dirty ring
> > > > > as the following error message indicates.
> > > > > 
> > > > >     # cd /home/gavin/sandbox/kvm-unit-tests/tests
> > > > >     # QEMU=/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> > > > >       ACCEL=kvm,dirty-ring-size=65536 ./its-migration
> > > > >        :
> > > > >     BUILD_HEAD=2fffb37e
> > > > >     timeout -k 1s --foreground 90s /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> > > > >     -nodefaults -machine virt -accel kvm,dirty-ring-size=65536 -cpu cortex-a57             \
> > > > >     -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd   \
> > > > >     -device pci-testdev -display none -serial stdio -kernel _NO_FILE_4Uhere_ -smp 160      \
> > > > >     -machine gic-version=3 -append its-pending-migration # -initrd /tmp/tmp.gfDLa1EtWk
> > > > >     qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
> > > > > 
> > > > > Allow to specify extra properties for accelerators. With this, the
> > > > > "its-migration" can be tested for the combination of KVM and dirty
> > > > > ring.
> > > > > 
> > > > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > > > ---
> > > > > v3: Split $ACCEL to $ACCEL and $ACCEL_PROPS in get_qemu_accelerator()
> > > > >       and don't print them as output, suggested by Drew.
> > > > > ---
> > > > >    arm/run               | 12 ++++--------
> > > > >    powerpc/run           |  5 ++---
> > > > >    s390x/run             |  5 ++---
> > > > >    scripts/arch-run.bash | 21 +++++++++++++--------
> > > > >    x86/run               |  5 ++---
> > > > >    5 files changed, 23 insertions(+), 25 deletions(-)
> > > > > 
> > > > > diff --git a/arm/run b/arm/run
> > > > > index c6f25b8..d9ebe59 100755
> > > > > --- a/arm/run
> > > > > +++ b/arm/run
> > > > > @@ -10,10 +10,8 @@ if [ -z "$KUT_STANDALONE" ]; then
> > > > >    fi
> > > > >    processor="$PROCESSOR"
> > > > > -accel=$(get_qemu_accelerator) ||
> > > > > -	exit $?
> > > > > -
> > > > > -if [ "$accel" = "kvm" ]; then
> > > > > +get_qemu_accelerator || exit $?
> > > > > +if [ "$ACCEL" = "kvm" ]; then
> > > > >    	QEMU_ARCH=$HOST
> > > > >    fi
> > > > > @@ -23,11 +21,9 @@ qemu=$(search_qemu_binary) ||
> > > > >    if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
> > > > >       [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
> > > > >       [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
> > > > > -	accel=tcg
> > > > > +	ACCEL="tcg"
> > > > >    fi
> > > > 
> > > > As I pointed out in the v2 review we can't just s/accel/ACCEL/ without
> > > > other changes. Now ACCEL will also be set when the above condition
> > > > is checked, making it useless. Please ensure the test case that commit
> > > > c7d6c7f00e7c ("arm/run: Use TCG with qemu-system-arm on arm64 systems")
> > > > fixed still works with your patch.
> > > > 
> > > 
> > > Sorry that I missed your comments for v2. In order to make the test case
> > > in c7d6c7f00e7c working, we just need to call set_qemu_accelerator() after
> > > the chunk of code, like below. When $ACCEL is set to "tcg" by the conditional
> > > code, it won't be changed in the following set_qemu_accelerator().
> > > 
> > > Could you Please confirm if it looks good to you so that I can integrate
> > > the changes to v4 and post it.
> > > 
> > > arm/run
> > > --------
> > > 
> > > processor="$PROCESSOR"
> > > 
> > > if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
> > >     [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
> > >     [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
> > >          ACCEL="tcg"
> > > fi
> > > 
> > > set_qemu_accelerator || exit $?
> > > if [ "$ACCEL" = "kvm" ]; then
> > >          QEMU_ARCH=$HOST
> > > fi
> > > 
> > 
> > Looks fine, but please give it a test run.
> > 
> 
> Thanks, Drew. v4 has been posted for further review. Since I don't have a 'arm'
> host around, I adjust $ARCH to "arm64" for a simulation and the test case included
> in commit c7d6c7f00e7c should be working fine: We eventually have "tcg" instead of
> "kvm" accelerator for the combination, which is expected by commit c7d6c7f00e7c

You don't need an arm host. Indeed $HOST should be aarch64 for this case.
You just need to compile the tests for arm instead of aarch64. That's easy
to do with a cross compiler

 ./configure --arch=arm --cross-prefix=arm-linux-gnu-

But hacking the condition is fine too for this simple case.

Thanks,
drew

> 
> v4: https://lore.kernel.org/kvmarm/20230623035750.312679-1-gshan@redhat.com/T/#u
> 
> Thanks,
> Gavin
>