| Message ID | 20210318124500.45447-2-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scripts: Fix accel handling | expand |
On Thu, Mar 18, 2021 at 12:44:59PM +0000, Janosch Frank wrote: > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index 5997e384..8cc9a61e 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -342,8 +342,11 @@ trap_exit_push () > > kvm_available () > { > - [ -c /dev/kvm ] || > - return 1 > + if $($qemu -accel kvm 2> /dev/null); then > + return 0; > + else > + return 1; > + fi > Hi Janosch, Are we sure that even old QEMU supports this type of probing? If not, then we should probably keep the /dev/kvm test as a fallback. Thanks, drew
On Thu, Mar 18, 2021 at 12:44:59PM +0000, Janosch Frank wrote: > The existence of the /dev/kvm character device doesn't imply that the > kvm module is part of the kernel or that it's always magically loaded > when needed. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arm/run | 4 ++-- > powerpc/run | 4 ++-- > s390x/run | 4 ++-- > scripts/arch-run.bash | 7 +++++-- > x86/run | 4 ++-- > 5 files changed, 13 insertions(+), 10 deletions(-) > > diff --git a/arm/run b/arm/run > index a390ca5a..ca2d44e0 100755 > --- a/arm/run > +++ b/arm/run > @@ -10,10 +10,10 @@ if [ -z "$STANDALONE" ]; then > fi > processor="$PROCESSOR" > > -ACCEL=$(get_qemu_accelerator) || > +qemu=$(search_qemu_binary) || > exit $? > > -qemu=$(search_qemu_binary) || > +ACCEL=$(get_qemu_accelerator) || > exit $? How about renaming search_qemu_binary() to set_qemu_accelerator(), which would also ensure QEMU is set (if it doesn't error out on failure) and then call that from get_qemu_accelerator()? That way we don't need to worry about this order of calls nor this lowercase 'qemu' variable being set. Also, we can rename get_qemu_accelerator() to set_qemu_accelerator() and ensure it sets ACCEL. Thanks, drew
On 3/18/21 4:31 PM, Andrew Jones wrote: > On Thu, Mar 18, 2021 at 12:44:59PM +0000, Janosch Frank wrote: >> The existence of the /dev/kvm character device doesn't imply that the >> kvm module is part of the kernel or that it's always magically loaded >> when needed. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arm/run | 4 ++-- >> powerpc/run | 4 ++-- >> s390x/run | 4 ++-- >> scripts/arch-run.bash | 7 +++++-- >> x86/run | 4 ++-- >> 5 files changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/arm/run b/arm/run >> index a390ca5a..ca2d44e0 100755 >> --- a/arm/run >> +++ b/arm/run >> @@ -10,10 +10,10 @@ if [ -z "$STANDALONE" ]; then >> fi >> processor="$PROCESSOR" >> >> -ACCEL=$(get_qemu_accelerator) || >> +qemu=$(search_qemu_binary) || >> exit $? >> >> -qemu=$(search_qemu_binary) || >> +ACCEL=$(get_qemu_accelerator) || >> exit $? > > How about renaming search_qemu_binary() to set_qemu_accelerator(), which > would also ensure QEMU is set (if it doesn't error out on failure) and > then call that from get_qemu_accelerator()? That way we don't need to > worry about this order of calls nor this lowercase 'qemu' variable being > set. Also, we can rename get_qemu_accelerator() to set_qemu_accelerator() > and ensure it sets ACCEL. Sure, I was already considering that. The fact that qemu and ACCEL are basically global and accel is not the same as ACCEL makes all of this harder to understand than it should be. > > Thanks, > drew >
On 3/18/21 4:31 PM, Andrew Jones wrote: > On Thu, Mar 18, 2021 at 12:44:59PM +0000, Janosch Frank wrote: >> The existence of the /dev/kvm character device doesn't imply that the >> kvm module is part of the kernel or that it's always magically loaded >> when needed. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arm/run | 4 ++-- >> powerpc/run | 4 ++-- >> s390x/run | 4 ++-- >> scripts/arch-run.bash | 7 +++++-- >> x86/run | 4 ++-- >> 5 files changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/arm/run b/arm/run >> index a390ca5a..ca2d44e0 100755 >> --- a/arm/run >> +++ b/arm/run >> @@ -10,10 +10,10 @@ if [ -z "$STANDALONE" ]; then >> fi >> processor="$PROCESSOR" >> >> -ACCEL=$(get_qemu_accelerator) || >> +qemu=$(search_qemu_binary) || >> exit $? >> >> -qemu=$(search_qemu_binary) || >> +ACCEL=$(get_qemu_accelerator) || >> exit $? > > How about renaming search_qemu_binary() to set_qemu_accelerator(), which > would also ensure QEMU is set (if it doesn't error out on failure) and > then call that from get_qemu_accelerator()? That way we don't need to > worry about this order of calls nor this lowercase 'qemu' variable being > set. Also, we can rename get_qemu_accelerator() to set_qemu_accelerator() > and ensure it sets ACCEL. > > Thanks, > drew > I've already broken off the accel.bash change and fixed the arch/run problem but this here will have two wait until after my easter vacation so don't wait for patches the next ~2 weeks.
diff --git a/arm/run b/arm/run index a390ca5a..ca2d44e0 100755 --- a/arm/run +++ b/arm/run @@ -10,10 +10,10 @@ if [ -z "$STANDALONE" ]; then fi processor="$PROCESSOR" -ACCEL=$(get_qemu_accelerator) || +qemu=$(search_qemu_binary) || exit $? -qemu=$(search_qemu_binary) || +ACCEL=$(get_qemu_accelerator) || exit $? if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then diff --git a/powerpc/run b/powerpc/run index 597ab96e..b51ac6fc 100755 --- a/powerpc/run +++ b/powerpc/run @@ -9,10 +9,10 @@ if [ -z "$STANDALONE" ]; then source scripts/arch-run.bash fi -ACCEL=$(get_qemu_accelerator) || +qemu=$(search_qemu_binary) || exit $? -qemu=$(search_qemu_binary) || +ACCEL=$(get_qemu_accelerator) || exit $? if ! $qemu -machine '?' 2>&1 | grep 'pseries' > /dev/null; then diff --git a/s390x/run b/s390x/run index 09805044..2ec6da70 100755 --- a/s390x/run +++ b/s390x/run @@ -9,10 +9,10 @@ if [ -z "$STANDALONE" ]; then source scripts/arch-run.bash fi -ACCEL=$(get_qemu_accelerator) || +qemu=$(search_qemu_binary) || exit $? -qemu=$(search_qemu_binary) || +ACCEL=$(get_qemu_accelerator) || exit $? M='-machine s390-ccw-virtio' diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 5997e384..8cc9a61e 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -342,8 +342,11 @@ trap_exit_push () kvm_available () { - [ -c /dev/kvm ] || - return 1 + if $($qemu -accel kvm 2> /dev/null); then + return 0; + else + return 1; + fi [ "$HOST" = "$ARCH_NAME" ] || ( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) || diff --git a/x86/run b/x86/run index 8b2425f4..cbf49143 100755 --- a/x86/run +++ b/x86/run @@ -9,10 +9,10 @@ if [ -z "$STANDALONE" ]; then source scripts/arch-run.bash fi -ACCEL=$(get_qemu_accelerator) || +qemu=$(search_qemu_binary) || exit $? -qemu=$(search_qemu_binary) || +ACCEL=$(get_qemu_accelerator) || exit $? if ! ${qemu} -device '?' 2>&1 | grep -F -e \"testdev\" -e \"pc-testdev\" > /dev/null;
The existence of the /dev/kvm character device doesn't imply that the kvm module is part of the kernel or that it's always magically loaded when needed. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- arm/run | 4 ++-- powerpc/run | 4 ++-- s390x/run | 4 ++-- scripts/arch-run.bash | 7 +++++-- x86/run | 4 ++-- 5 files changed, 13 insertions(+), 10 deletions(-)