| Message ID | 20230615062148.19883-1-gshan@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [kvm-unit-tests,v3] runtime: Allow to specify properties for accelerator | expand |
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>
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
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
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
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
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
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
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
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 >
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
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(-)