From: Peter Hoyes <Peter.Hoyes@arm.com>
Saving, restoring and migrating domains are not currently supported on
arm and arm64 platforms, so xendomains prints the warning:
An error occurred while saving domain:
command not implemented
when attempting to run `xendomains stop`. It otherwise continues to shut
down the domains cleanly, with the unsupported steps skipped.
Use `xl help` to detect whether save/restore/migrate is supported by the
platform. If not, do not attempt to run the corresponding command.
Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
---
tools/hotplug/Linux/xendomains.in | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/tools/hotplug/Linux/xendomains.in b/tools/hotplug/Linux/xendomains.in
index 70f4129ef4..bafcb874e1 100644
--- a/tools/hotplug/Linux/xendomains.in
+++ b/tools/hotplug/Linux/xendomains.in
@@ -229,6 +229,15 @@ parseln()
[ -n "$name" -a -n "$id" ] && return 0 || return 1
}
+subcmd_supported()
+{
+ local output
+ output=$("$CMD help | grep "^ $1"")
+ if [ ! "$output" ]; then
+ return 1
+ fi
+}
+
is_running()
{
get_xsdomid
@@ -260,7 +269,8 @@ start()
saved_domains=" "
if [ "$XENDOMAINS_RESTORE" = "true" ] &&
- contains_something "$XENDOMAINS_SAVE"
+ contains_something "$XENDOMAINS_SAVE" &&
+ subcmd_supported "restore"
then
echo -n "Restoring Xen domains:"
saved_domains=`ls $XENDOMAINS_SAVE`
@@ -411,7 +421,7 @@ stop()
echo -n "(zomb)"
continue
fi
- if test -n "$XENDOMAINS_MIGRATE"; then
+ if test -n "$XENDOMAINS_MIGRATE" && subcmd_supported "migrate"; then
echo -n "(migr)"
watchdog_xencmd migrate &
WDOG_PID=$!
@@ -430,7 +440,7 @@ stop()
continue
fi
fi
- if test -n "$XENDOMAINS_SAVE"; then
+ if test -n "$XENDOMAINS_SAVE" && subcmd_supported "save"; then
echo -n "(save)"
watchdog_xencmd save &
WDOG_PID=$!
--
2.34.1> On 22 Mar 2023, at 13:58, Peter Hoyes <Peter.Hoyes@arm.com> wrote: > > From: Peter Hoyes <Peter.Hoyes@arm.com> > > Saving, restoring and migrating domains are not currently supported on > arm and arm64 platforms, so xendomains prints the warning: > > An error occurred while saving domain: > command not implemented > > when attempting to run `xendomains stop`. It otherwise continues to shut > down the domains cleanly, with the unsupported steps skipped. > > Use `xl help` to detect whether save/restore/migrate is supported by the > platform. If not, do not attempt to run the corresponding command. > > Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com> > --- Hi Anthony, Regarding this patch, is there any update? I’m asking just because here https://patchwork.kernel.org/project/xen-devel/patch/20230322135800.3869458-1-peter.hoyes@arm.com/ it seems you were volunteering to fix that in another way. Cheers, Luca
On Thu, Oct 19, 2023 at 12:50:52PM +0000, Luca Fancellu wrote: > > > > On 22 Mar 2023, at 13:58, Peter Hoyes <Peter.Hoyes@arm.com> wrote: > > > > From: Peter Hoyes <Peter.Hoyes@arm.com> > > > > Saving, restoring and migrating domains are not currently supported on > > arm and arm64 platforms, so xendomains prints the warning: > > > > An error occurred while saving domain: > > command not implemented > > > > when attempting to run `xendomains stop`. It otherwise continues to shut > > down the domains cleanly, with the unsupported steps skipped. > > > > Use `xl help` to detect whether save/restore/migrate is supported by the > > platform. If not, do not attempt to run the corresponding command. > > > > Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com> > > --- > > Hi Anthony, > > Regarding this patch, is there any update? > > I’m asking just because here https://patchwork.kernel.org/project/xen-devel/patch/20230322135800.3869458-1-peter.hoyes@arm.com/ it seems you were volunteering to fix that in another way. I did, https://patchwork.kernel.org/project/xen-devel/patch/20230519162454.50337-1-anthony.perard@citrix.com/ But looks like I need to update the patch. I think it's a bit late for 3.18, but I should be able to update the patch for next release. Cheers, -- Anthony PERARD
On Wed, Mar 22, 2023 at 01:58:00PM +0000, Peter Hoyes wrote:
> From: Peter Hoyes <Peter.Hoyes@arm.com>
>
> Saving, restoring and migrating domains are not currently supported on
> arm and arm64 platforms, so xendomains prints the warning:
>
> An error occurred while saving domain:
> command not implemented
>
> when attempting to run `xendomains stop`. It otherwise continues to shut
> down the domains cleanly, with the unsupported steps skipped.
The patch looks kind of ok, but shouldn't $XENDOMAINS_SAVE be set to an
empty string in the config by the admin instead?
Or is the issue that $XENDOMAINS_SAVE is set by default, even on arm* ?
Maybe it's easier to check that the command is implemented at run time
rather than trying to have a good default value for XENDOMAINS_SAVE at
install/package time.
> Use `xl help` to detect whether save/restore/migrate is supported by the
> platform. If not, do not attempt to run the corresponding command.
>
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> ---
> tools/hotplug/Linux/xendomains.in | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/tools/hotplug/Linux/xendomains.in b/tools/hotplug/Linux/xendomains.in
> index 70f4129ef4..bafcb874e1 100644
> --- a/tools/hotplug/Linux/xendomains.in
> +++ b/tools/hotplug/Linux/xendomains.in
> @@ -229,6 +229,15 @@ parseln()
> [ -n "$name" -a -n "$id" ] && return 0 || return 1
> }
>
> +subcmd_supported()
> +{
> + local output
> + output=$("$CMD help | grep "^ $1"")
> + if [ ! "$output" ]; then
> + return 1
> + fi
It looks like some quote are in the wrong place. You probably wanted to
write:
output="$($CMD help | grep "^ $1")"
But I'd like to have this slightly more robust, to match the whole
command, rather than the beginning.
(For example `subcmd_supported "pci"` would reture true even if no "pci"
command exist.)
Something like:
$CMD help | grep "^ $1\( \|$\)"
To check that the command name is followed by a space or end-of-line
(even if it seems that there's always a space printed.)
A similar pattern is used in "tools/xl/bash-completion", so it should
probably be fine in the future.
Then, we don't really need the "$output" from grep, we could just ask it
if there's a match or not, with --quiet:
$CMD help | grep -q "^ $1\( \|$\)"
And that would be the whole function. Would that works for you?
The rest of the patch looks fine.
Thanks,
--
Anthony PERARD
On 04/04/2023 17:28, Anthony PERARD wrote: > On Wed, Mar 22, 2023 at 01:58:00PM +0000, Peter Hoyes wrote: >> From: Peter Hoyes <Peter.Hoyes@arm.com> >> >> Saving, restoring and migrating domains are not currently supported on >> arm and arm64 platforms, so xendomains prints the warning: >> >> An error occurred while saving domain: >> command not implemented >> >> when attempting to run `xendomains stop`. It otherwise continues to shut >> down the domains cleanly, with the unsupported steps skipped. > The patch looks kind of ok, but shouldn't $XENDOMAINS_SAVE be set to an > empty string in the config by the admin instead? > > Or is the issue that $XENDOMAINS_SAVE is set by default, even on arm* ? Yea the default is the issue. We are building for embedded, using Yocto, so there isn't really an admin. > > Maybe it's easier to check that the command is implemented at run time > rather than trying to have a good default value for XENDOMAINS_SAVE at > install/package time. It would be cleaner to do this at build time for sure, but I'm not sure the autotools config file approach for sysconfig.xendomains.in can handle the logic for this? Thanks, Peter
On Thu, Apr 06, 2023 at 03:57:12PM +0100, Peter Hoyes wrote: > On 04/04/2023 17:28, Anthony PERARD wrote: > > On Wed, Mar 22, 2023 at 01:58:00PM +0000, Peter Hoyes wrote: > > > From: Peter Hoyes <Peter.Hoyes@arm.com> > > > > > > Saving, restoring and migrating domains are not currently supported on > > > arm and arm64 platforms, so xendomains prints the warning: > > > > > > An error occurred while saving domain: > > > command not implemented > > > > > > when attempting to run `xendomains stop`. It otherwise continues to shut > > > down the domains cleanly, with the unsupported steps skipped. > > The patch looks kind of ok, but shouldn't $XENDOMAINS_SAVE be set to an > > empty string in the config by the admin instead? > > > > Or is the issue that $XENDOMAINS_SAVE is set by default, even on arm* ? > Yea the default is the issue. We are building for embedded, using Yocto, so > there isn't really an admin. > > > > Maybe it's easier to check that the command is implemented at run time > > rather than trying to have a good default value for XENDOMAINS_SAVE at > > install/package time. > > It would be cleaner to do this at build time for sure, but I'm not sure the > autotools config file approach for sysconfig.xendomains.in can handle the > logic for this? I think that possible, we can already change config in ./configure based on the CPU. I'm preparing a patch. Thanks, -- Anthony PERARD
Saving, restoring and migrating domains are not currently supported on
arm and arm64 platforms, so xendomains prints the warning:
An error occurred while saving domain:
command not implemented
when attempting to run `xendomains stop`. It otherwise continues to shut
down the domains cleanly, with the unsupported steps skipped.
Also in sysconfig.xendomains, change "Default" to "Example" as the
real default is an empty value.
Reported-by: Peter Hoyes <Peter.Hoyes@arm.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Peter, what do you think of this approach?
For reference, there's also a way to findout if a macro exist, with
AC_CHECK_DECL(), but the libxl.h header depends on other headers that
needs to be generated.
---
tools/configure | 11 +++++++++++
tools/configure.ac | 13 +++++++++++++
tools/hotplug/Linux/init.d/sysconfig.xendomains.in | 4 ++--
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/tools/configure b/tools/configure
index 52b4717d01..a722f72c08 100755
--- a/tools/configure
+++ b/tools/configure
@@ -624,6 +624,7 @@ ac_includes_default="\
ac_subst_vars='LTLIBOBJS
LIBOBJS
+XENDOMAINS_SAVE_DIR
pvshim
ninepfs
SYSTEMD_LIBS
@@ -10155,6 +10156,16 @@ if test "$ax_found" = "0"; then :
fi
+case "$host_cpu" in
+ arm*|aarch64)
+ XENDOMAINS_SAVE_DIR=
+ ;;
+ *)
+ XENDOMAINS_SAVE_DIR="$XEN_LIB_DIR/save"
+ ;;
+esac
+
+
cat >confcache <<\_ACEOF
# This file is a shell script that caches the results of configure
# tests run on this system so they can be shared between configure
diff --git a/tools/configure.ac b/tools/configure.ac
index 3cccf41960..0f0983f6b7 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -517,4 +517,17 @@ AS_IF([test "x$pvshim" = "xy"], [
AX_FIND_HEADER([INCLUDE_ENDIAN_H], [endian.h sys/endian.h])
+dnl Disable autosave of domain in xendomains on shutdown
+dnl due to missing support. This should be in sync with
+dnl LIBXL_HAVE_NO_SUSPEND_RESUME in libxl.h
+case "$host_cpu" in
+ arm*|aarch64)
+ XENDOMAINS_SAVE_DIR=
+ ;;
+ *)
+ XENDOMAINS_SAVE_DIR="$XEN_LIB_DIR/save"
+ ;;
+esac
+AC_SUBST(XENDOMAINS_SAVE_DIR)
+
AC_OUTPUT()
diff --git a/tools/hotplug/Linux/init.d/sysconfig.xendomains.in b/tools/hotplug/Linux/init.d/sysconfig.xendomains.in
index f61ef9c4d1..3c49f18bb0 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xendomains.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xendomains.in
@@ -45,7 +45,7 @@ XENDOMAINS_CREATE_USLEEP=5000000
XENDOMAINS_MIGRATE=""
## Type: string
-## Default: @XEN_LIB_DIR@/save
+## Example: @XEN_LIB_DIR@/save
#
# Directory to save running domains to when the system (dom0) is
# shut down. Will also be used to restore domains from if # XENDOMAINS_RESTORE
@@ -53,7 +53,7 @@ XENDOMAINS_MIGRATE=""
# (e.g. because you rather shut domains down).
# If domain saving does succeed, SHUTDOWN will not be executed.
#
-XENDOMAINS_SAVE=@XEN_LIB_DIR@/save
+XENDOMAINS_SAVE=@XENDOMAINS_SAVE_DIR@
## Type: string
## Default: "--wait"
--
Anthony PERARDHi Anthony, On 19/05/2023 17:24, Anthony PERARD wrote: > Saving, restoring and migrating domains are not currently supported on > arm and arm64 platforms, so xendomains prints the warning: > > An error occurred while saving domain: > command not implemented > > when attempting to run `xendomains stop`. It otherwise continues to shut > down the domains cleanly, with the unsupported steps skipped. > > Also in sysconfig.xendomains, change "Default" to "Example" as the > real default is an empty value. > > Reported-by: Peter Hoyes <Peter.Hoyes@arm.com> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Peter, what do you think of this approach? > > For reference, there's also a way to findout if a macro exist, with > AC_CHECK_DECL(), but the libxl.h header depends on other headers that > needs to be generated. > --- > tools/configure | 11 +++++++++++ > tools/configure.ac | 13 +++++++++++++ > tools/hotplug/Linux/init.d/sysconfig.xendomains.in | 4 ++-- > 3 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/tools/configure b/tools/configure > index 52b4717d01..a722f72c08 100755 > --- a/tools/configure > +++ b/tools/configure > @@ -624,6 +624,7 @@ ac_includes_default="\ > > ac_subst_vars='LTLIBOBJS > LIBOBJS > +XENDOMAINS_SAVE_DIR > pvshim > ninepfs > SYSTEMD_LIBS > @@ -10155,6 +10156,16 @@ if test "$ax_found" = "0"; then : > fi > > > +case "$host_cpu" in > + arm*|aarch64) > + XENDOMAINS_SAVE_DIR= > + ;; > + *) > + XENDOMAINS_SAVE_DIR="$XEN_LIB_DIR/save" > + ;; > +esac > + > + > cat >confcache <<\_ACEOF > # This file is a shell script that caches the results of configure > # tests run on this system so they can be shared between configure > diff --git a/tools/configure.ac b/tools/configure.ac > index 3cccf41960..0f0983f6b7 100644 > --- a/tools/configure.ac > +++ b/tools/configure.ac > @@ -517,4 +517,17 @@ AS_IF([test "x$pvshim" = "xy"], [ > > AX_FIND_HEADER([INCLUDE_ENDIAN_H], [endian.h sys/endian.h]) > > +dnl Disable autosave of domain in xendomains on shutdown > +dnl due to missing support. This should be in sync with > +dnl LIBXL_HAVE_NO_SUSPEND_RESUME in libxl.h Quite likely, a developer adding a new arch will first look at the definition of LIXBL_HAVE_NO_SUSPEND_RESUME. So it would be good if we have a similar message there to remind them to update this case. That said... > +case "$host_cpu" in > + arm*|aarch64) > + XENDOMAINS_SAVE_DIR= > + ;; > + *) > + XENDOMAINS_SAVE_DIR="$XEN_LIB_DIR/save" > + ;; > +esac ... I am wondering if the switch should be the other way around. IOW, the default should be no support for suspend/resume. This will make easier to add support for RISC-V (I suspect support for suspend/resume will not be in the first version) or any new other arch. Cheers, -- Julien Grall
On Mon, May 22, 2023 at 08:34:52PM +0100, Julien Grall wrote: > On 19/05/2023 17:24, Anthony PERARD wrote: > > diff --git a/tools/configure.ac b/tools/configure.ac > > index 3cccf41960..0f0983f6b7 100644 > > --- a/tools/configure.ac > > +++ b/tools/configure.ac > > @@ -517,4 +517,17 @@ AS_IF([test "x$pvshim" = "xy"], [ > > AX_FIND_HEADER([INCLUDE_ENDIAN_H], [endian.h sys/endian.h]) > > +dnl Disable autosave of domain in xendomains on shutdown > > +dnl due to missing support. This should be in sync with > > +dnl LIBXL_HAVE_NO_SUSPEND_RESUME in libxl.h > > Quite likely, a developer adding a new arch will first look at the > definition of LIXBL_HAVE_NO_SUSPEND_RESUME. So it would be good if we have a > similar message there to remind them to update this case. That said... Probably true, I'll look at adding a comment there. > > +case "$host_cpu" in > > + arm*|aarch64) > > + XENDOMAINS_SAVE_DIR= > > + ;; > > + *) > > + XENDOMAINS_SAVE_DIR="$XEN_LIB_DIR/save" > > + ;; > > +esac > > ... I am wondering if the switch should be the other way around. IOW, the > default should be no support for suspend/resume. This will make easier to > add support for RISC-V (I suspect support for suspend/resume will not be in > the first version) or any new other arch. Sounds good, I'll look at that. Thanks, -- Anthony PERARD
On 19/05/2023 17:24, Anthony PERARD wrote: > Saving, restoring and migrating domains are not currently supported on > arm and arm64 platforms, so xendomains prints the warning: > > An error occurred while saving domain: > command not implemented > > when attempting to run `xendomains stop`. It otherwise continues to shut > down the domains cleanly, with the unsupported steps skipped. > > Also in sysconfig.xendomains, change "Default" to "Example" as the > real default is an empty value. > > Reported-by: Peter Hoyes <Peter.Hoyes@arm.com> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Peter, what do you think of this approach? Thanks for preparing this. Just validated that the warning above is not present with both qemu-aarch64 and an internal stack. Tested-by: Peter Hoyes <peter.hoyes@arm.com> > > For reference, there's also a way to findout if a macro exist, with > AC_CHECK_DECL(), but the libxl.h header depends on other headers that > needs to be generated. > --- > tools/configure | 11 +++++++++++ > tools/configure.ac | 13 +++++++++++++ > tools/hotplug/Linux/init.d/sysconfig.xendomains.in | 4 ++-- > 3 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/tools/configure b/tools/configure > index 52b4717d01..a722f72c08 100755 > --- a/tools/configure > +++ b/tools/configure > @@ -624,6 +624,7 @@ ac_includes_default="\ > > ac_subst_vars='LTLIBOBJS > LIBOBJS > +XENDOMAINS_SAVE_DIR > pvshim > ninepfs > SYSTEMD_LIBS > @@ -10155,6 +10156,16 @@ if test "$ax_found" = "0"; then : > fi > > > +case "$host_cpu" in > + arm*|aarch64) > + XENDOMAINS_SAVE_DIR= > + ;; > + *) > + XENDOMAINS_SAVE_DIR="$XEN_LIB_DIR/save" > + ;; > +esac > + > + > cat >confcache <<\_ACEOF > # This file is a shell script that caches the results of configure > # tests run on this system so they can be shared between configure > diff --git a/tools/configure.ac b/tools/configure.ac > index 3cccf41960..0f0983f6b7 100644 > --- a/tools/configure.ac > +++ b/tools/configure.ac > @@ -517,4 +517,17 @@ AS_IF([test "x$pvshim" = "xy"], [ > > AX_FIND_HEADER([INCLUDE_ENDIAN_H], [endian.h sys/endian.h]) > > +dnl Disable autosave of domain in xendomains on shutdown > +dnl due to missing support. This should be in sync with > +dnl LIBXL_HAVE_NO_SUSPEND_RESUME in libxl.h > +case "$host_cpu" in > + arm*|aarch64) > + XENDOMAINS_SAVE_DIR= > + ;; > + *) > + XENDOMAINS_SAVE_DIR="$XEN_LIB_DIR/save" > + ;; > +esac > +AC_SUBST(XENDOMAINS_SAVE_DIR) > + > AC_OUTPUT() > diff --git a/tools/hotplug/Linux/init.d/sysconfig.xendomains.in b/tools/hotplug/Linux/init.d/sysconfig.xendomains.in > index f61ef9c4d1..3c49f18bb0 100644 > --- a/tools/hotplug/Linux/init.d/sysconfig.xendomains.in > +++ b/tools/hotplug/Linux/init.d/sysconfig.xendomains.in > @@ -45,7 +45,7 @@ XENDOMAINS_CREATE_USLEEP=5000000 > XENDOMAINS_MIGRATE="" > > ## Type: string > -## Default: @XEN_LIB_DIR@/save > +## Example: @XEN_LIB_DIR@/save > # > # Directory to save running domains to when the system (dom0) is > # shut down. Will also be used to restore domains from if # XENDOMAINS_RESTORE > @@ -53,7 +53,7 @@ XENDOMAINS_MIGRATE="" > # (e.g. because you rather shut domains down). > # If domain saving does succeed, SHUTDOWN will not be executed. > # > -XENDOMAINS_SAVE=@XEN_LIB_DIR@/save > +XENDOMAINS_SAVE=@XENDOMAINS_SAVE_DIR@ > > ## Type: string > ## Default: "--wait"
© 2016 - 2024 Red Hat, Inc.