Re: [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format
On Wed, Apr 24, 2024 at 11:34:43AM +0800, Henry Wang wrote:
> diff --git a/tools/libs/light/libxl_dt_overlay.c
> b/tools/libs/light/libxl_dt_overlay.c
> index cdb62b28cf..eaf11a0f9c 100644
> --- a/tools/libs/light/libxl_dt_overlay.c
> +++ b/tools/libs/light/libxl_dt_overlay.c
> @@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt,
> size_t size)
> return 0;
> }
>
> +static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
> + size_t size)
> +{
> +int rc = 0;
> +int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
> +const struct fdt_property *fdt_prop_node = NULL;
> +int overlay;
> +int prop_len = 0;
> +int subnode = 0;
> +int fragment;
> +const char *prop_name;
> +const char *target_path = "/";
> +
> +fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
> +prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
> +_len);
> +if (prop_name == NULL) {
> +LOG(ERROR, "target-path property not found\n");
LOG* macros already takes care of adding \n, no need to add an extra
one.
> +rc = ERROR_FAIL;
> +goto err;
> +}
> +
> +/* Change target path for domU dtb. */
> +rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",
fdt_setprop_string() isn't a libxl function, store the return value in a
variable named `r` instead.
> +target_path);
> +if (rc) {
> +LOG(ERROR, "Setting interrupt parent property failed for %s\n",
> +prop_name);
> +goto err;
> +}
> +
> +overlay = fdt_subnode_offset(overlay_dt_domU, fragment,
> "__overlay__");
> +
> +fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
> +{
> +const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
> + NULL);
> +
> +fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
> +"interrupt-parent", _len);
> +if (fdt_prop_node == NULL) {
> +LOG(DETAIL, "%s property not found for %s. Skip to next
> node\n",
> +"interrupt-parent", node_name);
Why do you have "interrupt-parent" in a separate argument? Do you meant
to do something like
const char *some_name = "interrupt-parent";
and use that in the 4 different places that this string is used? (Using
a variable mean that we (or the compiler) can make sure that they are
all spelled correctly.
> +continue;
> +}
> +
> +rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
> + "interrupt-parent",
> + virtual_interrupt_parent);
> +if (rc) {
> +LOG(ERROR, "Setting interrupt parent property failed for
> %s\n",
> +"interrupt-parent");
> +goto err;
> +}
> +}
> +}
> +
> +return 0;
Missed indentation.
> +
> +err:
> +return rc;
A few things, looks like `rc` is always going to be ERROR_FAIL here,
unless you find an libxl_error code that better describe the error, so
you could forgo the `rc` variable.
Also, if you don't need to clean up anything in the function or have a
generic error message, you could simply "return " instead of using the
"goto" style.
> +}
> +
> int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
> uint32_t overlay_dt_size, uint8_t overlay_op,
> bool auto_mode, bool domain_mapping)
> @@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void
> *overlay_dt,
> rc = ERROR_FAIL;
> }
>
> + /*
> + * auto_mode doesn't apply to dom0 as dom0 can get the physical
> + * description of the hardware.
> + */
> +if (domid && auto_mode) {
> +if (overlay_op == LIBXL_DT_OVERLAY_ADD)
Shouldn't libxl complain if the operation is different?
> +rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
> +}
> +
> out:
> GC_FREE;
> return rc;
Thanks,
--
Anthony PERARD
Re: [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations
rgc - 1], "-e"))
> +auto_mode = false;
> +
> +if (argc == 4 || !auto_mode) {
> +domain_id = find_domain(argv[argc-1]);
> +domain_mapping = true;
> +}
> +
> +if (argc == 5 || !auto_mode) {
> +domain_id = find_domain(argv[argc-2]);
> +domain_mapping = true;
> +}
Sorry, I can't review that changes, this needs a change in the help
message of dt-overlay, and something in the man page. (and that argument
parsing looks convoluted).
> +
> +/* User didn't prove any overlay operation. */
I guess you meant "provide" instead of prove. But the comment can be
discarded, it doesn't explain anything useful that the error message
doesn't already explain.
> +if (overlay_ops == NULL) {
> +fprintf(stderr, "No overlay operation mode provided\n");
> +return ERROR_FAIL;
That should be EXIT_FAILURE instead. (and I realise that the function
already return ERROR_FAIL by mistake in several places. (ERROR_FAIL is a
libxl error return value of -3, which isn't really a good exit value for
CLI programmes.))
Thanks,
--
Anthony PERARD
Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160
On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote: > Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32. > This was done to allocate and assign IRQs to a running domain. > > Signed-off-by: Vikram Garhwal > Signed-off-by: Stefano Stabellini > Signed-off-by: Henry Wang > --- > tools/libs/light/libxl_arm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index dd5c9f4917..50dbd0f2a9 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > > LOG(DEBUG, "Configure the domain"); > > -config->arch.nr_spis = nr_spis; > +/* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. > */ > +config->arch.nr_spis = MAX(nr_spis, 160); Is there a way that that Xen or libxl could find out what the minimum number of SPI needs to be? Are we going to have to increase that minimum number every time a new platform comes along? It doesn't appear that libxl is using that `nr_spis` value and it is probably just given to Xen. So my guess is that Xen could simply take care of the minimum value, gic_number_lines() seems to be a Xen function. Thanks, -- Anthony PERARD
Re: [PATCH 04/15] tools/libs/light: Always enable IOMMU
On Wed, Apr 24, 2024 at 11:34:38AM +0800, Henry Wang wrote:
> For overlay with iommu functionality to work with running VMs, we need
> to enable IOMMU when iomem presents for the domains.
>
> Signed-off-by: Vikram Garhwal
> Signed-off-by: Henry Wang
> ---
> tools/libs/light/libxl_arm.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 1cb89fa584..dd5c9f4917 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -222,6 +222,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
> }
>
> +#ifdef LIBXL_HAVE_DT_OVERLAY
libxl_arm.c is only build on Arm, so this should be defined, so no need
to check.
> +if (d_config->b_info.num_iomem) {
> +config->flags |= XEN_DOMCTL_CDF_iommu;
Is this doing the same thing as the previous patch?
Thanks,
--
Anthony PERARD
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On Fri, Apr 26, 2024 at 09:51:47AM +0100, Anthony PERARD wrote: > Run `systemd-notify --ready` instead. Hopefully, that will be enough. > ($NOTIFY_SOCKET is a socket, and a bit more complicated that I though, > it can start with "@" for example) FTR: If it turns out that calling systemd-notify binary isn't working well enough, we could have an implementation of sd_notify() in our tree, openssh are doing there own here: https://bugzilla.mindrot.org/show_bug.cgi?id=2641 and there's an example implementation on systemd's documentation: https://www.freedesktop.org/software/systemd/man/devel/sd_notify.html#Notes (Nothing for ocaml) But let's go with `systemd-notify --ready` as it is just easier to write a bit of shell script. Cheers, -- Anthony PERARD
Re: [PATCH 3/3] tools/xg: Clean up xend-style overrides for CPU policies
On Wed, Feb 07, 2024 at 05:39:57PM +, Alejandro Vallejo wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 5699a26b946e..cee0be80ba5b 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -772,49 +616,45 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t
> domid, bool restore,
> * apic_id_size values greater than 7. Limit the value to
> * 7 for now.
> */
> -if ( p->policy.extd.nc < 0x7f )
> +if ( cur->policy.extd.nc < 0x7f )
> {
> -if ( p->policy.extd.apic_id_size != 0 &&
> p->policy.extd.apic_id_size < 0x7 )
> -p->policy.extd.apic_id_size++;
> +if ( cur->policy.extd.apic_id_size != 0 &&
> cur->policy.extd.apic_id_size < 0x7 )
> +cur->policy.extd.apic_id_size++;
>
> -p->policy.extd.nc = (p->policy.extd.nc << 1) | 1;
> +cur->policy.extd.nc = (cur->policy.extd.nc << 1) | 1;
> }
> break;
> }
> }
>
> -nr_leaves = ARRAY_SIZE(p->leaves);
> -rc = x86_cpuid_copy_to_buffer(>policy, p->leaves, _leaves);
> -if ( rc )
> +if ( xend || msr )
> {
> -ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
> -goto out;
> +// The overrides are over the serialised form of the policy
Comments should use /* */
> +if ( (rc = xc_cpu_policy_serialise(xch, cur)) )
> +goto out;
> +
> +if ( (rc = xc_cpuid_xend_policy(xch, domid, xend, host, def, cur)) )
> +goto out;
> +if ( (rc = xc_msr_policy(xch, domid, msr, host, def, cur)) )
> +goto out;
What if `xend` is set, but `msr` isn't? Looks like there's going to be a
segv in xc_msr_policy() because it doesn't check that `msr` is actually
set.
Thanks,
--
Anthony PERARD
Re: [PATCH 2/3] tools/xg: Streamline cpu policy serialise/deserialise calls
On Wed, Feb 07, 2024 at 05:39:56PM +, Alejandro Vallejo wrote: > The idea is to use xc_cpu_policy_t as a single object containing both the > serialised and deserialised forms of the policy. Note that we need lengths > for the arrays, as the serialised policies may be shorter than the array > capacities. > > * Add the serialised lengths to the struct so we can distinguish > between length and capacity of the serialisation buffers. > * Remove explicit buffer+lengths in serialise/deserialise calls > and use the internal buffer inside xc_cpu_policy_t instead. > * Refactor everything to use the new serialisation functions. > * Remove redundant serialization calls and avoid allocating dynamic > memory aside from the policy objects in xen-cpuid. Also minor cleanup > in the policy print call sites. > > No functional change intended. > > Signed-off-by: Alejandro Vallejo The patch looks fine beside xen-cpuid.c which might need to tweaked due to change needed in the first patch. Thanks, -- Anthony PERARD
Re: [PATCH 1/3] tools/xg: Move xc_cpu_policy_t to xenguest.h
On Wed, Feb 07, 2024 at 05:39:55PM +, Alejandro Vallejo wrote:
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index e01f494b772a..4e9078fdee4d 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -784,7 +784,13 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
>unsigned long *mfn0);
>
> #if defined(__i386__) || defined(__x86_64__)
> -typedef struct xc_cpu_policy xc_cpu_policy_t;
> +#include
I don't think it's a good idea to expose "cpu-policy.h" to "xenguest.h",
and it's going to break the build of every tools outside of xen
repository that are using xenguest.h. xenguest.h is installed on a
system, but cpu-policy.h isn't.
> +typedef struct xc_cpu_policy {
> +struct cpu_policy policy;
> +xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
> +xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
> +} xc_cpu_policy_t;
Would using an accessor be an option to access `leaves` and `msrs` for
"xen-cpuid" tool? That would avoid the need to expose the "cpu-policy.h"
headers.
With accessors, we might not need to expose xc_cpu_policy_serialise() to
the world anymore, and it could be internal to libxenguest, probably, I
haven't check.
Thanks,
--
Anthony PERARD
Re: [PATCH 2/2] tools: Drop libsystemd as a dependency
On Thu, Apr 25, 2024 at 06:32:16PM +0100, Andrew Cooper wrote:
> diff --git a/m4/systemd.m4 b/m4/systemd.m4
> index 112dc11b5e05..aa1ebe94f56c 100644
> --- a/m4/systemd.m4
> +++ b/m4/systemd.m4
> @@ -41,15 +41,6 @@ AC_DEFUN([AX_ALLOW_SYSTEMD_OPTS], [
> ])
>
> AC_DEFUN([AX_CHECK_SYSTEMD_LIBS], [
> - PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon],,
> - [PKG_CHECK_MODULES([SYSTEMD], [libsystemd >= 209])]
> -)
> - dnl pkg-config older than 0.24 does not set these for
> - dnl PKG_CHECK_MODULES() worth also noting is that as of version 208
> - dnl of systemd pkg-config --cflags currently yields no extra flags yet.
> - AC_SUBST([SYSTEMD_CFLAGS])
> - AC_SUBST([SYSTEMD_LIBS])
> -
> AS_IF([test "x$SYSTEMD_DIR" = x], [
> dnl In order to use the line below we need to fix upstream systemd
> dnl to properly ${prefix} for child variables in
> @@ -83,23 +74,11 @@ AC_DEFUN([AX_CHECK_SYSTEMD_LIBS], [
> AC_DEFUN([AX_CHECK_SYSTEMD], [
> dnl Respect user override to disable
> AS_IF([test "x$enable_systemd" != "xno"], [
> - AS_IF([test "x$systemd" = "xy" ], [
> - AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and enabled])
> - systemd=y
> - AX_CHECK_SYSTEMD_LIBS()
I think you need to keep calling AX_CHECK_SYSTEMD_LIBS() here,
otherwise, nothing sets $SYSTEMD_DIR or $SYSTEMD_MODULES_LOAD.
> - ],[
> - AS_IF([test "x$enable_systemd" = "xyes"],
> - [AC_MSG_ERROR([Unable to find systemd development
> library])],
> - [systemd=n])
> - ])
> + systemd=y
> ],[systemd=n])
> ])
>
> AC_DEFUN([AX_CHECK_SYSTEMD_ENABLE_AVAILABLE], [
> - PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon], [systemd="y"],[
> - PKG_CHECK_MODULES([SYSTEMD], [libsystemd >= 209],
> - [systemd="y"],[systemd="n"])
> - ])
Instead, or in addition, you could AX_AVAILABLE_SYSTEMD() in
configure.ac by AX_ENABLE_SYSTEMD(). (Or AX_ALLOW_SYSTEMD()).
With the current patch, AX_CHECK_SYSTEMD() will enable systemd
"support", even if it supposed to be disabled by default. So it's better
to use AX_ENABLE_SYSTEMD() as this will set the correct help message.
And can you add an entry in CHANGELOG? As systemd support isn't
automatically enabled with the presence of the libs anymore.
Thanks,
--
Anthony PERARD
Re: [PATCH] xen-livepatch: fix --force option comparison
On Fri, Apr 26, 2024 at 09:21:44AM +0200, Roger Pau Monne wrote:
> The check for --force option shouldn't be against 0.
>
> Reported-by: Jan Beulich
> Fixes: 62a72092a517 ('livepatch: introduce --force option')
> Signed-off-by: Roger Pau Monné
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On Thu, Apr 25, 2024 at 07:16:23PM +0100, Andrew Cooper wrote:
> On 25/04/2024 7:06 pm, Anthony PERARD wrote:
> > On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
> >> libsystemd is a giant dependency for one single function, but in the wake
> >> of
> >> the xz backdoor, it turns out that even systemd leadership recommend
> >> against
> >> linking against libsystemd for sd_notify().
> >>
> >> Since commit 7b61011e1450 ("tools: make xenstore domain easy
> >> configurable") in
> >> Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
> > That's not enough, it's needs to be `systemd-notify --ready` to be a
> > replacement for sd_notify(READY=1).
> >
> >> not even necessary for the xenstored's to call sd_notify() themselves.
> > So, sd_notify() or equivalent is still necessary.
> >
> >> Therefore, just drop the calls to sd_notify() and stop linking against
> >> libsystemd.
> > Sounds good, be we need to replace the call by something like:
> > echo READY=1 > $NOTIFY_SOCKET
> > implemented in C and ocaml. Detail to be checked.
> >
> > Otherwise, things won't work.
>
> Hmm. It worked in XenRT when stripping this all out, but that is
I don't know how XenServer is setup, maybe it doesn't matter? Anyway...
> extremely unintuitive behaviour for `systemd-notify --booted`, seeing as
> it's entirely different to --ready.
Yes, this --booted option should probably not exist, and there's
`systemctl is-system-running` that does something similar.
>
> I've got no interest in keeping the C around, but if:
>
> [ -n "$NOTIFY_SOCKET" ] && echo READY=1 > $NOTIFY_SOCKET
>
> works, then can't we just use that after waiting for the the pidfile ?
Run `systemd-notify --ready` instead. Hopefully, that will be enough.
($NOTIFY_SOCKET is a socket, and a bit more complicated that I though,
it can start with "@" for example)
Cheers,
--
Anthony PERARD
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
> libsystemd is a giant dependency for one single function, but in the wake of
> the xz backdoor, it turns out that even systemd leadership recommend against
> linking against libsystemd for sd_notify().
>
> Since commit 7b61011e1450 ("tools: make xenstore domain easy configurable") in
> Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
That's not enough, it's needs to be `systemd-notify --ready` to be a
replacement for sd_notify(READY=1).
> not even necessary for the xenstored's to call sd_notify() themselves.
So, sd_notify() or equivalent is still necessary.
> Therefore, just drop the calls to sd_notify() and stop linking against
> libsystemd.
Sounds good, be we need to replace the call by something like:
echo READY=1 > $NOTIFY_SOCKET
implemented in C and ocaml. Detail to be checked.
Otherwise, things won't work.
Thanks,
--
Anthony PERARD
[XEN PATCH] MAINTAINERS: Update my email address
Signed-off-by: Anthony PERARD --- MAINTAINERS | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index d1850c134d..6ba7d2765f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -208,7 +208,7 @@ Maintainers List (try to look for most precise areas first) 9PFSD M: Juergen Gross -M: Anthony PERARD +M: Anthony PERARD S: Supported F: tools/9pfsd/ @@ -381,7 +381,7 @@ F: xen/arch/x86/machine_kexec.c F: xen/arch/x86/x86_64/kexec_reloc.S LIBS -M: Anthony PERARD +M: Anthony PERARD R: Juergen Gross S: Supported F: tools/include/libxenvchan.h @@ -427,7 +427,7 @@ S: Supported F: tools/ocaml/ OVMF UPSTREAM -M: Anthony PERARD +M: Anthony PERARD S: Supported T: git https://xenbits.xenproject.org/git-http/ovmf.git @@ -460,7 +460,7 @@ T: git https://xenbits.xenproject.org/git-http/qemu-xen-traditional.git QEMU UPSTREAM M: Stefano Stabellini -M: Anthony Perard +M: Anthony Perard S: Supported T: git https://xenbits.xenproject.org/git-http/qemu-xen.git @@ -512,7 +512,7 @@ F: xen/arch/arm/include/asm/tee F: xen/arch/arm/tee/ TOOLSTACK -M: Anthony PERARD +M: Anthony PERARD S: Supported F: autogen.sh F: config/*.in -- Anthony PERARD
Re: [PATCH v4 1/4] xen-livepatch: fix parameter name parsing
On Wed, Apr 24, 2024 at 10:19:54AM +0200, Roger Pau Monne wrote:
> It's incorrect to restrict strncmp to the length of the command line input
> parameter, as then a user passing a rune like:
>
> % xen-livepatch up foo.livepatch
>
> Would match against the "upload" command, because the string comparison has
> been truncated to the length of the input argument. Use strcmp instead which
> doesn't truncate. Otherwise in order to keep using strncmp we would need to
> also check strings are of the same length before doing the comparison.
>
> Fixes: 05bb8afedede ('xen-xsplice: Tool to manipulate xsplice payloads')
> Signed-off-by: Roger Pau Monné
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check
On Tue, Apr 23, 2024 at 04:26:53PM +0100, Fouad Hilly wrote: > On Mon, Apr 22, 2024 at 6:57 PM Anthony PERARD > wrote: > > On Tue, Apr 16, 2024 at 10:15:46AM +0100, Fouad Hilly wrote: > > > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > > > index e3c1943e3633..4178fd2221ea 100644 > > > --- a/tools/misc/xen-ucode.c > > > +++ b/tools/misc/xen-ucode.c > > > @@ -24,7 +26,8 @@ static void usage(const char *name) > > > "Usage: %s [microcode file] [options]\n" > > > > Now, that usage line is wrong. The options needs to go before the file. > I am not sure what you mean "wrong"? from parsing perspective, both > scenarios can be successfully executed: > xen-ucode firmware-file --force > xen-ucode --force firmware-file > it becomes wrong if there is an argument expects the file as an input. Oh, sorry, I didn't know that getopt() would look for options on all arguments, even when separated by nonoptions. I'm used to CLI programs that takes options first, then files, even if that might work with a different order. Cheers, -- Anthony PERARD
Re: [PATCH v2 4/5] x86: Use getopt to handle command line args
On Tue, Apr 23, 2024 at 04:16:10PM +0100, Fouad Hilly wrote: > On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD > wrote: > > On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote: > > > +if ( argc != 2 ) > > > +goto ext_err; > > > > Why only two arguments allowed? And why check `argc` before calling > > getopt_long() ? > At this stage of the patch series, xen-ucode expects either firmware > file or a single argument, that is why argc should be 2. > If there is no argument or many arguments that are not supposed to be, > we exit with error other than try to parse the arguments. Right, but what's the point of introducing getopt_long() if we are not going to use it? The patch tries to make it nicer to introduce more options to the tool but then put an extra check that make this actually harder. This condition is even change in the next patch, that's one more reason that says that it's a wrong check. You can let getopt_long() parse all the options, deal with them, then you can deal with the nonoptions after that, and check that's there's only one nonoption. > > > + ext_err: > > > +fprintf(stderr, > > > +"xen-ucode: Xen microcode updating tool\n" > > > +"Usage: %s [microcode file] [options]\n", argv[0]); > > > > Isn't there a usage() function that we could use? > As there is an error, stderr should be used, which was a comment on V1. > > > > > +show_curr_cpu(stderr); > > > > Why call show_curr_cpu() on the error path? > Informative, but could be removed. We are on the error path, it's looks like the usage message is printed before the cpu information, so if the error is due to wrong options then that information is lost. We should print why there's an error, not much else would be useful. Error messages should be as late as possible or they are getting lost in the scroll of the terminal. So yes, it's probably best to remove. Thanks, -- Anthony PERARD
Re: [PATCH v3 2/4] livepatch: introduce --force option
On Tue, Apr 23, 2024 at 03:12:47PM +0200, Roger Pau Monne wrote: > Introduce a xen-livepatch tool --force option, that's propagated into the > hyerpvisor for livepatch operations. The intention is for the option to be > used to bypass some checks that would otherwise prevent the patch from being > loaded. > > Re purpose the pad field in xen_sysctl_livepatch_op to be a flags field that > applies to all livepatch operations. The flag is currently only set by the > hypercall wrappers for the XEN_SYSCTL_LIVEPATCH_UPLOAD operation, as that's so > far the only one where it will be used initially. Other uses can be added as > required. > > Note that helpers would set the .pad field to 0, that's been removed since the > structure is already zero initialized at definition. > > No functional usages of the new flag introduced in this patch. > > Signed-off-by: Roger Pau Monné For the tools: Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check
On Tue, Apr 16, 2024 at 10:15:46AM +0100, Fouad Hilly wrote: > Introduce --force option to xen-ucode to force skipping microcode version > check, which allows the user to update x86 microcode even if both versions > are the same. > > [v2] > 1- Changed data type from uint32_t to unsigned int. > 2- Corrected line length. > 3- Removed XENPF_UCODE_FLAG_FORCE_NOT_SET. > 4- Corrected indentations. > 5- Changed command line options to have the file name as first argument when > applicable. > 6- --force option doesn't require an argument anymore. > 7- Used optint to access filename in argv. > > Signed-off-by: Fouad Hilly > --- > > Suggested-by: Andrew Cooper You might want to move that tag before the '---' separation otherwise it wont be part of the commit message. `git am` discard every thing after the '---' line. Also add the tag before your SOB. > --- > tools/include/xenctrl.h | 3 ++- > tools/libs/ctrl/xc_misc.c | 13 +++-- > tools/misc/xen-ucode.c| 18 +- > 3 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > index e3c1943e3633..4178fd2221ea 100644 > --- a/tools/misc/xen-ucode.c > +++ b/tools/misc/xen-ucode.c > @@ -24,7 +26,8 @@ static void usage(const char *name) > "Usage: %s [microcode file] [options]\n" Now, that usage line is wrong. The options needs to go before the file. That could be fix on the previous patch. With that usage line, we would want to run `./xen-ucode ucode.bin --force`, but I don't think that would work. > "Options:\n" > " -h, --helpdisplay this help and exit\n" > - " -s, --show-cpu-info show CPU information and exit\n", > + " -s, --show-cpu-info show CPU information and exit\n" > + " -f, --force force to skip micorocde version check\n", > name, name); > } > Thanks, -- Anthony PERARD
Re: [PATCH v2 4/5] x86: Use getopt to handle command line args
On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index 0c0b2337b4ea..e3c1943e3633 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
>
> static xc_interface *xch;
>
> @@ -22,7 +23,8 @@ static void usage(const char *name)
> printf("%s: Xen microcode updating tool\n"
> "Usage: %s [microcode file] [options]\n"
> "Options:\n"
> - "show-cou-info show CPU information and exit\n",
> + " -h, --helpdisplay this help and exit\n"
> + " -s, --show-cpu-info show CPU information and exit\n",
> name, name);
> }
>
> @@ -86,6 +88,13 @@ int main(int argc, char *argv[])
> char *filename, *buf;
> size_t len;
> struct stat st;
> +int opt;
> +
> +static const struct option options[] = {
> +{"help", no_argument, NULL, 'h'},
> +{"show-cpu-info", no_argument, NULL, 's'},
> +{NULL, no_argument, NULL, 0}
> +};
>
> xch = xc_interface_open(NULL, NULL, 0);
> if ( xch == NULL )
> @@ -95,19 +104,22 @@ int main(int argc, char *argv[])
> exit(1);
> }
>
> -if ( argc < 2 )
> -{
> -fprintf(stderr,
> -"xen-ucode: Xen microcode updating tool\n"
> -"Usage: %s [ | show-cpu-info]\n", argv[0]);
> -show_curr_cpu(stderr);
> -exit(2);
> -}
> +if ( argc != 2 )
> +goto ext_err;
Why only two arguments allowed? And why check `argc` before calling
getopt_long() ?
>
> -if ( !strcmp(argv[1], "show-cpu-info") )
> +while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
> {
> -show_curr_cpu(stdout);
> -return 0;
> +switch (opt)
> +{
> +case 'h':
> +usage(argv[0]);
> +exit(EXIT_SUCCESS);
> +case 's':
> +show_curr_cpu(stdout);
> +exit(EXIT_SUCCESS);
> +default:
> +goto ext_err;
> +}
> }
>
> filename = argv[1];
So, after calling getopt_long(), the variable `optind` point to the next
argument that getopt_long() didn't recognize as an argument. It would be
a good time to start using it, and check that the are actually argument
left on the command line, even if in the current patch the only possible
outcome is that argv[1] has something that isn't an option.
> @@ -152,4 +164,11 @@ int main(int argc, char *argv[])
> close(fd);
>
> return 0;
> +
> + ext_err:
> + fprintf(stderr,
> +"xen-ucode: Xen microcode updating tool\n"
> +"Usage: %s [microcode file] [options]\n", argv[0]);
Isn't there a usage() function that we could use?
> +show_curr_cpu(stderr);
Why call show_curr_cpu() on the error path?
> +exit(STDERR_FILENO);
Still not an exit value.
Thanks,
--
Anthony PERARD
Re: [PATCH v2 3/5] x86: Add usage() to print out usage message
On Tue, Apr 16, 2024 at 10:15:44AM +0100, Fouad Hilly wrote:
> Refactor xen-ucode tool by adding usage() to handle usage\help messages
> As we add more command options this will keep help\usage messages in common
> block
>
> [v2]
> 1- Improved message description.
> 2- Fixed formatting and indentation.
> 3- Error message to print to stderr.
>
> Signed-off-by: Fouad Hilly
> ---
> tools/misc/xen-ucode.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index c6ae6498d659..0c0b2337b4ea 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -17,6 +17,15 @@ static xc_interface *xch;
> static const char intel_id[] = "GenuineIntel";
> static const char amd_id[] = "AuthenticAMD";
>
> +static void usage(const char *name)
> +{
> +printf("%s: Xen microcode updating tool\n"
> + "Usage: %s [microcode file] [options]\n"
> + "Options:\n"
> + "show-cou-info show CPU information and exit\n",
Don't change the usage message just yet. It still is
"Usage: %s [ | show-cpu-info]"
The current one mean we can run one of:
./xen-ucode ucode.bin
./xen-ucode show-cpu-info
The proposed help message in this patch mean we could have one of:
./xen-ucode ucode.bin
./xen-ucode show-cpu-info
./xen-ucode ucode.bin show-cpu-info
But that last one is an error.
Thanks,
--
Anthony PERARD
Re: [PATCH v3 4/4] docs/man: Add xenwatchdog manual page
On Thu, Apr 11, 2024 at 07:20:23PM +0100, le...@solinno.co.uk wrote: > From: Leigh Brown > > Add a manual page for xenwatchdogd. > > Signed-off-by: Leigh Brown Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 2/4] tools/misc: xenwatchdogd enhancements
On Thu, Apr 11, 2024 at 07:20:21PM +0100, le...@solinno.co.uk wrote: > From: Leigh Brown > > Add usage() function, the ability to run in the foreground, and > the ability to disarm the watchdog timer when exiting. > > Add enhanced parameter parsing and validation, making use of > getopt_long(). Check the number of parameters are correct, the > timeout is at least two seconds (to allow a minimum sleep time of > one second), and that the sleep time is at least one and less > than the watchdog timeout. > > With these changes, the daemon will no longer instantly reboot > the domain if you enter a zero timeout (or non-numeric parameter), > and prevent the daemon consuming 100% of a CPU due to zero sleep > time. > > Signed-off-by: Leigh Brown Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 3/4] tools/misc: Add xenwatchdogd.c copyright notice
On Thu, Apr 11, 2024 at 07:20:22PM +0100, le...@solinno.co.uk wrote: > From: Leigh Brown > > Add copyright notice and description of the program. > > Signed-off-by: Leigh Brown > > --- > tools/misc/xenwatchdogd.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c > index a16d1efc13..2884ca6ca9 100644 > --- a/tools/misc/xenwatchdogd.c > +++ b/tools/misc/xenwatchdogd.c > @@ -1,3 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * xenwatchdogd.c > + * > + * Watchdog based on Xen hypercall watchdog interface. > + * > + * Copyright 2010 Citrix Ltd > + * Copyright 2024 Leigh Brown > + * > + */ Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 1/4] tools/misc: xenwatchdogd: add parse_secs()
On Thu, Apr 11, 2024 at 07:20:20PM +0100, le...@solinno.co.uk wrote: > From: Leigh Brown > > Create a new parse_secs() function to parse the timeout and sleep > parameters. This ensures that non-numeric parameters are not > accidentally treated as numbers. > > Signed-off-by: Leigh Brown Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [RFC PATCH v2 2/2] libs/light: expand device model start timeout use
On Thu, Apr 11, 2024 at 03:28:20PM +0300, Manos Pitsidianakis wrote: > Various timeout values that depend on the device model should also > respect the device model start timeout setting. This commit adds the > __libxl_device_model_start_timeout() value to those time outs without > changing their default values. I don't like much this patch. It multiplies all qmp_cmd timeouts by 7. First, could you make two separate patches for the changes? One to change LIBXL_QMP_CMD_TIMEOUT, and one to change LIBXL_STUBDOM_START_TIMEOUT. Second, when the environment variable is unset, the timeout should stay at 10 for the qmp_cmd one. If for some reason that chosen value is to low, we could always have a separate patch to adjust the value. Cheers, -- Anthony PERARD
Re: [RFC PATCH v2 1/2] libs/light: add device model start timeout env var
alue will be use by libxl__xswait_start(). > if (rc) goto out; > return; > out: > @@ -1976,7 +1976,7 @@ static void do_pci_remove(libxl__egc *egc, > pci_remove_state *prs) > prs->xswait.what = "Device Model"; > prs->xswait.path = DEVICE_MODEL_XS_PATH(gc, > libxl_get_stubdom_id(CTX, domid), domid, "/state"); > -prs->xswait.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000; > +prs->xswait.timeout_ms = __libxl_device_model_start_timeout() * > 1000; That value will be use by libxl__xswait_start(). > prs->xswait.callback = pci_remove_qemu_trad_watch_state_cb; > rc = libxl__xswait_start(gc, >xswait); > if (rc) goto out_fail; So, overall, I'm fine with been able to change the timeout via env var, but disabling the timeout might not be easy or possible, even if "-1" might be an option, or -1000 in most case. So, you change the man page to only allow value>0 ? And a timeout of 0 might be best to revert to the default. Thanks, -- Anthony PERARD
Re: [PATCH v2 1/3] hotplug: Update block-tap
On Sun, Apr 07, 2024 at 04:49:51PM -0400, Jason Andryuk wrote: > diff --git a/tools/hotplug/Linux/block-tap b/tools/hotplug/Linux/block-tap > index 89247921b9..126e472786 100755 > --- a/tools/hotplug/Linux/block-tap > +++ b/tools/hotplug/Linux/block-tap > @@ -18,11 +18,11 @@ > # > # Usage: > # > -# Target should be specified using the following syntax: > +# Disks should be specified using the following syntax: > # > -# script=block-tap,vdev=xvda,target=: > +# vdev=xvda,backendtype=tap,format=vhd,target=/srv/target.vhd I still have unanswered question from the previous round: Is `block-tap` still going to work with the current example given in the script header? That is: "script=block-tap,vdev=xvda,target=:" Or maybe, that example is already broken? If it's not broken, there could be users which rely on it. But maybe it's not really broken, and the new syntax is better anyway. My guess is that using "script=block-tap,..." might still work, but we should say something in the CHANGELOG to encourage people to move to the new syntax, with "backendtype=tap" to avoid issues. In any case, the patch looks good: Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [OSSTEST PATCH 00/36] Switch to Debian Bookworm
On Mon, Apr 08, 2024 at 10:29:08AM +0100, Anthony PERARD wrote: > On Fri, Apr 05, 2024 at 04:54:30PM +0100, Anthony PERARD wrote: > > On Thu, Mar 28, 2024 at 05:54:04PM +, Anthony PERARD wrote: > > > On Mon, Mar 18, 2024 at 04:55:09PM +0000, Anthony PERARD wrote: > > > > I've now pushed > > "production-config: Set Bookworm's debian-installer version" > > > > and these two: > > > > > > Switch to Debian Bookworm as default suite > > > > make-hosts-flight: default to bookworm > > > > osstest should start to use Debian Bookworm soon. > > osstest failed it's own push gate, I didn't notice that the Arm* builds > was still using an old kernel (from our linux-arm-xen branch) instead of > 6.1. So I've rewind this push and pushed an other fix instead: > https://lore.kernel.org/xen-devel/20240408092542.36711-1-anthony.per...@citrix.com/ I've pushed the switch to Debian Bookworm again, and this time it when through. osstest have started to use Bookworm for testing. Cheers, -- Anthony PERARD
Re: [PATCH v2] libxl: Enable stubdom cdrom changing
logging about a "query-fdsets" command been ran.
> +rc = libxl__ev_time_register_rel(cis->ao, >timeout_retry,
> + cdrom_insert_stubdom_query_fdset,
> + 200);
> +if (rc) goto out;
> +
> +return;
> +
> + found:
> +cis->stubdom_fdset = fdset;
> +
> +LOGD(DEBUG, cis->domid, "Found fdset %d", cis->stubdom_fdset);
> +cdrom_insert_ejected(egc, >qmp, NULL, rc);
> +return;
> +
> + out:
> +cdrom_insert_done(egc, cis, rc); /* must be last */
> +}
> +
> static void cdrom_insert_ejected(libxl__egc *egc,
> libxl__ev_qmp *qmp,
> const libxl__json_object *response,
> @@ -1081,10 +1417,13 @@ static void cdrom_insert_ejected(libxl__egc *egc,
> rc = libxl__dm_check_start(gc, _config, domid);
> if (rc) goto out;
>
> +/* A linux stubdom will perform add-fd with calculated stubdom_fdset. */
Left over comment?
> if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN &&
> +libxl_get_stubdom_id(CTX, cis->domid) == 0 &&
> disk->format != LIBXL_DISK_FORMAT_EMPTY) {
> libxl__json_object *args = NULL;
>
> +LOGD(DEBUG, cis->domid, "Doing QMP add-fd path");
I don't think this logging is useful.
> assert(qmp->payload_fd == -1);
> qmp->payload_fd = open(disk->pdev_path, O_RDONLY);
> if (qmp->payload_fd < 0) {
> @@ -1094,7 +1433,11 @@ static void cdrom_insert_ejected(libxl__egc *egc,
> goto out;
> }
>
> -/* This free form parameter is not use by QEMU or libxl. */
> +/*
> + * This free form parameter is not used by QEMU or non-stubdom libxl.
> + * For a linux stubdom, opaque is set to "stub-devid:$devid" to
> + * identify the fdset.
> + */
Nothing is going to use this particular "opaque" value, right? The
comment say that for stubdom, the value is set to a particular value,
but it's not done so here. A comment about "stub-devid:$devid" might be
useful in query_fdsets_find_fdset(), but not here.
> QMP_PARAMETERS_SPRINTF(, "opaque", "%s:%s",
> libxl_disk_format_to_string(disk->format),
> disk->pdev_path);
> @@ -1103,6 +1446,7 @@ static void cdrom_insert_ejected(libxl__egc *egc,
> if (rc) goto out;
> has_callback = true;
> } else {
> +LOGD(DEBUG, cis->domid, "Skipping QMP add-fd path");
I think we can found out about that because "add-fd" isn't called. It
doesn't feel like a useful logging.
> has_callback = false;
> }
>
> @@ -1115,8 +1459,16 @@ out:
> if (rc) {
> cdrom_insert_done(egc, cis, rc); /* must be last */
> } else if (!has_callback) {
> -/* Only called if no asynchronous callback are set. */
This comment should still apply, even if we run for a stubdom,
otherwise, `has_callback` is badly named.
> -cdrom_insert_inserted(egc, qmp, NULL, 0); /* must be last */
> +if (libxl_get_stubdom_id(CTX, cis->domid) &&
> +disk->format != LIBXL_DISK_FORMAT_EMPTY) {
> +LOGD(DEBUG, cis->domid,
> + "stubdom %d needs to perform add-fd internally",
> + libxl_get_stubdom_id(CTX, cis->domid));
> +cdrom_insert_addfd_cb(egc, qmp, NULL, 0); /* must be last */
> +} else {
> +/* Only called if no asynchronous callback are set. */
> +cdrom_insert_inserted(egc, qmp, NULL, 0); /* must be last */
> +}
> }
> }
>
> @@ -1135,17 +1487,22 @@ static void cdrom_insert_addfd_cb(libxl__egc *egc,
> /* convenience aliases */
> libxl_device_disk *disk = cis->disk;
>
> -close(qmp->payload_fd);
> -qmp->payload_fd = -1;
I think I'd rather keep this here, before the error check. Just call
close() conditionally on the value of payload_fd? The thing is, if
payload_fd is still set, libxl__ev_qmp* is going to try to submit the fd
again, and closing the fd earlier is better.
> if (rc) goto out;
>
> -o = libxl__json_map_get("fdset-id", response, JSON_INTEGER);
> -if (!o) {
> -rc = ERROR_FAIL;
> -goto out;
> +/* response non-NULL only for non-stubdom */
> +if (response) {
I'm not comfortable with checking the value of `response` to find out if
we are on the linux stubdom path. It could be missing for other reason
(probably on programming error). There's `cis->stubdom_fdset` that
should have a correct value in the linux stubdom case, can we rely on
that instead? (Which also refer to why I asked early to init this field)
> +close(qmp->payload_fd);
> +qmp->payload_fd = -1;
> +
> +o = libxl__json_map_get("fdset-id", response, JSON_INTEGER);
> +if (!o) {
> +rc = ERROR_FAIL;
> +goto out;
> +}
> +fdset = libxl__json_object_get_integer(o);
> +} else {
> +fdset = cis->stubdom_fdset;
> }
> -fdset = libxl__json_object_get_integer(o);
>
> devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
> qmp->callback = cdrom_insert_inserted;
Thanks,
--
Anthony PERARD
Re: [PATCH] osstest: increase boot timeout for Debian PV guests
On Mon, Apr 15, 2024 at 10:33:19AM +0200, Roger Pau Monné wrote: > On Mon, Apr 15, 2024 at 09:15:51AM +0100, Anthony PERARD wrote: > > On Fri, Apr 12, 2024 at 04:11:21PM +0200, Roger Pau Monne wrote: > > > The current timeout of 40s seems to be too low for AMD boxes (pinots and > > > rimavas) in the lab after XSA-455, see: > > > > There's something else we can tweak if only some machine need extra > > time, it is an host property "TimeoutFactor", which can increase all > > timeout for a single machine. (It's use on the cubietruck for example.) > > > > Or is it better to just increase boot time for all (or at least those) > > pv guest? > > I did consider that, but given the timeout is just limited to PV guest > startup I considered the "TimeoutFactor" too broad. I think > increasing the Debian PV boot timeout from 40s to 60s is a minor > adjustment, and shouldn't affect other tests. > > Let me know if you still prefer to use "TimeoutFactor" and I will look > into it. Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
[OSSTEST PATCH] production-config: override mirror url for buster, use archive
buster-backport isn't available on the main mirror anymore. Signed-off-by: Anthony PERARD --- Notes: I've tested the patch already, so I'll push that soon. production-config | 2 ++ 1 file changed, 2 insertions(+) diff --git a/production-config b/production-config index 6345c40c..5b0c640d 100644 --- a/production-config +++ b/production-config @@ -95,6 +95,8 @@ TftpDiVersion_buster 2023-06-20 DebianSnapshotBackports_jessie http://snapshot.debian.org/archive/debian/20190206T211314Z/ +DebianMirror_buster http://archive.debian.org/debian/ + # For ISO installs DebianImageVersion_wheezy 7.2.0 DebianImageVersion_jessie 8.2.0 -- Anthony PERARD
Re: [PATCH] osstest: increase boot timeout for Debian PV guests
On Fri, Apr 12, 2024 at 04:11:21PM +0200, Roger Pau Monne wrote: > The current timeout of 40s seems to be too low for AMD boxes (pinots and > rimavas) in the lab after XSA-455, see: There's something else we can tweak if only some machine need extra time, it is an host property "TimeoutFactor", which can increase all timeout for a single machine. (It's use on the cubietruck for example.) Or is it better to just increase boot time for all (or at least those) pv guest? Cheers, -- Anthony PERARD
Re: [RFC PATCH v1 0/2] convert LIBXL_DEVICE_MODEL_START_TIMEOUT to env var
On Thu, Apr 11, 2024 at 01:23:07PM +0300, Manos Pitsidianakis wrote: > Hello Anthony, > > I do know about that, in fact I used it and it did not output your > email. Here's what the `get_maintainer.pl` outputs: (same output for > all patches): > > $ ./scripts/get_maintainer.pl --no-l > ../../patches/libxl-dm-timeout-v1/v1-0001-libs-light-add-device-model-start-timeout-env-var.patch > Andrew Cooper > George Dunlap > Jan Beulich > Julien Grall > Stefano Stabellini Oh, sorry, I didn't realise this was "THE REST" that is the list of default maintainers. So for some reason, the script fail to parse the patches to find which files are modified. And I think I know why. Usually, `git format-patch` output something like: diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c index 4369fef161..9ffdd50c69 100644 --- a/tools/libs/light/libxl_dm.c +++ b/tools/libs/light/libxl_dm.c with all path starting with "a/" or "b/", which get_maintainer then strip. (It strip them from the "diff --git" line, but remove the first "directory" from the path for line starting with "---" or "+++".) The patches in this series instead look like this: diff --git tools/libs/light/libxl_dm.c tools/libs/light/libxl_dm.c index 4369fef161..9ffdd50c69 100644 --- tools/libs/light/libxl_dm.c +++ tools/libs/light/libxl_dm.c with "a/" and "b/" missing, and that confuses get_maintainer.pl. It tries to find maintainers for "libs/light/libxl_dm.c" which don't exist. Did you by any chance used "--no-prefix"? It doesn't seems worse it to handle this case as I'm guessing more than on tool rely on those. Cheers, -- Anthony PERARD
Re: [RFC PATCH v1 0/2] convert LIBXL_DEVICE_MODEL_START_TIMEOUT to env var
On Wed, Apr 10, 2024 at 02:43:13PM +0300, Manos Pitsidianakis wrote: > This patch series proposes converting the compile-time define > LIBXL_DEVICE_MODEL_START_TIMEOUT value to an optionally overridden by > environment variable value, just like the current behavior for > LIBXL_BOOTLOADER_TIMEOUT is. > > Manos Pitsidianakis (2): > libs/light: add device model start timeout env var > libs/light: expand device model start timeout use > > docs/man/xl.1.pod.in | 11 +++ > tools/libs/light/libxl_9pfs.c| 2 +- > tools/libs/light/libxl_device.c | 2 +- > tools/libs/light/libxl_dm.c | 10 +- > tools/libs/light/libxl_dom_suspend.c | 2 +- > tools/libs/light/libxl_domain.c | 5 +++-- > tools/libs/light/libxl_internal.h| 6 ++ > tools/libs/light/libxl_pci.c | 10 +- > tools/libs/light/libxl_usb.c | 8 > 9 files changed, 37 insertions(+), 19 deletions(-) > > > base-commit: f48299cad5c3c69fdc2c101517a6dab9c9827ea5 > -- > γαῖα πυρί μιχθήτω > > Hi Manos, Did you know that you could run something like `git send-email --cc-cmd="scripts/get_maintainer.pl --no-l" ...` and git would CC the maintainers of the code? I've configure my xen repo to have that been automatic, with git config sendemail.cccmd 'cd `git rev-parse --show-toplevel` && scripts/get_maintainer.pl --no-l' There's other way to send patch, like using "scripts/add_maintainers.pl" described on this page: https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Sending_a_Patch_Series Not sure which one is better. Anyway, I've added the series to my list of patch to look at. But I might miss it next time if I'm not CCed. Cheers, -- Anthony PERARD
Re: [PATCH 5/5] x86: Add --force option to xen-ucode to override microcode version check
On Fri, Apr 05, 2024 at 01:11:28PM +0100, Fouad Hilly wrote:
> diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
> index 5ecdfa2c7934..edce45bc2a17 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -21,10 +23,11 @@ static const char amd_id[] = "AuthenticAMD";
> static void usage(const char *name)
> {
> printf("%s: Xen microcode updating tool\n"
> -"Usage: %s [ | --show-cpu-info]\n"
> +"Usage: %s [[--force] | --show-cpu-info]\n"
How about "Usage: %s [OPTIONS..] [MICROCODE FILE]" ?
> "\n"
> " -h, --helpdisplay this help and exit\n"
> " -s, --show-cpu-info show CPU information and exit\n"
> +" -f, --force force to skip micorocde version check\n"
typo: microcode ;-)
> "\n"
> , name, name);
> }
> @@ -89,11 +92,13 @@ int main(int argc, char *argv[])
> char *filename = NULL, *buf;
> size_t len;
> struct stat st;
> +uint32_t ucode_flag = XENPF_UCODE_FLAG_FORCE_NOT_SET;
> int opt;
>
> const static struct option options[] = {
> {"help", no_argument, NULL, 'h'},
> {"show-cpu-info", no_argument, NULL, 's'},
> +{"force", required_argument, NULL, 'f'},
This is weird, could you do without the argument?
It is weird because sometime "microcode file" is an argument of
"--force", sometime it is part of the rests of the options.
> {NULL, no_argument, NULL, 0}
> };
>
> @@ -105,10 +110,10 @@ int main(int argc, char *argv[])
> exit(1);
> }
>
> -if ( argc != 2 )
> +if ( argc < 2 || argc > 3)
> goto ext_err;
>
> -while ( (opt = getopt_long(argc, argv, "hs:", options, NULL)) != -1 )
> +while ( (opt = getopt_long(argc, argv, "hsf:", options, NULL)) != -1 )
> {
> switch (opt)
> {
> @@ -120,12 +125,17 @@ int main(int argc, char *argv[])
> goto ext_err;
> show_curr_cpu(stdout);
> exit(EXIT_SUCCESS);
> +case 'f':
> +ucode_flag = XENPF_UCODE_FLAG_FORCE_SET;
> +filename = optarg;
> +break;
> default:
> goto ext_err;
> }
> }
>
> -filename = argv[1];
> +if ( argc == 2 )
> +filename = argv[1];
Sometime we take filename from argv[1], sometime we don't? The logic is
going to be very confusing, takeout of context, only set `filename` from a
single place please.
Thanks,
--
Anthony PERARD
Re: [PATCH 4/5] x86: Use getopt to handle command line args
On Fri, Apr 05, 2024 at 01:11:27PM +0100, Fouad Hilly wrote:
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index 1edcebfb9f9c..9bde991c5df5 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
>
> static xc_interface *xch;
>
> @@ -20,7 +21,10 @@ static const char amd_id[] = "AuthenticAMD";
> static void usage(const char *name)
> {
> printf("%s: Xen microcode updating tool\n"
> -"Usage: %s [ | show-cpu-info]\n"
> +"Usage: %s [ | --show-cpu-info]\n"
This look like a change worth mentioning to users, can we add something
in the CHANGELOG to say "show-cpu-info" is no longer an option and
users/admin should use "--show-cpu-info" instead?
> +"\n"
> +" -h, --helpdisplay this help and exit\n"
> +" -s, --show-cpu-info show CPU information and exit\n"
> "\n"
> , name, name);
> }
> @@ -82,9 +86,16 @@ static void show_curr_cpu(FILE *f)
> int main(int argc, char *argv[])
> {
> int fd, ret;
> -char *filename, *buf;
> +char *filename = NULL, *buf;
> size_t len;
> struct stat st;
> +int opt;
> +
> +const static struct option options[] = {
> +{"help", no_argument, NULL, 'h'},
> +{"show-cpu-info", no_argument, NULL, 's'},
> +{NULL, no_argument, NULL, 0}
> +};
>
> xch = xc_interface_open(NULL, NULL, 0);
> if ( xch == NULL )
> @@ -94,20 +105,33 @@ int main(int argc, char *argv[])
> exit(1);
> }
>
> -if ( argc < 2 )
> +if ( argc != 2 )
This is overly restrictive, and doesn't need to be, especially when this
patch introduces the use of getopt_long().
> +goto ext_err;
> +
> +while ( (opt = getopt_long(argc, argv, "hs:", options, NULL)) != -1 )
`-s` requires an argument but `--show-cpu-info`, looks there's an extra
':' in the `optstring`, it should read "hs", not "hs:".
> {
> -usage(argv[0]);
> -show_curr_cpu(stderr);
> -exit(2);
> +switch (opt)
> +{
> +case 'h':
> +usage(argv[0]);
> +exit(EXIT_SUCCESS);
> +case 's':
> +if ( argc > 2 )
Why is `-s` only allowed alone? What if want to include some other
option like "--json" to print the cpu-info in a different format?
I think one way to deal with this would be to record the fact that we
want to display the cpu information, and after the getopt_long() loop,
check that they are no more arguments. (Check out `optind` in the man page)
> +goto ext_err;
> +show_curr_cpu(stdout);
> +exit(EXIT_SUCCESS);
> +default:
> +goto ext_err;
> +}
> }
>
> -if ( !strcmp(argv[1], "show-cpu-info") )
> +filename = argv[1];
> +if ( filename == NULL )
> {
> -show_curr_cpu(stdout);
> -return 0;
> +printf("File name error\n");
> +goto ext_err;
> }
>
> -filename = argv[1];
> fd = open(filename, O_RDONLY);
> if ( fd < 0 )
> {
> @@ -149,4 +173,9 @@ int main(int argc, char *argv[])
> close(fd);
>
> return 0;
> +
> +ext_err:
> +usage(argv[0]);
> +show_curr_cpu(stderr);
Why is show_curr_cpu() called on an error path?
> +exit(STDERR_FILENO);
STDERR_FILENO isn't an exit code, it's a file descriptor.
Thanks,
--
Anthony PERARD
Re: [PATCH v2 0/6] xenwatchdogd bugfixes and enhancements
On Fri, Mar 29, 2024 at 11:10:50AM +, le...@solinno.co.uk wrote: > From: Leigh Brown > > The primary intention of this patch series is to replace the > pathologically bad behaviour of rebooting the domain if you run > "xenwatchdogd -h". To that end, I have implemented comprehensive > argument validation. This validation ensures you can't pass > arguments that instantly reboot the domain or cause it to spin > loop running sleep(0) repeatedly. > > I added a couple of enhancements whilst working on the changes as > they were easy enough. In particular, being able to run in the > foreground, disarming the watchdog on exit, help and a manpage. > > Changes since v1: > - Add Signed-off-by on every patch. > - Make global variables static. > > Full list of changes: > - Use getopt_long() to add -h/--help with associated usage help. > - Add -F/--foreground parameter to run without daemonizing. > - Add -x/--save-exit parameter to disarm the watchdog when exiting. > - Validate timeout is numeric and is at least two seconds. > - Validate sleep is numeric and is at least one and less than timeout. > - Check for too many arguments. > - Use symbol constants instead of magic numbers where possible. > - Make all functions except main() and global variables static. > - Add a manual page for xenwatchdogd(). It might be worth mentioning something about these in the CHANGELOG. Probably not need to go into detail, maybe just "improved xenwatchdogd options, see new man page." Cheers, -- Anthony PERARD
Re: [PATCH v2 6/6] docs/man: Add xenwatchdog manual page
On Fri, Mar 29, 2024 at 11:10:56AM +, le...@solinno.co.uk wrote: > diff --git a/docs/man/xenwatchdogd.8.pod b/docs/man/xenwatchdogd.8.pod > new file mode 100644 > index 00..2f6454f183 > --- /dev/null > +++ b/docs/man/xenwatchdogd.8.pod > @@ -0,0 +1,54 @@ > +=head1 NAME > + > +xenwatchdogd - Xen hypercall based watchdog daemon > + > +=head1 SYNOPSIS > + > +B [ I ] > [ > ] > + > +=head1 DESCRIPTION > + > +B arms the Xen watchdog timer to I every I > +seconds. If the xenwatchdogd process dies or is delayed for more than > +I seconds, then Xen will reboot the domain. Xen will not reboot the domain, it will just kill the domain with watchdog as explanation. I think the toolstack is in charge of rebooting the domain. There's a setting for `xl` created VM named on_watchdog="ACTION", which by default is "destroy". So it's more likely that the domain will be killed rather than rebooted. So something like: Depending on the configuration for the guest, the domain might be destroyed, rebooted, or other. See B in xl.cfg(5) > + If the domain being > +rebooted is domain 0, the whole system will reboot. Maybe something like "if B is running in dom0, the whole system will reboot". I'm not sure if the host reboot in this case by default, probably. > +=head1 SIGNALS > + > +B Will cause the program to disarm the watchdog timer and exit, > +regardless of whether the safe exit option was passed. "whether B<--safe-exit> option" .. I think it's better to call-out the option explicitly. Thanks, -- Anthony PERARD
Re: [PATCH v2 5/6] tools/misc: xenwatchdogd enhancements
On Fri, Mar 29, 2024 at 11:10:55AM +, le...@solinno.co.uk wrote: > From: Leigh Brown > > Add enhanced parameter parsing and validation, making use of > getopt_long(). Adds usage() function, ability to run in the foreground, > and the ability to disarm the watchdog timer when exiting. Now checks > the number of parameters are correct, that timeout is at least two > seconds (to allow a minimum sleep time of one second), and that the > sleep time is at least one and less than the watchdog timeout. After > these changes, the daemon will no longer instantly reboot the domain > if you enter a zero timeout (or non-numeric parameter), and prevent > the daemon consuming 100% of a CPU. Add a copyright message. This is > based on the previous commits which were from Citrix email addresses. This to me is really hard to read, it just looks like a blob of text, where it supposed to be a list with several modification listed. The part about the copyright should be in its own paragraph for example. > Signed-off-by: Leigh Brown > --- > tools/misc/xenwatchdogd.c | 111 ++ > 1 file changed, 101 insertions(+), 10 deletions(-) > > diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c > index 19ec4c5359..b78320f86d 100644 > --- a/tools/misc/xenwatchdogd.c > +++ b/tools/misc/xenwatchdogd.c > @@ -1,3 +1,20 @@ > +/* > + * xenwatchdogd.c > + * > + * Watchdog based on Xen hypercall watchdog interface > + * > + * Copyright 2010-2024 Citrix Ltd and other contributors This is probably more like: Copyright (C) 2010 Citrix Ltd. Copyright (C) 2024 *** your copyright here *** Because it's looks like the only contribution from us was in 2010, and I suppose it's fine to have more than one copyright line. > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU Lesser General Public License as published > + * by the Free Software Foundation; version 2.1 only. with the special > + * exception on linking described in file LICENSE. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU Lesser General Public License for more details. This might be the wrong license, the default license we use is GPL 2.0 only, not LGPL. See :/COPYING . These days, we prefer SPDX tags instead of the full licence text. So overall, the header of the file should look something like: /* SPDX-License-Identifier: GPL-2.0-only */ /* * xenwatchdogd.c * * Watchdog based on Xen hypercall watchdog interface * * Copyright (C) 2010 Citrix Ltd. * Copyright (C) 2024 *** your copyright here *** */ I don't know if adding the file name in that header is very useful, but I don't mind either way. Also, could you do this in a separate patch? > + */ > > #include > #include Nice change overall, it's just the license part that need fixing. Thanks, -- Anthony PERARD
Re: [PATCH v2 4/6] tools/misc: xenwatchdogd: add parse_secs()
On Fri, Mar 29, 2024 at 11:10:54AM +, le...@solinno.co.uk wrote:
> diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
> index 2e7f9f51c5..19ec4c5359 100644
> --- a/tools/misc/xenwatchdogd.c
> +++ b/tools/misc/xenwatchdogd.c
> @@ -49,6 +49,18 @@ static void catch_usr1(int sig)
> done = true;
> }
>
> +static int parse_secs(const char *arg, const char *what)
> +{
> +char *endptr;
> +unsigned long val;
> +
> +val = strtoul(arg, , 0);
> +if (val > INT_MAX || *endptr != 0)
nit: '\0' would be more accurate ^ here.
Otherwise, just `*endptr` without "!= 0" would work too.
But comparing a char to a digit feels wrong.
> + errx(EXIT_FAILURE, "invalid %s: '%s'", what, arg);
> +
> +return val;
> +}
> +
> int main(int argc, char **argv)
> {
> int id;
> @@ -64,16 +76,11 @@ int main(int argc, char **argv)
> if (h == NULL)
> err(EXIT_FAILURE, "xc_interface_open");
>
> -t = strtoul(argv[1], NULL, 0);
> -if (t == ULONG_MAX)
> - err(EXIT_FAILURE, "strtoul");
> +t = parse_secs(argv[optind], "timeout");
>
> s = t / 2;
> -if (argc == 3) {
> - s = strtoul(argv[2], NULL, 0);
> - if (s == ULONG_MAX)
> - err(EXIT_FAILURE, "strtoul");
> -}
> +if (argc == 3)
> + s = parse_secs(argv[optind], "sleep");
This is parsing the wrong value here, should it be +1 to parse the sleep
argument instead of the timeout argument a second time.
Also, are you sure you want to start using "optind" here? It's a
variable used by getopt(), but we don't use it yet. It almost feel like
"optind" happen to be set to 1 by luck.
Thanks,
--
Anthony PERARD
Re: [PATCH v2 3/6] tools/misc: xenwatchdogd: add static qualifier
On Fri, Mar 29, 2024 at 11:10:53AM +, le...@solinno.co.uk wrote: > From: Leigh Brown > > Make all functions except main() static in xenwatchdogd.c. Also make > the remaining global variable static. > > Signed-off-by: Leigh Brown Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2 2/6] tools/misc: rework xenwatchdogd signal handling
On Fri, Mar 29, 2024 at 11:10:52AM +, le...@solinno.co.uk wrote: > From: Leigh Brown > > Rework xenwatchdogd signal handling to do the minimum in the signal > handler. This is a very minor enhancement. > > Signed-off-by: Leigh Brown Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2 1/6] tools/misc: xenwatchdogd: use EXIT_* constants
On Fri, Mar 29, 2024 at 11:10:51AM +, le...@solinno.co.uk wrote: > From: Leigh Brown > > Use EXIT_SUCCESS/EXIT_FAILURE constants instead of magic numbers. > > Signed-off-by: Leigh Brown Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2] libxl: devd: Spawn QEMU for 9pfs
On Sun, Apr 07, 2024 at 04:58:09PM -0400, Jason Andryuk wrote: > Add support for xl devd to support 9pfs in a domU. devd need to spawn a > pvqemu for the domain to service 9pfs as well as qdisk backends. Rename > num_qdisks to pvqemu_refcnt to be more generic. > > Keep the qdisk-backend-pid xenstore key as well as the disk-%u log file. > They are externally visible, so the might be used by other tooling. nit: ^^^ they > > Signed-off-by: Jason Andryuk > Signed-off-by: Jason Andryuk > --- > v2: > Retain xenstore qdisk-backend-pid and qdisk-%u logfile Patch looks fine otherwise: Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH] xen-hvm: Avoid livelock while handling buffered ioreqs
On Thu, Apr 04, 2024 at 03:08:33PM +0100, Ross Lagerwall wrote:
> diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
> index 1627da739822..1116b3978938 100644
> --- a/hw/xen/xen-hvm-common.c
> +++ b/hw/xen/xen-hvm-common.c
> @@ -521,22 +521,30 @@ static bool handle_buffered_iopage(XenIOState *state)
[...]
>
> static void handle_buffered_io(void *opaque)
> {
> +unsigned int handled;
> XenIOState *state = opaque;
>
> -if (handle_buffered_iopage(state)) {
> +handled = handle_buffered_iopage(state);
> +if (handled >= IOREQ_BUFFER_SLOT_NUM) {
> +/* We handled a full page of ioreqs. Schedule a timer to continue
> + * processing while giving other stuff a chance to run.
> + */
./scripts/checkpatch.pl report a style issue here:
WARNING: Block comments use a leading /* on a separate line
I can try to remember to fix that on commit.
> timer_mod(state->buffered_io_timer,
> -BUFFER_IO_MAX_DELAY +
> qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
> -} else {
> +qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
> +} else if (handled == 0) {
Just curious, why did you check for `handled == 0` here instead of
`handled != 0`? That would have avoided to invert the last 2 cases, and
the patch would just have introduce a new case without changing the
order of the existing ones. But not that important I guess.
> timer_del(state->buffered_io_timer);
> qemu_xen_evtchn_unmask(state->xce_handle,
> state->bufioreq_local_port);
> +} else {
> +timer_mod(state->buffered_io_timer,
> + BUFFER_IO_MAX_DELAY +
> qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
> }
> }
Cheers,
--
Anthony PERARD
Re: Linux Xen PV CPA W^X violation false-positives
On Mon, Apr 08, 2024 at 11:22:59AM +0100, Anthony PERARD wrote: > On Thu, Mar 28, 2024 at 02:00:14PM +0100, Jürgen Groß wrote: > > Hi Jason, > > > > On 28.03.24 02:24, Jason Andryuk wrote: > > > On Wed, Mar 27, 2024 at 7:46 AM Jürgen Groß wrote: > > > > > > > > On 24.01.24 17:54, Jason Andryuk wrote: > > > > > + > > > > > + return new; > > > > > + } > > > > > + } > > > > > + > > > > >end = start + npg * PAGE_SIZE - 1; > > > > >WARN_ONCE(1, "CPA detected W^X violation: %016llx -> %016llx > > > > > range: 0x%016lx - 0x%016lx PFN %lx\n", > > > > > (unsigned long long)pgprot_val(old), > > > > > > > > Jason, do you want to send a V2 with your Signed-off, or would you like > > > > me to > > > > try upstreaming the patch? > > > > > > Hi Jürgen, > > > > > > Yes, please upstream your approach. I wasn't sure how to deal with > > > it, so it was more of a bug report. > > > > The final solution was a bit more complicated, as there are some > > corner cases to be considered. OTOH it is now complete by looking > > at all used translation entries. > > > > Are you able to test the attached patch? I don't see the original > > issue and can only verify the patch doesn't cause any regression. > > > > > > Juergen > > Hi Jürgen, > > I gave a try to the patch in this email with osstest, and I can't find a > single "CPA detected W^X violation" log entry, when there's seems to be > many in osstest in general, from dom0 it seems as it's on the host > serial console usually. > > http://logs.test-lab.xenproject.org/osstest/logs/185252/ > > If you look in several "serial-$host.log*" files, there will be the > "CPA detected" message, but they happen on previous test run. > > I did an other smaller run before this one, and same thing: > http://logs.test-lab.xenproject.org/osstest/logs/185186/ > > And this other run as well, which I failed to setup properly with lots > of broken, but no failure due to the patch and I can't find any "CPA > detected" messages. > http://logs.test-lab.xenproject.org/osstest/logs/185248/ > > I hope that helps? BTW, I did apply the patch to Linux 6.1: https://xenbits.xen.org/gitweb/?p=people/aperard/linux.git;a=shortlog;h=refs/heads/9b5a105a-650c-4b33-9f4e-03612fb67...@suse.com -- Anthony PERARD
Re: Linux Xen PV CPA W^X violation false-positives
return NULL;
> @@ -679,6 +689,9 @@ pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long
> address,
> if (p4d_leaf(*p4d) || !p4d_present(*p4d))
> return (pte_t *)p4d;
>
> + *nx |= p4d_flags(*p4d) & _PAGE_NX;
> + *rw &= p4d_flags(*p4d) & _PAGE_RW;
> +
> pud = pud_offset(p4d, address);
> if (pud_none(*pud))
> return NULL;
> @@ -687,6 +700,9 @@ pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long
> address,
> if (pud_leaf(*pud) || !pud_present(*pud))
> return (pte_t *)pud;
>
> + *nx |= pud_flags(*pud) & _PAGE_NX;
> + *rw &= pud_flags(*pud) & _PAGE_RW;
> +
> pmd = pmd_offset(pud, address);
> if (pmd_none(*pmd))
> return NULL;
> @@ -695,6 +711,9 @@ pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long
> address,
> if (pmd_leaf(*pmd) || !pmd_present(*pmd))
> return (pte_t *)pmd;
>
> + *nx |= pmd_flags(*pmd) & _PAGE_NX;
> + *rw &= pmd_flags(*pmd) & _PAGE_RW;
> +
> *level = PG_LEVEL_4K;
>
> return pte_offset_kernel(pmd, address);
> @@ -710,18 +729,24 @@ pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long
> address,
> */
> pte_t *lookup_address(unsigned long address, unsigned int *level)
> {
> - return lookup_address_in_pgd(pgd_offset_k(address), address, level);
> + bool nx, rw;
> +
> + return lookup_address_in_pgd(pgd_offset_k(address), address, level,
> + , );
> }
> EXPORT_SYMBOL_GPL(lookup_address);
>
> static pte_t *_lookup_address_cpa(struct cpa_data *cpa, unsigned long
> address,
> - unsigned int *level)
> + unsigned int *level, bool *nx, bool *rw)
> {
> - if (cpa->pgd)
> - return lookup_address_in_pgd(cpa->pgd + pgd_index(address),
> -address, level);
> + pgd_t *pgd;
> +
> + if (!cpa->pgd)
> + pgd = pgd_offset_k(address);
> + else
> + pgd = cpa->pgd + pgd_index(address);
>
> - return lookup_address(address, level);
> + return lookup_address_in_pgd(pgd, address, level, nx, rw);
> }
>
> /*
> @@ -849,12 +874,13 @@ static int __should_split_large_page(pte_t *kpte,
> unsigned long address,
> pgprot_t old_prot, new_prot, req_prot, chk_prot;
> pte_t new_pte, *tmp;
> enum pg_level level;
> + bool nx, rw;
>
> /*
>* Check for races, another CPU might have split this page
>* up already:
>*/
> - tmp = _lookup_address_cpa(cpa, address, );
> + tmp = _lookup_address_cpa(cpa, address, , , );
> if (tmp != kpte)
> return 1;
>
> @@ -965,7 +991,8 @@ static int __should_split_large_page(pte_t *kpte,
> unsigned long address,
> new_prot = static_protections(req_prot, lpaddr, old_pfn, numpages,
> psize, CPA_DETECT);
>
> - new_prot = verify_rwx(old_prot, new_prot, lpaddr, old_pfn, numpages);
> + new_prot = verify_rwx(old_prot, new_prot, lpaddr, old_pfn, numpages,
> + nx, rw);
>
> /*
>* If there is a conflict, split the large page.
> @@ -1046,6 +1073,7 @@ __split_large_page(struct cpa_data *cpa, pte_t *kpte,
> unsigned long address,
> pte_t *pbase = (pte_t *)page_address(base);
> unsigned int i, level;
> pgprot_t ref_prot;
> + bool nx, rw;
> pte_t *tmp;
>
> spin_lock(_lock);
> @@ -1053,7 +1081,7 @@ __split_large_page(struct cpa_data *cpa, pte_t *kpte,
> unsigned long address,
>* Check for races, another CPU might have split this page
>* up for us already:
>*/
> - tmp = _lookup_address_cpa(cpa, address, );
> + tmp = _lookup_address_cpa(cpa, address, , , );
> if (tmp != kpte) {
> spin_unlock(_lock);
> return 1;
> @@ -1594,10 +1622,11 @@ static int __change_page_attr(struct cpa_data *cpa,
> int primary)
> int do_split, err;
> unsigned int level;
> pte_t *kpte, old_pte;
> + bool nx, rw;
>
> address = __cpa_addr(cpa, cpa->curpage);
> repeat:
> - kpte = _lookup_address_cpa(cpa, address, );
> + kpte = _lookup_address_cpa(cpa, address, , , );
> if (!kpte)
> return __cpa_process_fault(cpa, address, primary);
>
> @@ -1619,7 +1648,8 @@ static int __change_page_attr(struct cpa_data *cpa, int
> primary)
> new_prot = static_protections(new_prot, address, pfn, 1, 0,
> CPA_PROTECT);
>
> - new_prot = verify_rwx(old_prot, new_prot, address, pfn, 1);
> + new_prot = verify_rwx(old_prot, new_prot, address, pfn, 1,
> + nx, rw);
>
> new_prot = pgprot_clear_protnone_bits(new_prot);
>
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index cffe1157a90a..9131d80cff36 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -338,12 +338,13 @@ void snp_dump_hva_rmpentry(unsigned long hva)
> {
> unsigned long paddr;
> unsigned int level;
> + bool nx, rw;
> pgd_t *pgd;
> pte_t *pte;
>
> pgd = __va(read_cr3_pa());
> pgd += pgd_index(hva);
> - pte = lookup_address_in_pgd(pgd, hva, );
> + pte = lookup_address_in_pgd(pgd, hva, , , );
>
> if (!pte) {
> pr_err("Can't dump RMP entry for HVA %lx: no PTE/PFN found\n",
> hva);
> --
> 2.35.3
>
--
Anthony PERARD
Re: [OSSTEST PATCH 00/36] Switch to Debian Bookworm
On Fri, Apr 05, 2024 at 04:54:30PM +0100, Anthony PERARD wrote: > On Thu, Mar 28, 2024 at 05:54:04PM +0000, Anthony PERARD wrote: > > On Mon, Mar 18, 2024 at 04:55:09PM +, Anthony PERARD wrote: > > I've now pushed > "production-config: Set Bookworm's debian-installer version" > > and these two: > > > > Switch to Debian Bookworm as default suite > > > make-hosts-flight: default to bookworm > > osstest should start to use Debian Bookworm soon. osstest failed it's own push gate, I didn't notice that the Arm* builds was still using an old kernel (from our linux-arm-xen branch) instead of 6.1. So I've rewind this push and pushed an other fix instead: https://lore.kernel.org/xen-devel/20240408092542.36711-1-anthony.per...@citrix.com/ Cheers, -- Anthony PERARD
[OSSTEST PATCH] cr-daily-branch: Actually use Linux 6.1 by default on Arm
Commit 95ee1714750b wasn't effective at changing the default version
of Linux, osstest kept on using "linux-arm-xen".
This time, make osstest use the same revision for both LINUX and
LINUX_ARM, like it was done for "linux*" branches only before.
If for some reason $BASE_TAG_LINUX and $BASE_TAG_LINUX_ARM are
different, this change would be wrong, but there's shouldn't be a need
for them to be different.
Fixes: 95ee1714750b ("ap-common: Switch to Linux 6.1 by default on Arm")
Signed-off-by: Anthony PERARD
---
Notes:
Already pushed.
ap-common | 2 +-
cr-daily-branch | 14 --
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/ap-common b/ap-common
index eabb85c6..5770d2b1 100644
--- a/ap-common
+++ b/ap-common
@@ -60,7 +60,7 @@
: ${PUSH_TREE_LINUX:=$XENBITS:/home/xen/git/linux-pvops.git}
: ${BASE_TREE_LINUX:=git://xenbits.xen.org/linux-pvops.git}
: ${BASE_TAG_LINUX:=tested/linux-6.1}
-: ${BASE_TAG_LINUX_ARM:=tested/linux-6.1}
+: ${BASE_TAG_LINUX_ARM:=${BASE_TAG_LINUX}}
if [ "x${TREE_LINUX}" = x ]; then
: ${TREE_LINUX:=${BASE_TREE_LINUX}}
diff --git a/cr-daily-branch b/cr-daily-branch
index 6ec3aa62..23e716e4 100755
--- a/cr-daily-branch
+++ b/cr-daily-branch
@@ -186,16 +186,10 @@ if [ "x$REVISION_LINUX" = x ]; then
export REVISION_LINUX
fi
if [ "x$REVISION_LINUX_ARM" = x ]; then
-if [ "x$tree" = "xlinux" ] ; then
- TREE_LINUX_ARM=$TREE_LINUX
- export TREE_LINUX_ARM
- REVISION_LINUX_ARM=$REVISION_LINUX
-export REVISION_LINUX_ARM
-else
- determine_version REVISION_LINUX_ARM ${linuxbranch:-linux-arm-xen} \
- LINUX_ARM
-export REVISION_LINUX_ARM
-fi
+TREE_LINUX_ARM=$TREE_LINUX
+export TREE_LINUX_ARM
+REVISION_LINUX_ARM=$REVISION_LINUX
+export REVISION_LINUX_ARM
fi
if [ "x$REVISION_SEABIOS" = x ]; then
case "$branch" in
--
Anthony PERARD
Re: [OSSTEST PATCH 00/36] Switch to Debian Bookworm
On Thu, Mar 28, 2024 at 05:54:04PM +, Anthony PERARD wrote: > On Mon, Mar 18, 2024 at 04:55:09PM +0000, Anthony PERARD wrote: I've now pushed "production-config: Set Bookworm's debian-installer version" and these two: > > Switch to Debian Bookworm as default suite > > make-hosts-flight: default to bookworm osstest should start to use Debian Bookworm soon. Cheers, -- Anthony PERARD
Re: [PATCH v3 2/2] xen: fix stubdom PCI addr
On Wed, Mar 27, 2024 at 04:05:15AM +0100, Marek Marczykowski-Górecki wrote: > When running in a stubdomain, the config space access via sysfs needs to > use BDF as seen inside stubdomain (connected via xen-pcifront), which is > different from the real BDF. For other purposes (hypercall parameters > etc), the real BDF needs to be used. > Get the in-stubdomain BDF by looking up relevant PV PCI xenstore > entries. > > Signed-off-by: Marek Marczykowski-Górecki Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 1/2] hw/xen: detect when running inside stubdomain
On Wed, Mar 27, 2024 at 04:05:14AM +0100, Marek Marczykowski-Górecki wrote: > Introduce global xen_is_stubdomain variable when qemu is running inside > a stubdomain instead of dom0. This will be relevant for subsequent > patches, as few things like accessing PCI config space need to be done > differently. > > Signed-off-by: Marek Marczykowski-Górecki Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [RFC XEN PATCH v6 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi
On Thu, Mar 28, 2024 at 02:34:02PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 2cec83e0b734..debf6ec6ddc7 100644
> @@ -1500,13 +1510,25 @@ static void pci_add_dm_done(libxl__egc *egc,
> rc = ERROR_FAIL;
> goto out;
> }
> -r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
> -if (r < 0) {
> -LOGED(ERROR, domainid,
> - "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> -fclose(f);
> -rc = ERROR_FAIL;
> -goto out;
> +if (is_gsi) {
> +r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
> +if (r < 0 && r != -EOPNOTSUPP) {
Is it correct to check `r` for "-EOPNOTSUPP" ? Because `man ioctl` and
"xenctrl.h:105" says that `r` is 0 on success or -1 on error with `errno`
set.
> +LOGED(ERROR, domainid,
> + "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, r);
Is the `r` value useful to print? Because LOGED() already prints
strerror(errno).
> +fclose(f);
> +rc = ERROR_FAIL;
> +goto out;
> +}
> +}
> +if (!is_gsi || r == -EOPNOTSUPP) {
> +r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
> +if (r < 0) {
> +LOGED(ERROR, domainid,
> +"xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> +fclose(f);
> +rc = ERROR_FAIL;
> +goto out;
> +}
> }
> }
> fclose(f);
> @@ -2180,6 +2202,7 @@ static void pci_remove_detached(libxl__egc *egc,
> FILE *f;
> uint32_t domainid = prs->domid;
> bool isstubdom;
> +bool is_gsi = true;
>
> /* Convenience aliases */
> libxl_device_pci *const pci = >pci;
> @@ -2243,6 +2266,7 @@ skip_bar:
> /* To compitable with old version of kernel, still need to use irq */
> sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> pci->bus, pci->dev, pci->func);
> +is_gsi = false;
> }
> f = fopen(sysfs_path, "r");
> if (f == NULL) {
> @@ -2261,9 +2285,17 @@ skip_bar:
> */
> LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
> }
> -rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
> -if (rc < 0) {
> -LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
> +if (is_gsi) {
> +rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);
Could you store the return code of this system call into `r` instead?
`rc` is supposed to be use exclusively for libxl error code, so the
current code is wrong, but we can partially fixed that in your patch.
> +if (rc < 0 && rc != -EOPNOTSUPP) {
Shouldn't you check EOPNOTSUPP in `errno` instead?
> +LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d", irq);
> +}
> +}
> +if (!is_gsi || rc == -EOPNOTSUPP) {
> +rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
Same here, use `r` instead of `rc`.
> +if (rc < 0) {
> +LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
> +}
> }
> }
>
Thanks,
--
Anthony PERARD
Re: [PATCH v6 2/4] tools: Move MB/GB() to common-macros.h
On Wed, Mar 27, 2024 at 05:51:00PM -0400, Jason Andryuk wrote:
> Consolidate to a single set of common macros for tools.
>
> MB() will gain another use in libelf, so this movement makes it
> available.
>
> Requested-by: Jan Beulich
> Signed-off-by: Jason Andryuk
> Reviewed-by: Jan Beulich
> ---
So, this patch fixes potential use issues with the macros in hvmloader
and init-xenstore-domain. While it's not perfect, it still better.
I'll try "MB(memory + 0)" with the different macros:
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 87be213dec..14078bde1e 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -38,9 +38,6 @@ void __bug(const char *file, int line)
> __attribute__((noreturn));
> #define BUG() __bug(__FILE__, __LINE__)
> #define BUG_ON(p) do { if (p) BUG(); } while (0)
>
> -#define MB(mb) (mb##ULL << 20)
> -#define GB(gb) (gb##ULL << 30)
With this change we have this change for MB(memory + 0) when applied:
- (memory + 0ULL << 20)
+ ((memory + 0ULL) << 20)
> static inline int test_bit(unsigned int b, const void *p)
> {
> return !!(((const uint8_t *)p)[b>>3] & (1u<<(b&7)));
> diff --git a/tools/helpers/init-xenstore-domain.c
> b/tools/helpers/init-xenstore-domain.c
> index 5405842dfe..f38ba8d6b5 100644
> --- a/tools/helpers/init-xenstore-domain.c
> +++ b/tools/helpers/init-xenstore-domain.c
> @@ -20,7 +20,6 @@
> #include "init-dom-json.h"
>
> #define LAPIC_BASE_ADDRESS 0xfee0UL
> -#define GB(x) ((uint64_t)x << 30)
With this change we have this change for GB(memory + 0) when applied:
- ((uint64_t)memory + 0 << 30)
+ ((memory + 0ULL) << 30)
So overall, patch makes things better and less duplication:
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH v6 1/4] tools/init-xenstore-domain: Replace variable MB() usage
On Wed, Mar 27, 2024 at 05:50:59PM -0400, Jason Andryuk wrote:
> The local MB() & GB() macros will be replaced with a common
> implementation, but those only work with constant values. Introduce a
By the way, this is not true, the macro introduce in the following patch
("tools: Move MB/GB() to common-macros.h") works (compiler doesn't
complain) if you do something like MB(maxmem+0) ;-).
> static inline mb_to_bytes() in place of the MB() macro to convert the
> variable values.
>
> diff --git a/tools/helpers/init-xenstore-domain.c
> b/tools/helpers/init-xenstore-domain.c
> index 1683438c5c..5405842dfe 100644
> --- a/tools/helpers/init-xenstore-domain.c
> +++ b/tools/helpers/init-xenstore-domain.c
> @@ -20,7 +20,6 @@
> #include "init-dom-json.h"
>
> #define LAPIC_BASE_ADDRESS 0xfee0UL
> -#define MB(x) ((uint64_t)x << 20)
> #define GB(x) ((uint64_t)x << 30)
>
> static uint32_t domid = ~0;
> @@ -36,6 +35,11 @@ static xc_evtchn_port_or_error_t console_evtchn;
> static xentoollog_level minmsglevel = XTL_PROGRESS;
> static void *logger;
>
> +static inline uint64_t mb_to_bytes(int mem)
> +{
> + return (uint64_t)mem << 20;
> +}
> +
> static struct option options[] = {
> { "kernel", 1, NULL, 'k' },
> { "memory", 1, NULL, 'm' },
> @@ -76,8 +80,8 @@ static int build(xc_interface *xch)
> int rv, xs_fd;
> struct xc_dom_image *dom = NULL;
> int limit_kb = (maxmem ? : memory) * 1024 + X86_HVM_NR_SPECIAL_PAGES * 4;
> -uint64_t mem_size = MB(memory);
> -uint64_t max_size = MB(maxmem ? : memory);
> +uint64_t mem_size = mb_to_bytes(memory);
> +uint64_t max_size = mb_to_bytes(maxmem ? : memory);
> struct e820entry e820[3];
> struct xen_domctl_createdomain config = {
> .ssidref = SECINITSID_DOMU,
Sorry, just notice they were more versions of the series, so repeating
here:
Looks like this change actually fix a bug. When `maxmem` is set, we
would have "max_size = maxmem", otherwise, it would be
"max_size = memory << 20"
The macro should have been written as
#define MB(x) ((uint64_t)(x) << 20)
This patch could be backported to 4.17 and later, with:
Fixes: 134d53f57707 ("tools/init-xenstore-domain: fix memory map for PVH
stubdom")
Anyway, this patch on it's own looks fine:
Reviewed-by: Anthony PERARD
--
Anthony PERARD
Re: [PATCH v4 2/5] tools/init-xenstore-domain: Replace variable MB() usage
On Mon, Mar 25, 2024 at 04:45:12PM -0400, Jason Andryuk wrote:
> The local MB() & GB() macros will be replaced with a common
> implementation, but those only work with constant values. Introduce a
> static inline mb_to_bytes() in place of the MB() macro to convert the
> variable values.
>
> Signed-off-by: Jason Andryuk
> ---
> v4:
> New
> ---
> tools/helpers/init-xenstore-domain.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tools/helpers/init-xenstore-domain.c
> b/tools/helpers/init-xenstore-domain.c
> index 1683438c5c..5405842dfe 100644
> --- a/tools/helpers/init-xenstore-domain.c
> +++ b/tools/helpers/init-xenstore-domain.c
> @@ -20,7 +20,6 @@
> #include "init-dom-json.h"
>
> #define LAPIC_BASE_ADDRESS 0xfee0UL
> -#define MB(x) ((uint64_t)x << 20)
> #define GB(x) ((uint64_t)x << 30)
>
> static uint32_t domid = ~0;
> @@ -36,6 +35,11 @@ static xc_evtchn_port_or_error_t console_evtchn;
> static xentoollog_level minmsglevel = XTL_PROGRESS;
> static void *logger;
>
> +static inline uint64_t mb_to_bytes(int mem)
> +{
> + return (uint64_t)mem << 20;
> +}
> +
> static struct option options[] = {
> { "kernel", 1, NULL, 'k' },
> { "memory", 1, NULL, 'm' },
> @@ -76,8 +80,8 @@ static int build(xc_interface *xch)
> int rv, xs_fd;
> struct xc_dom_image *dom = NULL;
> int limit_kb = (maxmem ? : memory) * 1024 + X86_HVM_NR_SPECIAL_PAGES * 4;
> -uint64_t mem_size = MB(memory);
> -uint64_t max_size = MB(maxmem ? : memory);
> +uint64_t mem_size = mb_to_bytes(memory);
> +uint64_t max_size = mb_to_bytes(maxmem ? : memory);
Looks like this change actually fix a bug. When `maxmem` is set, we
would have "max_size = maxmem", otherwise, it would be
"max_size = memory << 20"
The macro should have been written as
#define MB(x) ((uint64_t)(x) << 20)
This patch could be backported to 4.17 and later, with:
Fixes: 134d53f57707 ("tools/init-xenstore-domain: fix memory map for PVH
stubdom")
Anyway, this patch on it's own looks fine:
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
[OSSTEST PATCH 37/36] production-config: Set Bookworm's debian-installer version
Signed-off-by: Anthony PERARD --- I've got this one more patch to add to the series, which I forgot to write. I'll put it before "Switch to Debian Bookworm as default suite" when I'll push the rest of the series. production-config | 1 + 1 file changed, 1 insertion(+) diff --git a/production-config b/production-config index 6345c40c..0eb8f0f3 100644 --- a/production-config +++ b/production-config @@ -92,6 +92,7 @@ TftpDiVersion_wheezy 2016-06-08 TftpDiVersion_jessie 2018-06-26 TftpDiVersion_stretch 2020-09-24 TftpDiVersion_buster 2023-06-20 +TftpDiVersion_bookworm 2024-03-26 DebianSnapshotBackports_jessie http://snapshot.debian.org/archive/debian/20190206T211314Z/ -- Anthony PERARD
[OSSTEST PATCH] ap-common: Fix libvirt's git repo URL
The current URL doesn't work anymore and just timeout, switch to the
new main repo URL.
Signed-off-by: Anthony PERARD
---
Notes:
I'll push that later today.
Last libvirt test by osstest was on 20 February.
ap-common | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ap-common b/ap-common
index 5384891b..eabb85c6 100644
--- a/ap-common
+++ b/ap-common
@@ -41,7 +41,7 @@
: ${PUSH_TREE_FREEBSD:=$XENBITS:/home/xen/git/freebsd.git}
: ${BASE_TREE_FREEBSD:=git://xenbits.xen.org/freebsd.git}
-: ${TREE_LIBVIRT:=git://libvirt.org/libvirt.git}
+: ${TREE_LIBVIRT:=https://gitlab.com/libvirt/libvirt.git}
: ${PUSH_TREE_LIBVIRT:=$XENBITS:/home/xen/git/libvirt.git}
: ${BASE_TREE_LIBVIRT:=git://xenbits.xen.org/libvirt.git}
--
Anthony PERARD
Re: [OSSTEST PATCH 00/36] Switch to Debian Bookworm
On Mon, Mar 18, 2024 at 04:55:09PM +, Anthony PERARD wrote: > I intend to push this series in two waves. > > First, push up to commit "Temporally switch "qemu-mainline" branch to > Bookworm". This is to test that osstest still works fine if we need to use > "buster" for a branch. Also upstream QEMU doesn't build on buster anymore, so > I've included a commit to use bookworm for it. > > Second, push the remaning two patches, at least a week later, which will > switch > the default debian suite. > > Anthony PERARD (36): > production-config: Add bookworm debian install media filename > ts-xen-build-prep: Only force git protocol v2 on buster > mgi-common: Fix fetch_debian_package error message > mg-debian-installer-update: Download non-free firmware from new repo. > ts-host-install: fix ntp.conf path on bookworm > ts-host-install: fix ntp server setting > ts-host-install: Restart ntp service > preseed_create: Use new "d-i grub-installer/update-nvram" for UEFI > installation > preseed_create: osstest-erase-other-disks: workaround creating > /dev/sdXD files > preseed_create: Workaround fail grub-install on arndale > ts-host-install,preseed_create: Do lvm vgextend at install time > di_installcmdline_core: Add link_wait_timeout to install cmdline > Disable persistent net generator on Bookworm > preseed_base, ts-host-install: Change NIC NamePolicy to "mac" > ts-xen-build-prep: Change package selection for Bookworm > bl_getmenu_open: Read grub.cfg as root > target_editfile: Use the same user to retrieve and send > ts-xen-install: remove "libc6-xen" package installation > overlay-bookworm: Import grub's 20_linux_xen from Debian Bookworm > overlay-bookworm: 20_linux_xen: Fix XSM entries generation > ts-xtf-install: Install python symlink > setupboot_grub2: Parse arm64 uefi grub verbes > bookworm: Extend ARM clock workaround > ts-nested-setup, setup l1 lvm volume groupe in guest > ts-leak-check: add new name for udevd workers > ts-debian-hvm-install: Allow udev failure in install media > ts-debian-fixup: Fix nic names for bookworm > ts-debian-install: keep avoiding to use pygrub > ts-debian-hvm-install: Increase min guest ram size > bookworm: Extend guest bootloader workaround > ts-debian-*-install: Replace dots in hostnames by dashs > ts-xen-install: Fix bridge setup, ask to copy MAC addr > make-flight: Keep using buster for L2 guest in nested tests I've pushed up to here. I'll push the last two patches later. > Temporally switch "qemu-mainline" branch to Bookworm And I'll drop this one. > Switch to Debian Bookworm as default suite > make-hosts-flight: default to bookworm Cheers, -- Anthony PERARD
Re: [RFC XEN PATCH v6 4/5] libxl: Use gsi instead of irq for mapping pirq
On Thu, Mar 28, 2024 at 02:34:01PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 96cb4da0794e..2cec83e0b734 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1478,8 +1478,14 @@ static void pci_add_dm_done(libxl__egc *egc,
> fclose(f);
> if (!pci_supp_legacy_irq())
> goto out_no_irq;
> -sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> +sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/gsi", pci->domain,
> pci->bus, pci->dev, pci->func);
> +r = access(sysfs_path, F_OK);
> +if (r && errno == ENOENT) {
> +/* To compitable with old version of kernel, still need to use irq */
There's a typo, this would be "To be compatible ...". Also maybe
something like "Fallback to "/irq" for compatibility with old version of
the kernel." might sound better.
> +sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> + pci->bus, pci->dev, pci->func);
> +}
> f = fopen(sysfs_path, "r");
> if (f == NULL) {
> LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
> @@ -2229,9 +2235,15 @@ skip_bar:
> if (!pci_supp_legacy_irq())
> goto skip_legacy_irq;
>
> -sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> +sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/gsi", pci->domain,
> pci->bus, pci->dev, pci->func);
>
> +rc = access(sysfs_path, F_OK);
Please, don't use the variable `rc` here, this one is reserved for libxl
error/return code in libxl. Introduce `int r` instead.
> +if (rc && errno == ENOENT) {
> +/* To compitable with old version of kernel, still need to use irq */
> +sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> + pci->bus, pci->dev, pci->func);
> +}
> f = fopen(sysfs_path, "r");
> if (f == NULL) {
> LOGED(ERROR, domid, "Couldn't open %s", sysfs_path);
With those two things fixed: Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 19/19] hw/xen: Have most of Xen files become target-agnostic
On Tue, Nov 14, 2023 at 03:38:15PM +0100, Philippe Mathieu-Daudé wrote:
> Previous commits re-organized the target-specific bits
> from Xen files. We can now build the common files once
> instead of per-target.
>
> Only 4 files call libxen API (thus its CPPFLAGS):
> - xen-hvm-common.c,
> - xen_pt.c, xen_pt_graphics.c, xen_pt_msi.c
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> Reworked since v1 so dropping David's R-b tag.
> ---
> accel/xen/meson.build | 2 +-
> hw/block/dataplane/meson.build | 2 +-
> hw/xen/meson.build | 21 ++---
> 3 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/accel/xen/meson.build b/accel/xen/meson.build
> index 002bdb03c6..455ad5d6be 100644
> --- a/accel/xen/meson.build
> +++ b/accel/xen/meson.build
> @@ -1 +1 @@
> -specific_ss.add(when: 'CONFIG_XEN', if_true: files('xen-all.c'))
> +system_ss.add(when: 'CONFIG_XEN', if_true: files('xen-all.c'))
> diff --git a/hw/block/dataplane/meson.build b/hw/block/dataplane/meson.build
> index 025b3b061b..4d8bcb0bb9 100644
> --- a/hw/block/dataplane/meson.build
> +++ b/hw/block/dataplane/meson.build
> @@ -1,2 +1,2 @@
> system_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
> -specific_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-block.c'))
> +system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-block.c'))
> diff --git a/hw/xen/meson.build b/hw/xen/meson.build
> index d887fa9ba4..403cab49cf 100644
> --- a/hw/xen/meson.build
> +++ b/hw/xen/meson.build
> @@ -7,26 +7,25 @@ system_ss.add(when: ['CONFIG_XEN_BUS'], if_true: files(
>'xen_pvdev.c',
> ))
>
> -system_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
> +system_ss.add(when: ['CONFIG_XEN'], if_true: files(
>'xen-operations.c',
> -))
> -
> -xen_specific_ss = ss.source_set()
> -xen_specific_ss.add(files(
>'xen-mapcache.c',
> +))
> +system_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
>'xen-hvm-common.c',
> ))
> +
> if have_xen_pci_passthrough
> - xen_specific_ss.add(files(
> + system_ss.add(when: ['CONFIG_XEN'], if_true: files(
> 'xen-host-pci-device.c',
> -'xen_pt.c',
> 'xen_pt_config_init.c',
> -'xen_pt_graphics.c',
> 'xen_pt_load_rom.c',
> + ))
> + system_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
> +'xen_pt.c',
> +'xen_pt_graphics.c',
How is it useful to separate those source files? In the commit
description, there's a talk about "CPPFLAGS", but having `when: [xen]`
doesn't change the flags used to build those objects, so the talk about
"CPPFLAGS" is confusing.
Second, if for some reason the dependency `xen` is false, but
`CONFIG_XEN` is true, then we wouldn't be able to build QEMU. Try
linking a binary with "xen_pt_config_init.o" but without "xen_pt.o",
that's not going to work. So even if that first source file doesn't
directly depend on the Xen libraries, it depends on "xen_pt.o" which
depends on the Xen libraries. So ultimately, I think all those source
files should have the same condition: ['CONFIG_XEN', xen].
I've only checked the xen_pt* source files, I don't know if the same
applies to "xen-operations.c" or "xen-mapcache.c".
Beside this, QEMU built with Xen support still seems to works fine, so
adding the objects to `system_ss` instead of `specific_ss` seems
alright.
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 16/19] hw/xen/xen_pt: Add missing license
On Tue, Nov 14, 2023 at 03:38:12PM +0100, Philippe Mathieu-Daudé wrote:
> Commit eaab4d60d3 ("Introduce Xen PCI Passthrough, qdevice")
> introduced both xen_pt.[ch], but only added the license to
> xen_pt.c. Use the same license for xen_pt.h.
>
> Suggested-by: David Woodhouse
> Signed-off-by: Philippe Mathieu-Daudé
Fine by me. Looks like there was a license header before:
https://xenbits.xen.org/gitweb/?p=qemu-xen-unstable.git;a=blob;f=hw/pass-through.h;h=0b5822414e24d199a064abccc4d378dcaf569bd6;hb=HEAD
I don't know why I didn't copied it over here.
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 15/19] hw/xen: Reduce inclusion of 'cpu.h' to target-specific sources
On Tue, Nov 14, 2023 at 03:38:11PM +0100, Philippe Mathieu-Daudé wrote: > We rarely need to include "cpu.h" in headers. Including it > 'taint' headers to be target-specific. Here only the i386/arm > implementations requires "cpu.h", so include it there and > remove from the "hw/xen/xen-hvm-common.h" *common* header. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Richard Henderson > Reviewed-by: David Woodhouse Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [RFC PATCH-for-9.0 v2 13/19] hw/xen: Remove use of 'target_ulong' in handle_ioreq()
On Tue, Nov 14, 2023 at 03:38:09PM +0100, Philippe Mathieu-Daudé wrote:
> Per commit f17068c1c7 ("xen-hvm: reorganize xen-hvm and move common
> function to xen-hvm-common"), handle_ioreq() is expected to be
> target-agnostic. However it uses 'target_ulong', which is a target
> specific definition.
>
> Per xen/include/public/hvm/ioreq.h header:
>
> struct ioreq {
> uint64_t addr; /* physical address */
> uint64_t data; /* data (or paddr of data) */
> uint32_t count; /* for rep prefixes */
> uint32_t size; /* size in bytes */
> uint32_t vp_eport; /* evtchn for notifications to/from device model
> */
> uint16_t _pad0;
> uint8_t state:4;
> uint8_t data_is_ptr:1; /* if 1, data above is the guest paddr
> * of the real data to use. */
> uint8_t dir:1; /* 1=read, 0=write */
> uint8_t df:1;
> uint8_t _pad1:1;
> uint8_t type; /* I/O type */
> };
> typedef struct ioreq ioreq_t;
>
> If 'data' is not a pointer, it is a u64.
>
> - In PIO / VMWARE_PORT modes, only 32-bit are used.
>
> - In MMIO COPY mode, memory is accessed by chunks of 64-bit
Looks like it could also be 8, 16, or 32 as well, depending on
req->size.
> - In PCI_CONFIG mode, access is u8 or u16 or u32.
>
> - None of TIMEOFFSET / INVALIDATE use 'req'.
>
> - Fallback is only used in x86 for VMWARE_PORT.
>
> Masking the upper bits of 'data' to keep 'req->size' low bits
> is irrelevant of the target word size. Remove the word size
> check and always extract the relevant bits.
When building QEMU for Xen, we tend to build the target "i386-softmmu",
which looks like to have target_ulong == uint32_t. So the `data`
clamping would only apply to size 8 and 16. The clamping with
target_ulong was introduce long time ago, here:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=b4a663b87df3954557434a2d31bff7f6b2706ec1
and they were more IOREQ types.
So my guess is it isn't relevant anymore, but extending the clamping to
32-bits request should be fine, when using qemu-system-i386 that is, as
it is already be done if one use qemu-system-x86_64.
So I think the patch is fine, and the tests I've ran so far worked fine.
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 12/19] hw/xen: Merge 'hw/xen/arch_hvm.h' in 'hw/xen/xen-hvm-common.h'
On Tue, Nov 14, 2023 at 03:38:08PM +0100, Philippe Mathieu-Daudé wrote: > We don't need a target-specific header for common target-specific > prototypes. Declare xen_arch_handle_ioreq() and xen_arch_set_memory() > in "hw/xen/xen-hvm-common.h". > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: David Woodhouse > Reviewed-by: Richard Henderson Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 11/19] hw/xen/xen_arch_hvm: Rename prototypes using 'xen_arch_' prefix
On Tue, Nov 14, 2023 at 03:38:07PM +0100, Philippe Mathieu-Daudé wrote: > Use a common 'xen_arch_' prefix for architecture-specific functions. > Rename xen_arch_set_memory() and xen_arch_handle_ioreq(). > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: David Woodhouse > Reviewed-by: Richard Henderson Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [RFC PATCH-for-9.0 v2 09/19] hw/block/xen_blkif: Align structs with QEMU_ALIGNED() instead of #pragma
On Tue, Nov 14, 2023 at 03:38:05PM +0100, Philippe Mathieu-Daudé wrote:
> Except imported source files, QEMU code base uses
> the QEMU_ALIGNED() macro to align its structures.
This patch only convert the alignment, but discard pack. We need both or
the struct is changed.
> ---
> hw/block/xen_blkif.h | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h
> index 99733529c1..c1d154d502 100644
> --- a/hw/block/xen_blkif.h
> +++ b/hw/block/xen_blkif.h
> @@ -18,7 +18,6 @@ struct blkif_common_response {
> };
>
> /* i386 protocol version */
> -#pragma pack(push, 4)
> struct blkif_x86_32_request {
> uint8_toperation;/* BLKIF_OP_???
> */
> uint8_tnr_segments; /* number of segments
> */
> @@ -26,7 +25,7 @@ struct blkif_x86_32_request {
> uint64_t id; /* private guest value, echoed in resp
> */
> blkif_sector_t sector_number;/* start sector idx on disk (r/w only)
> */
> struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> -};
> +} QEMU_ALIGNED(4);
E.g. for this one, I've compare the output of
`pahole --class_name=blkif_x86_32_request build/qemu-system-i386`:
--- before
+++ after
@@ -1,11 +1,15 @@
struct blkif_x86_32_request {
uint8_toperation;/* 0 1 */
uint8_tnr_segments; /* 1 1 */
uint16_t handle; /* 2 2 */
- uint64_t id; /* 4 8 */
- uint64_t sector_number;/*12 8 */
- struct blkif_request_segment seg[11];/*2088 */
- /* size: 108, cachelines: 2, members: 6 */
- /* last cacheline: 44 bytes */
-} __attribute__((__packed__));
+ /* XXX 4 bytes hole, try to pack */
+
+ uint64_t id; /* 8 8 */
+ uint64_t sector_number;/*16 8 */
+ struct blkif_request_segment seg[11];/*2488 */
+
+ /* size: 112, cachelines: 2, members: 6 */
+ /* sum members: 108, holes: 1, sum holes: 4 */
+ /* last cacheline: 48 bytes */
+} __attribute__((__aligned__(8)));
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 08/19] hw/xen: Remove unused Xen stubs
On Tue, Nov 14, 2023 at 03:38:04PM +0100, Philippe Mathieu-Daudé wrote:
> All these stubs are protected by a 'if (xen_enabled())' check.
Are you sure? There's still nothing that prevent a compiler from wanting
those, I don't think.
Sure, often compilers will remove dead code in `if(0){...}`, but there's
no guaranty, is there?
Cheers,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 05/19] hw/display: Restrict xen_register_framebuffer() call to Xen
On Tue, Nov 14, 2023 at 03:38:01PM +0100, Philippe Mathieu-Daudé wrote: > Only call xen_register_framebuffer() when Xen is enabled. > > Signed-off-by: Philippe Mathieu-Daudé I don't think this patch is very useful but it's fine, so: Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2 2/2] xen: fix stubdom PCI addr
First things first, could you fix the coding style? Run something like `./scripts/checkpatch.pl @^..` or `./scripts/checkpatch.pl master..`. Patchew might have run that for you if the patch series had a cover letter. On Tue, Mar 05, 2024 at 08:12:30PM +0100, Marek Marczykowski-Górecki wrote: > diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c > index 8c6e9a1716..8ea2a5a4af 100644 > --- a/hw/xen/xen-host-pci-device.c > +++ b/hw/xen/xen-host-pci-device.c > @@ -9,6 +9,8 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "qemu/cutils.h" > +#include "hw/xen/xen-legacy-backend.h" I'd like to avoid this header here, that would be complicated at the moment, as the global variable `xenstore` would be missing. So for now, that's fine. I guess that could be rework if something like Philippe talked about at https://lore.kernel.org/qemu-devel/429a5a27-21b9-45bd-a1a6-a1c2ccc48...@linaro.org/ materialise. Beside the coding style, the patch looks file. Thanks, -- Anthony PERARD
Re: [PATCH v2 1/2] hw/xen: detect when running inside stubdomain
On Tue, Mar 05, 2024 at 08:12:29PM +0100, Marek Marczykowski-Górecki wrote:
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index 124dd5f3d6..6bd4e6eb2f 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -603,6 +603,20 @@ static void xen_set_dynamic_sysbus(void)
> machine_class_allow_dynamic_sysbus_dev(mc, TYPE_XENSYSDEV);
> }
>
> +static bool xen_check_stubdomain(void)
> +{
> +char *dm_path = g_strdup_printf("/local/domain/%d/image", xen_domid);
> +int32_t dm_domid;
> +bool is_stubdom = false;
> +
> +if (!xenstore_read_int(dm_path, "device-model-domid", _domid)) {
> +is_stubdom = dm_domid != 0;
> +}
> +
> +g_free(dm_path);
> +return is_stubdom;
> +}
> +
> void xen_be_init(void)
> {
> xenstore = qemu_xen_xs_open();
> @@ -616,6 +630,8 @@ void xen_be_init(void)
> exit(1);
> }
>
> +xen_is_stubdomain = xen_check_stubdomain();
This isn't really a backend specific information, and xen_be_init() is
all about old backend implementation support. (qdisk which have been the
first to be rewritten doesn't need xen_be_init(), or shouldn't). Could
we move the initialisation elsewhere?
Is this relevant PV guests? If not, we could move the initialisation to
xen_hvm_init_pc().
Also, avoid having xen_check_stubdomain() depending on
"xen-legacy-backend", if possible.
(In xen_hvm_init_pc(), a call to xen_register_ioreq() opens another
xenstore, as `state->xenstore`.)
(There's already been effort to build QEMU without legacy backends, that
stubdom check would break in this scenario.)
Thanks,
--
Anthony PERARD
Re: [OSSTEST PATCH 33/36] make-flight: Keep using buster for L2 guest in nested tests
On Wed, Mar 20, 2024 at 06:24:18PM +0100, Roger Pau Monné wrote: > On Mon, Mar 18, 2024 at 04:55:42PM +0000, Anthony PERARD wrote: > > When starting the installation of the L2 guest, L0 kills L1. Switching > > the L2 guest back to Debian Buster works fine, so do that to prevent > > regression in the test. > > > > Part of the logs from the host L0: > > > > > domain_crash called from arch/x86/hvm/vmx/vvmx.c:2770 > > > Domain 3 (vcpu#0) crashed on cpu#4: > > > d3v0 vmentry failure (reason 0x8021): Invalid guest state (2) > > Hm, I guess we need this as otherwise the changes themselves won't > past the self-push gate, but this is just masking a real issue. I > would be tempted to (after the switch to bookworm has gone in) revert > this and force-push it into osstest, so that the failure is visible. That means that we might have to force-push on every branches. Or fix Xen. Thanks, -- Anthony PERARD
Re: [OSSTEST PATCH 34/36] Temporally switch "qemu-mainline" branch to Bookworm
On Wed, Mar 20, 2024 at 06:25:35PM +0100, Roger Pau Monné wrote: > On Mon, Mar 18, 2024 at 04:55:43PM +0000, Anthony PERARD wrote: > > QEMU doesn't build on buster anymore. > > > > This should be remove once bookworm is the default suite. > > Is this change required anymore, patch 35 makes bookworm the default, > hence it seems pointless to switch QEMU just one patch ahead. You didn't read the cover letter? But I guess I can skip this patch, it's not that important. Cheers, -- Anthony PERARD
Re: [OSSTEST PATCH 10/36] preseed_create: Workaround fail grub-install on arndale
On Wed, Mar 20, 2024 at 05:38:17PM +0100, Roger Pau Monné wrote: > On Mon, Mar 18, 2024 at 04:55:19PM +0000, Anthony PERARD wrote: > > grub-installer on arndale-* machine fails with Debian Bookworm. It > > tries to install "grub-pc" which doesn't exist. Skip installation. > > > > Somehow, cubietruck-* installation works fine. > > I'm kind of puzzled by this, as cubietruck and arndales are both armhf > IIRC? (IOW: they should use the same repo?) > > Does the install of grub-pc succeed on the cubietruck, or is skipped > for some other reason that doesn't require us to intervene? It's skipped. The difference I can see is that, on the cubietruck, there's an extra step. And it's due to this extra logging: debconf: --> METAGET debian-installer/flash-kernel-installer/title Description debconf: <-- 0 Make the system bootable On the cubietruck, there would be these installation step: ..., Select and install software, Make the system bootable, Install the GRUB boot loader, ... But on the arndale, it would be instead: ..., Select and install software, Install the GRUB boot loader, ... Somehow, flash-kernel-installer does something different on both machine. Thanks, -- Anthony PERARD
Backport request for 4.17
Hi,
Would it be possible to backport 18a36b4a9b08 ("tools: ipxe: update for
fixing build with GCC12") to Xen 4.17 ?
This would be to allow building Xen 4.17 on Debian Bookworm, and to allow
osstest to test Xen 4.18 with Debian Bookworm. osstest always tries to
migration from N-1 to N, so it would need to be able to build both 4.17
and 4.18 to actually test 4.18. Otherwise, I can tell osstest to keep
using Debian Buster to test 4.18.
For context:
https://lore.kernel.org/xen-devel/20240318165545.3898-36-anthony.per...@citrix.com/
That commit pulls a newer version of IPXE, I don't think there's be
compatibility issue with Xen, and hopefully nothing breaks.
Thanks,
--
Anthony PERARD
Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map
On Fri, Mar 08, 2024 at 09:54:35AM +0800, Henry Wang wrote:
> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> index fee93459c4..92c612f6da 100644
> --- a/tools/helpers/init-dom0less.c
> +++ b/tools/helpers/init-dom0less.c
> @@ -23,16 +23,30 @@ static int alloc_xs_page(struct xc_interface_core *xch,
> libxl_dominfo *info,
> uint64_t *xenstore_pfn)
> {
> -int rc;
> -const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
> -xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) +
> XENSTORE_PFN_OFFSET;
> +int rc, i;
> +xen_pfn_t base = ((xen_pfn_t)-1);
> +xen_pfn_t p2m = ((xen_pfn_t)-1);
> +uint32_t nr_regions = XEN_MAX_MEM_REGIONS;
> +struct xen_mem_region mem_regions[XEN_MAX_MEM_REGIONS] = {0};
> +
> +rc = xc_get_domain_mem_map(xch, info->domid, mem_regions, _regions);
Shouldn't you check the value of in `rc`?
> +for ( i = 0; i < nr_regions; i++ )
> +{
> +if ( mem_regions[i].type == GUEST_MEM_REGION_MAGIC )
> +{
> +base = mem_regions[i].start >> XC_PAGE_SHIFT;
> +p2m = (mem_regions[i].start >> XC_PAGE_SHIFT) +
> XENSTORE_PFN_OFFSET;
> +}
> +}
Thanks,
--
Anthony PERARD
Re: [XEN PATCH v2 1/1] tools: init scripts: Add missing XENCONSOLED_ARGS variable
On Mon, Mar 25, 2024 at 12:09:40PM +0100, Rafaël Kooi wrote: > Makes sense. To be honest I don't even know what arguments one would pass > to xenconsoled other than setting the log level to begin with. I would > assume this to be a niche enough use case where very few users would be > affected if we do decided to remove the variable. Maybe we could opt to > advertise it as something that we will be removing in a later release? I think it's fine to leave the variable where it is. On a systemd service file, it's kind of nice to be able to add new arguments without overwriting the whole command, as it means that we get the command line on an update if it changed. Cheers, -- Anthony PERARD
Re: [PATCH v7 06/14] tools: add support for cache coloring configuration
On Fri, Mar 15, 2024 at 11:58:54AM +0100, Carlo Nonato wrote:
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index 5546335973..79f206f616 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -726,6 +726,15 @@ int libxl__domain_make(libxl__gc *gc,
> libxl_domain_config *d_config,
> /* A new domain now exists */
> *domid = local_domid;
>
> +ret = xc_domain_set_llc_colors(ctx->xch, local_domid,
> + b_info->llc_colors,
> + b_info->num_llc_colors);
> +if (ret < 0 && errno != EOPNOTSUPP) {
Wait, this additional check on EOPNOTSUPP, does that mean we ignore
"llc_colors" configure by the admin of the VM if the system doesn't
support it? Shouldn't we also report an error in this case? At least if
`num_llc_colors > 0`.
Thanks,
--
Anthony PERARD
Re: [XEN PATCH v2 1/1] tools: init scripts: Add missing XENCONSOLED_ARGS variable
On Wed, Mar 20, 2024 at 08:48:33AM +0100, Rafaël Kooi wrote: > The systemd xenconsoled.service uses the XENCONSOLED_ARGS variable, but > it was missing from the xencommons file. Actually, I'm tempted to go the other way and remove XENCONSOLED_ARGS from the systemd's service file. In the other service files (openrc, sysvinit) for Linux/FreeBSD/NetBSD, XENCONSOLED_ARGS is simply used to construct the command line for `xenconsoled`. For the Linux one, if you set XENCONSOLED_TRACE, XENCONSOLED_ARGS from xencommons won't be used. On the other end, with systemd, it is very easy to modified a service file, to add an argument to the command line. So removing XENCONSOLED_ARGS would make more sense. But some user might have notice it exist and start using it, so it's probably too late to remove it. So overall, I don't think it's a good idea to advertise XENCONSOLED_ARGS, first because it's kind of useless for systemd, and second because it's broken for "tools/hotplug/Linux/init.d/xencommons.in" Side note: looks like on my test machine I've used systemd instead of editing /etc/default/xencommons to change XENCONSOLED_TRACE, so there's really no need for "xencommons" config file on systemd: # /etc/systemd/system/xenconsoled.service.d/trace_all.conf [Service] Environment=XENCONSOLED_TRACE=all That "xencommons" config file is just there to be compatible with both init system. Cheers, -- Anthony PERARD
Re: [OSSTEST PATCH 02/36] ts-xen-build-prep: Only force git protocol v2 on buster
On Wed, Mar 20, 2024 at 04:20:00PM +0100, Roger Pau Monné wrote:
> On Mon, Mar 18, 2024 at 04:55:11PM +0000, Anthony PERARD wrote:
> > Newer version of Debian and thus git would use this automatically, no
> > need to force it.
> >
> > Signed-off-by: Anthony PERARD
> > ---
> > Osstest/TestSupport.pm | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> > index f0e087aa..0dded9b2 100644
> > --- a/Osstest/TestSupport.pm
> > +++ b/Osstest/TestSupport.pm
> > @@ -3257,7 +3257,7 @@ END
> >
> > # Adding mandadory use of Git protocol v2
> > # necessary on buster when building QEMU v8.1
> > -$gitcfg .= < > +$gitcfg .= <{Suite} =~ m/buster/;
> > [protocol]
> > version = 2
>
> Do we need to limit ourselves to version 2 for the git cache stuff, or
> that doesn't matter?
It's not a limitation, this settings forces `git` to use an experimental
feature. In buster, git still default to version 1, which was an issue
for a single git repo, one of QEMU's meson wrap dependency.
In bookworm, git default to version 2, so I don't think there's need to
deviate from default beyond buster.
Once git-cache-proxy knows how to handle this version=2, and pass it on
to `git-upload-pack`, it will know how to handle v3 and beyond.
> I'm wondering whether it case version 3 of the protocol appears we
> would be in trouble by not having version = 2 in the config file.
No, git should be able to negotiate with itself. Even today, if a git
server only have protocol v1, it will just ignore that a client ask for
v2.
Thanks,
--
Anthony PERARD
Re: [PATCH v2] docs/man: add shutdown reasons to xl (list) man page
On Fri, Mar 08, 2024 at 04:19:20PM +0100, zithro / Cyril Rébert wrote: > Questions (unblocking): > - why a double space between all sentences ? It's an English thing maybe? Wikipedia has an article about it: https://en.wikipedia.org/wiki/Sentence_spacing But actually, single space seems like it used to be a french thing, even called "French spacing": https://en.wikipedia.org/wiki/History_of_sentence_spacing#French_and_English_spacing I guess it doesn't really matter. I don't really pay attention to it anyway. > - how to get a "simple LF" ? Ie. I want to use , not >(a simple has no effect, a double renders a ) That doesn't look to be possible, unless you actually managed to write the paragraph for all targeted formats, with things like =begin html..., but that would probably make things more complicated that necessary. > --- > diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in > index bed8393473..d37227ba58 100644 > --- a/docs/man/xl.1.pod.in > +++ b/docs/man/xl.1.pod.in > @@ -370,12 +370,50 @@ scheduling by the Xen hypervisor. > The guest OS has shut down (SCHEDOP_shutdown has been called) but the > domain is not dying yet. > > -=item B > +There are six shutdown reasons, which will be displayed after the "s" : > B<-rscwS>. > + > +Note that some states will only be displayed if "on_poweroff=preserve" is > set in > +the config file, see L). > + > +=over 4 > + > +=over 4 > + > +=item B > + > +The domain exited normally, it will get destroyed. > + > +=item B > + > +The domain will reboot. Well, that's not really true. I think to end up in this state, what happen is that the domain shutdown and ask for a reboot. It's normally the toolstack job to cleanup the domain then create a new domain for the guest. I guess you might see this state if you have "on_reboot=preserve" or you run `xl list` at the right time. Or the toolstack crashed and fail to kill the domain before a reboot. So maybe a better to have something like "The domain exited normally and ask for a reboot." Same things for the other entries (poweroff and watchdog). The status is just the current status of what is supposed to happen. But a few things might mean that the domain stay in that state unless the admin does something. This could be simply because there's "on_*=preserve" setting, the toolstack crashed or we just happen to look while the toolstack is cleaning thing up. > +=item B > + > +The domain is suspended to disk or to RAM. If suspended to disk, the domain > will > +get destroyed. I think in that state, the domain is just suspended, maybe to RAM? I don't know if suspend to disk would end up in this state, my guess is the domain will go to "poweroff" instead. But for suspend to "RAM", I think the toolstack is supposed to save the guest memory and guest config somewhere, maybe even on a different host for live migration? This is a state that seems to happen as part of a live migration (or `xl save` I guess). I tried to suspend a guest (runnning `systemd suspend`), but xl never reported `ss`, because there's nothing to do I guess. So this state is probably really about migration. Thanks, -- Anthony PERARD
Re: [PATCH RESEND] tools/xl_parse: remove message for tsc mode string
On Thu, Jul 13, 2023 at 04:53:57PM -0700, Elliott Mitchell wrote: > Normal behavior is to be silent. Generating a message for the preferred > input can be mistaken for an error. As such remove this message to match > other conditions. > > Signed-off-by: Elliott Mitchell Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
[XEN PATCH] docs/parse-support-md: Handle BulletList
Commit d638e304f13a introduced a bullet list, but parse-support-md
choke on it as it doesn't know what to do about it.
Introduce ri_BulletList() so that r_content() will find this new
function and call it instead of calling process_unknown().
Reported-by: Julien Grall
Fixes: d638e304f13a ("SUPPORT.MD: Fix matrix generation after 43c416d0d819 and
77c39a53cf5b")
Signed-off-by: Anthony PERARD
---
docs/parse-support-md | 7 +++
1 file changed, 7 insertions(+)
diff --git a/docs/parse-support-md b/docs/parse-support-md
index a397637639..b04f62da37 100755
--- a/docs/parse-support-md
+++ b/docs/parse-support-md
@@ -218,6 +218,13 @@ sub ri_DefinitionList {
}
}
+sub ri_BulletList {
+# Assume a paragraph introduce this bullet list, which would mean that
+# ri_Para() has already been called, and there's nothing else to do about
+# the caveat.
+return;
+}
+
sub process_unknown {
my ($c, $e) = @_;
$had_unknown = Dumper($e);
--
Anthony PERARD
[OSSTEST PATCH 23/36] bookworm: Extend ARM clock workaround
Still broken on arndale, serial stop working early, then the machine
timeout when working on creating a xen guest with xen-create-guest.
Signed-off-by: Anthony PERARD
---
Osstest/Debian.pm | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 4f07cdef..68f1be60 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -253,8 +253,10 @@ END
my @xenkopt = @kopt;
push @xenkopt, $xenkopt;
# https://bugs.xenproject.org/xen/bug/45
+ # #45 - arm: domain 0 disables clocks which are in fact being used
+ # https://lore.kernel.org/xen-devel/1414672390.2064.31.ca...@citrix.com/
push @xenkopt, "clk_ignore_unused"
- if $ho->{Suite} =~ m/wheezy|jessie|stretch|buster/;
+ if $ho->{Suite} =~ m/wheezy|jessie|stretch|buster|bookworm/;
$xenkopt = join ' ', @xenkopt;
logm("Dom0 Linux options: $xenkopt");
--
Anthony PERARD
[OSSTEST PATCH 24/36] ts-nested-setup, setup l1 lvm volume groupe in guest
LVM in l0 doesn't let us run pvcreate on the host LV, `pvcreate
$outer_lvdev` fails with:
Cannot use /dev/$l0-vg/l1_gueststorage_outer_lv: device is an LV
Signed-off-by: Anthony PERARD
---
ts-nested-setup | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/ts-nested-setup b/ts-nested-setup
index be3d3733..7fc8beef 100755
--- a/ts-nested-setup
+++ b/ts-nested-setup
@@ -80,14 +80,14 @@ sub guest_storage () {
target_cmd_root($l0, "vgremove -f $inner_vg ||:");
my $outer_lvdev = lv_create($l0, $outer_vg, $outer_lv, $size);
-target_cmd_root($l0, <block_attach($l1, "$outer_lvdev,raw,sdb,rw");
# NB this does not update the l1 guest config so if the l1 is shut
# down and recreated in the l0, this will vanish.
+
+target_cmd_root($l1, <
[OSSTEST PATCH 36/36] make-hosts-flight: default to bookworm
Signed-off-by: Anthony PERARD
---
make-hosts-flight | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/make-hosts-flight b/make-hosts-flight
index 63ac7b71..0177f605 100755
--- a/make-hosts-flight
+++ b/make-hosts-flight
@@ -26,7 +26,7 @@ blessing=$4
buildflight=$5
: ${ALL_ARCHES:=amd64 i386 arm64 armhf}
-: ${ALL_SUITES:=buster stretch jessie}
+: ${ALL_SUITES:=bookworm buster stretch jessie}
# ^ most preferred suite first
: ${PERHOST_MAXWAIT:=2} # seconds
--
Anthony PERARD
[OSSTEST PATCH 35/36] Switch to Debian Bookworm as default suite
Xen 4.17 doesn't build with Debian Bookworm. It fails to build
IPXE/etherboot, on "amd64". So we keep using Debian Buster on Xen 4.18
and earlier branches. Xen 4.18 builds 4.17 via job "build-amd64-prev".
Xen 4.17 would needs 18a36b4a9b08 ("tools: ipxe: update for fixing
build with GCC12") which is only in Xen 4.18.
Signed-off-by: Anthony PERARD
---
Osstest.pm | 2 +-
make-flight | 6 +-
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/Osstest.pm b/Osstest.pm
index a559ca4e..9ac2dc5c 100644
--- a/Osstest.pm
+++ b/Osstest.pm
@@ -87,7 +87,7 @@ our %c = qw(
Images images
-DebianSuite buster
+DebianSuite bookworm
DebianMirrorSubpath debian
TestHostKeypairPath id_rsa_osstest
diff --git a/make-flight b/make-flight
index 6e88cb13..0b528629 100755
--- a/make-flight
+++ b/make-flight
@@ -43,17 +43,13 @@ case "$xenbranch" in
xen-4.3-testing) defsuite="wheezy"; defguestsuite="wheezy";;
xen-4.9-testing) defsuite="stretch"; defguestsuite="stretch";;
xen-4.10-testing) defsuite="stretch"; defguestsuite="stretch";;
+ xen-4.1[1-8]-testing) defsuite="buster"; defguestsuite="buster";;
*)
defsuite=`getconfig DebianSuite`
defguestsuite=`getconfig GuestDebianSuite`
;;
esac
-# QEMU doesn't build on buster anymore, use bookworm for it.
-case "$branch" in
-qemu-mainline) defsuite="bookworm" ;;
-esac
-
# Pick default Debian Installer version to correspond to the chosen
# suite.
if [ -z "$defdi_version" ] ; then
--
Anthony PERARD
[OSSTEST PATCH 19/36] overlay-bookworm: Import grub's 20_linux_xen from Debian Bookworm
This is a copy of the file installed, from grub-common package.
We are going to edit it shortly.
Signed-off-by: Anthony PERARD
---
overlay-bookworm/etc/grub.d/20_linux_xen | 379 +++
1 file changed, 379 insertions(+)
create mode 100755 overlay-bookworm/etc/grub.d/20_linux_xen
diff --git a/overlay-bookworm/etc/grub.d/20_linux_xen
b/overlay-bookworm/etc/grub.d/20_linux_xen
new file mode 100755
index ..3a27fc6f
--- /dev/null
+++ b/overlay-bookworm/etc/grub.d/20_linux_xen
@@ -0,0 +1,379 @@
+#! /bin/sh
+set -e
+
+# grub-mkconfig helper script.
+# Copyright (C) 2006,2007,2008,2009,2010 Free Software Foundation, Inc.
+#
+# GRUB is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# GRUB is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GRUB. If not, see <http://www.gnu.org/licenses/>.
+
+prefix="/usr"
+exec_prefix="/usr"
+datarootdir="/usr/share"
+
+. "$pkgdatadir/grub-mkconfig_lib"
+
+export TEXTDOMAIN=grub
+export TEXTDOMAINDIR="${datarootdir}/locale"
+
+CLASS="--class gnu-linux --class gnu --class os --class xen"
+SUPPORTED_INITS="sysvinit:/lib/sysvinit/init systemd:/lib/systemd/systemd
upstart:/sbin/upstart"
+
+if [ "x${GRUB_DISTRIBUTOR}" = "x" ] ; then
+ OS=GNU/Linux
+else
+ OS="${GRUB_DISTRIBUTOR} GNU/Linux"
+ CLASS="--class $(echo ${GRUB_DISTRIBUTOR} | tr 'A-Z' 'a-z' | cut -d' '
-f1|LC_ALL=C sed 's,[^[:alnum:]_],_,g') ${CLASS}"
+fi
+
+# loop-AES arranges things so that /dev/loop/X can be our root device, but
+# the initrds that Linux uses don't like that.
+case ${GRUB_DEVICE} in
+ /dev/loop/*|/dev/loop[0-9])
+GRUB_DEVICE=`losetup ${GRUB_DEVICE} | sed -e "s/^[^(]*(\([^)]\+\)).*/\1/"`
+# We can't cope with devices loop-mounted from files here.
+case ${GRUB_DEVICE} in
+ /dev/*) ;;
+ *) exit 0 ;;
+esac
+ ;;
+esac
+
+# Default to disabling partition uuid support to maintian compatibility with
+# older kernels.
+GRUB_DISABLE_LINUX_PARTUUID=${GRUB_DISABLE_LINUX_PARTUUID-true}
+
+# btrfs may reside on multiple devices. We cannot pass them as value of root=
parameter
+# and mounting btrfs requires user space scanning, so force UUID in this case.
+if ( [ "x${GRUB_DEVICE_UUID}" = "x" ] && [ "x${GRUB_DEVICE_PARTUUID}" = "x" ]
) \
+|| ( [ "x${GRUB_DISABLE_LINUX_UUID}" = "xtrue" ] \
+ && [ "x${GRUB_DISABLE_LINUX_PARTUUID}" = "xtrue" ] ) \
+|| ( ! test -e "/dev/disk/by-uuid/${GRUB_DEVICE_UUID}" \
+ && ! test -e "/dev/disk/by-partuuid/${GRUB_DEVICE_PARTUUID}" ) \
+|| ( test -e "${GRUB_DEVICE}" && uses_abstraction "${GRUB_DEVICE}" lvm );
then
+ LINUX_ROOT_DEVICE=${GRUB_DEVICE}
+elif [ "x${GRUB_DEVICE_UUID}" = "x" ] \
+|| [ "x${GRUB_DISABLE_LINUX_UUID}" = "xtrue" ]; then
+ LINUX_ROOT_DEVICE=PARTUUID=${GRUB_DEVICE_PARTUUID}
+else
+ LINUX_ROOT_DEVICE=UUID=${GRUB_DEVICE_UUID}
+fi
+
+# Allow overriding GRUB_CMDLINE_LINUX and GRUB_CMDLINE_LINUX_DEFAULT.
+if [ "${GRUB_CMDLINE_LINUX_XEN_REPLACE}" ]; then
+ GRUB_CMDLINE_LINUX="${GRUB_CMDLINE_LINUX_XEN_REPLACE}"
+fi
+if [ "${GRUB_CMDLINE_LINUX_XEN_REPLACE_DEFAULT}" ]; then
+ GRUB_CMDLINE_LINUX_DEFAULT="${GRUB_CMDLINE_LINUX_XEN_REPLACE_DEFAULT}"
+fi
+
+case x"$GRUB_FS" in
+xbtrfs)
+ rootsubvol="`make_system_path_relative_to_its_root /`"
+ rootsubvol="${rootsubvol#/}"
+ if [ "x${rootsubvol}" != x ]; then
+ GRUB_CMDLINE_LINUX="rootflags=subvol=${rootsubvol}
${GRUB_CMDLINE_LINUX}"
+ fi;;
+xzfs)
+ rpool=`${grub_probe} --device ${GRUB_DEVICE} --target=fs_label
2>/dev/null || true`
+ bootfs="`make_system_path_relative_to_its_root / | sed -e "s,@$,,"`"
+ LINUX_ROOT_DEVICE="ZFS=${rpool}${bootfs%/}"
+ ;;
+esac
+
+title_correction_code=
+
+linux_entry ()
+{
+ linux_entry_xsm "$@" false
+ linux_entry_xsm "$@" true
+}
+linux_entry_xsm ()
+{
+ os="$1"
+ version="$2"
+ xen_version="$3"
+ type="$4"
+ args="$5"
+ xen_args="$6"
+ xsm="$7"
+ # If user wants to enable XSM support, make sure there's
+ # corresponding policy file.
+ if ${xs
[OSSTEST PATCH 30/36] bookworm: Extend guest bootloader workaround
Signed-off-by: Anthony PERARD
---
Osstest/Debian.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 68f1be60..3545f3fd 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -1176,7 +1176,7 @@ END
logm("\$arch is $arch, \$suite is $suite");
if ($xopts{PvMenuLst} &&
$arch =~ /^arm/ &&
- $suite =~ /wheezy|jessie|stretch|buster|sid/ ) {
+ $suite =~ /wheezy|jessie|stretch|buster|bookworm|sid/ ) {
# Debian doesn't currently know what bootloader to install in
# a Xen guest on ARM. We install pv-grub-menu above which
--
Anthony PERARD
[OSSTEST PATCH 34/36] Temporally switch "qemu-mainline" branch to Bookworm
QEMU doesn't build on buster anymore. This should be remove once bookworm is the default suite. Signed-off-by: Anthony PERARD --- make-flight | 5 + 1 file changed, 5 insertions(+) diff --git a/make-flight b/make-flight index d0c950bc..6e88cb13 100755 --- a/make-flight +++ b/make-flight @@ -49,6 +49,11 @@ case "$xenbranch" in ;; esac +# QEMU doesn't build on buster anymore, use bookworm for it. +case "$branch" in +qemu-mainline) defsuite="bookworm" ;; +esac + # Pick default Debian Installer version to correspond to the chosen # suite. if [ -z "$defdi_version" ] ; then -- Anthony PERARD
[OSSTEST PATCH 27/36] ts-debian-fixup: Fix nic names for bookworm
`xen-create-image` doesn't create image for bookworm with a working
network, we need to fix the interface name.
For reference, there's a bug report upstream:
"UnPredictableNetworkInterfaceNames 'fun' with Bookworm domU: eth0 -> enX0"
https://github.com/xen-tools/xen-tools/issues/65
Signed-off-by: Anthony PERARD
---
ts-debian-fixup | 15 +++
1 file changed, 15 insertions(+)
diff --git a/ts-debian-fixup b/ts-debian-fixup
index 810b3aba..4cf5f980 100755
--- a/ts-debian-fixup
+++ b/ts-debian-fixup
@@ -142,6 +142,20 @@ sub filesystems () {
END
}
+sub fix_networking () {
+return if debian_guest_suite($gho) !~ m/bookworm/;
+
+# `xen-create-image` doesn't setup network in a way that work with
bookworm.
+# The guest boots with "enX0" interface name, but it only try to setup
"eth0".
+# https://github.com/xen-tools/xen-tools/issues/65
+target_editfile_root($ho, $mountpoint."/etc/network/interfaces", sub {
+while (<::EI>) {
+s/\beth0\b/enX0/g;
+print ::EO or die $!;
+}
+});
+}
+
sub unmount () {
guest_umount_lv($ho, $gho);
}
@@ -243,6 +257,7 @@ target_cmd_root($ho, debian_dhcp_rofs_fix($ho,
$mountpoint));
console();
randomseed();
filesystems();
+fix_networking();
otherfixupcfg();
writecfg();
unmount();
--
Anthony PERARD
[OSSTEST PATCH 17/36] target_editfile: Use the same user to retrieve and send
The file "/boot/grub/grub.cfg" on Debian Bookworm isn't accessible
from the "osstest" user, but target_editfile_root() tries to grab the
file as "osstest" then edit it as "root.
Change teditfileex() to use the same $user also to get the file. This
will fix ts-examine-serial-pre step.
Signed-off-by: Anthony PERARD
---
Osstest/TestSupport.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 0dded9b2..b86f1d96 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -783,7 +783,7 @@ sub teditfileex {
logm("editing $rfile to $rdest as $lfile".'{,.new}');
}
-target_getfile($ho, 60, $rfile, $lfile);
+tgetfileex($user, $ho, 60, $rfile, $lfile);
open '::EI', "$lfile" or die "$lfile: $!";
open '::EO', "> $lfile.new" or die "$lfile.new: $!";
--
Anthony PERARD
[OSSTEST PATCH 25/36] ts-leak-check: add new name for udevd workers
udevd on Bookworm shows as "(udev-worker)" in the process list. Suppress them. Signed-off-by: Anthony PERARD --- ts-leak-check | 1 + 1 file changed, 1 insertion(+) diff --git a/ts-leak-check b/ts-leak-check index f3cca8aa..023a945f 100755 --- a/ts-leak-check +++ b/ts-leak-check @@ -203,6 +203,7 @@ xenstore /libxl process .* udevd process .* .*/systemd-udevd +process .* \(udev-worker\) process .* /.+/systemd-shim file /var/run/xenstored/db -- Anthony PERARD
[OSSTEST PATCH 33/36] make-flight: Keep using buster for L2 guest in nested tests
When starting the installation of the L2 guest, L0 kills L1. Switching
the L2 guest back to Debian Buster works fine, so do that to prevent
regression in the test.
Part of the logs from the host L0:
> domain_crash called from arch/x86/hvm/vmx/vvmx.c:2770
> Domain 3 (vcpu#0) crashed on cpu#4:
> d3v0 vmentry failure (reason 0x8021): Invalid guest state (2)
Signed-off-by: Anthony PERARD
---
make-flight | 15 ++-
mfi-common | 5 +++--
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/make-flight b/make-flight
index 155a0c1f..d0c950bc 100755
--- a/make-flight
+++ b/make-flight
@@ -360,6 +360,19 @@ do_hvm_debian_nested_tests () {
xen-4.3-testing) return;;
esac
+ local l2_runvar
+ case $guestsuite in
+ bookworm)
+ # Bookworm install image lead to a crash of l1, so keep using
+ # Buster's image.
+ l2_runvar=(l2_suite=buster)
+ l2_runvar+=(l2_image=$(usual_debianhvm_image amd64 buster))
+ ;;
+ *)
+ l2_runvar=(l2_image=$(usual_debianhvm_image amd64))
+ ;;
+ esac
+
for cpuvendor in amd intel; do
job_create_test
test-$xenarch$kern-$dom0arch$qemuu_suffix-nested-$cpuvendor \
@@ -368,7 +381,7 @@ do_hvm_debian_nested_tests () {
l1_vifmodel='e1000'\
l1_memsize='3072' \
l1_enable_nestedhvm='true' \
-l2_image=$(usual_debianhvm_image amd64)\
+${l2_runvar[@]}\
bios=$bios \
all_hostflags=$most_hostflags,hvm-$cpuvendor
diff --git a/mfi-common b/mfi-common
index 6dc39422..bbe714bf 100644
--- a/mfi-common
+++ b/mfi-common
@@ -547,18 +547,19 @@ job_create_test () {
usual_debianhvm_image () {
local arch=$1; shift
+ local suite=${1:-$guestsuite}
if [ -n "$DEBIAN_IMAGE_FILE" ]; then
echo $DEBIAN_IMAGE_FILE
return
fi
- local file=`getconfig DebianImageFile_${guestsuite}_${arch}`
+ local file=`getconfig DebianImageFile_${suite}_${arch}`
if [ -n "$file" ]; then
echo $file
return
fi
local ver=$DEBIAN_IMAGE_VERSION
if [ -z "$ver" ] ; then
- ver=`getconfig DebianImageVersion_$guestsuite`
+ ver=`getconfig DebianImageVersion_$suite`
fi
echo debian-$ver-$arch-CD-1.iso
}
--
Anthony PERARD
[OSSTEST PATCH 32/36] ts-xen-install: Fix bridge setup, ask to copy MAC addr
This ask to copy the MAC address from $physif on the bridge.
On Debian Bookworm, when running as a Xen guest for nested tests, the
bridge does get a random MAC address and a different IP address from
DHCP than before setting up the bridge, so the test fails.
Signed-off-by: Anthony PERARD
---
ts-xen-install | 3 +++
1 file changed, 3 insertions(+)
diff --git a/ts-xen-install b/ts-xen-install
index 3a913fce..645d8a79 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -312,6 +312,9 @@ sub ensurebridge () {
bridge_ports $physif
bridge_fd 0
bridge_stp off
+END
+$bridgex .= <{Suite} !~
m/wheezy|jessie|stretch|buster/);
+bridge_hw $physif
END
} else {
$iface= $physif;
--
Anthony PERARD
[OSSTEST PATCH 28/36] ts-debian-install: keep avoiding to use pygrub
xen-tools commit 83c37b476a75 ("Start all Debian releases since
Stretch (9) with pygrub by default") started to use pygrub by default.
Revert this.
With "pygrub" setting, xen-create-guest fails on armhf, the
80-install-kernel hook fails because it doesn't know about "armhf".
https://github.com/xen-tools/xen-tools/commit/83c37b476a7534c432ecc9941817aeb989677da6
There's "--nopygrub" but that doesn't work due to several issues, so
removing "pygrub" from "distributions.conf" is the only way.
Signed-off-by: Anthony PERARD
---
ts-debian-install | 15 +++
1 file changed, 15 insertions(+)
diff --git a/ts-debian-install b/ts-debian-install
index c42e8a37..a737bec9 100755
--- a/ts-debian-install
+++ b/ts-debian-install
@@ -78,6 +78,21 @@ sub ginstall () {
fi
END
}
+
+if ($ho->{Suite} =~ m/bookworm/) {
+# remove "pygrub" from /etc/xen-tools/distributions.conf
+# The "--nopygrub" option doesn't work.
+# https://github.com/xen-tools/xen-tools/issues/67
+# https://github.com/xen-tools/xen-tools/issues/68
+target_editfile_root($ho, "/etc/xen-tools/distributions.conf", sub {
+while (<::EI>) {
+unless (m/^#/) {
+s/ pygrub\b//;
+}
+print ::EO or die $!;
+}
+});
+}
my $cmd = '';
my $useproxy = $c{DebianMirrorProxy} // $c{HttpProxy};
$cmd .= <
[OSSTEST PATCH 16/36] bl_getmenu_open: Read grub.cfg as root
On bookworm, "/boot/grub/grub.cfg" isn't accessible by user "osstest",
so read the file as user "root".
Signed-off-by: Anthony PERARD
---
Osstest/Debian.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 31d32d6f..57f31977 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -139,7 +139,7 @@ sub debian_boot_setup ($;$) {
sub bl_getmenu_open ($$$) {
my ($ho, $rmenu, $lmenu) = @_;
-target_getfile($ho, 60, $rmenu, $lmenu);
+target_getfile_root($ho, 60, $rmenu, $lmenu);
my $f= new IO::File $lmenu, 'r' or die "$lmenu $?";
return $f;
}
--
Anthony PERARD
[OSSTEST PATCH 26/36] ts-debian-hvm-install: Allow udev failure in install media
Kernel in "debian-12.1.0-amd64-netinst.iso" prevent debian installer
from booting. Early on, it does `udevadm trigger --action=add`, which
fails, the same way as the following runes fails:
$ cat /sys/devices/virtual/input/input2/name
Xen Virtual Keyboard
$ echo add > /sys/devices/virtual/input/input2/uevent
[ 25.884403] synth uevent: /devices/virtual/input/input2: failed to send
uevent
[ 25.916498] input input2: uevent: failed to send synthetic uevent: -12
sh: write error: Cannot allocate memory
$ uname -a
Linux (none) 6.1.0-10-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.38-1 (2023-07-14)
x86_64 GNU/Linux
This doesn't looks like a new issue, Debian Buster ISO seems to do the
same thing (early boot command, and error in Linux logs), so it's
probable that now `udevadm trigger --action=add` return an error when
there's a failure.
Bug report in the kernel and in Debian:
https://bugzilla.kernel.org/show_bug.cgi?id=207695
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=983357
One way to workaround the issue is to remove the vkb device, with xl's
"vkb_device=0", but libvirt doesn't have support for this config
option.
The second option is to edit the installation media, and allow the
failure.
Once installed, the guest boot fine.
Signed-off-by: Anthony PERARD
---
Notes:
There's a potential fix for the kernel:
https://lore.kernel.org/xen-devel/20221209142615.33574-1-jandr...@gmail.com/
ts-debian-hvm-install | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 4deb443e..44eb3ab1 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -174,13 +174,14 @@ sub isolinux_cfg () {
END
}
-sub prepare_initrd ($$$) {
-my ($initrddir,$newiso,$preseed_file_path) = @_;
+sub prepare_initrd () {
+my ($initrddir,$newiso,$preseed_file_path,$extra_rune) = @_;
return <<"END";
rm -rf $initrddir
mkdir $initrddir
cd $initrddir
gzip -d < $newiso$ramdisk | cpio --extract --make-directories
--no-absolute-filename
+ $extra_rune
cp $preseed_file_path preseed.cfg
find . | cpio -H newc --create | gzip -9 > $newiso$ramdisk
cd -
@@ -268,7 +269,17 @@ sub prep () {
target_putfilecontents_root_stash($ho, 10, preseed(),
$preseed_file_path);
-$cmds = prepare_initrd($initrddir,$newiso,$preseed_file_path);
+my $extra_preseed_rune = '';
+if ($gsuite =~ m/bookworm/) {
+# Xen Virtual Keyboard initialisation fails and return ENOMEM
+# https://bugzilla.kernel.org/show_bug.cgi?id=207695
+# https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=983357
+$extra_preseed_rune .= <
[OSSTEST PATCH 29/36] ts-debian-hvm-install: Increase min guest ram size
768 MB for the guest isn't enough anymore for Debian Bookworm, at boot
it print this message:
Low memory
--
Entering low memory mode
This system has relatively little free memory, so it will enter low memory
mode. Among other things, this means that this program will proceed in
English.
You should set up swap space as soon as possible.
[Press enter to continue]
So, automatic installation fails. An empiric test shows that the min
was 817M for one particular situation (test-amd64-amd64-xl-qemuu-ovmf-amd64)
but we probably need some leg room, so increase to 1 GB.
Signed-off-by: Anthony PERARD
---
ts-debian-hvm-install | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 44eb3ab1..60c95b37 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -241,7 +241,7 @@ sub prep () {
if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
$ram_mb = $ram_lots;
} else {
- $ram_mb = 768;
+ $ram_mb = 1024;
}
}
logm("Host has $host_freemem_mb MB free memory, setting guest memory size
to $ram_mb MB");
--
Anthony PERARD
[OSSTEST PATCH 31/36] ts-debian-*-install: Replace dots in hostnames by dashs
When running ./ts-debian-di-install, hostname on the command line is
interpreted by the debian installer. As the installer find it to be a
FQDN, it uses part of the hostname as the domain, thus overwriting the
value from the DHCP and from d-i netcfg/get_domain setting (maybe).
But the result is that /etc/resolv.conf contains "search
bookworm.guest.osstest" and can't find an IP for "cache". So the
installation fails.
Also replace ".guest.osstest" in a few other places, even if it
may not be an issue.
Signed-off-by: Anthony PERARD
---
ts-debian-di-install | 4 ++--
ts-debian-hvm-install | 2 +-
ts-debian-install | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/ts-debian-di-install b/ts-debian-di-install
index d84407cf..06c7e1f4 100755
--- a/ts-debian-di-install
+++ b/ts-debian-di-install
@@ -68,8 +68,8 @@ our $ram_mb= $r{arch} =~ m/^armhf/ ? 768 : 2048;
our $disk_mb= 1;
our $guesthost= $gn.
-($r{"${gn}_suite"} ? ".".$r{"${gn}_suite"} : "").
-".guest.osstest";
+($r{"${gn}_suite"} ? "-".$r{"${gn}_suite"} : "").
+"-guest-osstest";
our $gho;
sub prep () {
diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 60c95b37..99e9acae 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -46,7 +46,7 @@ our $ho= selecthost($whhost);
our $ram_mb;
our $disk_mb= 1;
-our $guesthost= "$gn.guest.osstest";
+our $guesthost= "$gn-guest-osstest";
our $gho;
our ($kernel, $ramdisk);
diff --git a/ts-debian-install b/ts-debian-install
index a737bec9..62db487a 100755
--- a/ts-debian-install
+++ b/ts-debian-install
@@ -34,7 +34,7 @@ our $ram_mb=512;
our $swap_mb= 1000;
our $disk_mb= 1;
-our $guesthost= "$gn.guest.osstest";
+our $guesthost= "$gn-guest-osstest";
our $gho;
sub prep () {
--
Anthony PERARD
