Re: [PATCH v3 13/24] xen/console: introduce hwdom_crashconsole=

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> The new command line switch `hwdom_crashconsole=BOOL` allows to switch serial
> console input focus to xen for machine state inspection using keyhandler
> mechanism after the hardware domain crashes.
> 
> The new command line switch is aliased via `dom0=...,crashconsole` knob.
> 
> Such functionality can be useful while debugging dom0 bringup.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

First, going back to what was said on the earlier version of the series as
a whole: Is this really strictly related (i.e. in preparation of) adding
the vNS16550 driver? As previously indicated, it would help if the series
focused on its main goal, with less tightly related stuff kept for later
(or submitted independently where possible).

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -822,6 +822,7 @@ Specify the bit width of the DMA heap.
>  
>  ### dom0
>      = List of [ pv | pvh, shadow=<bool>, verbose=<bool>,
> +                crashconsole=<bool>,
>                  cpuid-faulting=<bool>, msr-relaxed=<bool> ] (x86)
>  
>      = List of [ sve=<integer> ] (Arm64)
> @@ -855,6 +856,10 @@ Controls for how dom0 is constructed on x86 systems.
>      information during the dom0 build.  It defaults to the compile time 
> choice
>      of `CONFIG_VERBOSE_DEBUG`.
>  
> +*   The `crashconsole` boolean instructs Xen to switch input console focus to
> +    the hypervisor when dom0 shuts down and avoid performing dom0 domain
> +    destruction.  Should only be used for debugging purposes.
> +
>  *   The `cpuid-faulting` boolean is an interim option, is only applicable to
>      PV dom0, and defaults to true.

With this, ...

> @@ -1491,6 +1496,15 @@ Specify whether guests are to be given access to 
> physical port 80
>  (often used for debugging purposes), to override the DMI based
>  detection of systems known to misbehave upon accesses to that port.
>  
> +### hwdom_crashconsole
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +The `hwdom_crashconsole` boolean instructs Xen to switch input console focus 
> to
> +the hypervisor when dom0 shuts down and avoid performing dom0 domain
> +destruction.  Should only be used for debugging purposes.

... do we then need this? We're kind of trying to limit the number of new
top-level options.

Then: Please prefer dashes over underscores in new command line options.

> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -286,6 +286,8 @@ int __init parse_arch_dom0_param(const char *s, const 
> char *e)
>          opt_dom0_cpuid_faulting = val;
>      else if ( (val = parse_boolean("msr-relaxed", s, e)) >= 0 )
>          opt_dom0_msr_relaxed = val;
> +    else if ( (val = parse_boolean("crashconsole", s, e)) >= 0 )
> +        opt_hwdom_crashconsole = !!val;

No need for !! when the lhs is of type bool (as can be seen in two
examples in context).

> @@ -1162,7 +1169,12 @@ int domain_shutdown(struct domain *d, u8 reason)
>      reason = d->shutdown_code;
>  
>      if ( is_hardware_domain(d) )
> -        hwdom_shutdown(reason);
> +    {
> +        if ( opt_hwdom_crashconsole )
> +            console_set_owner(DOMID_XEN);
> +        else
> +            hwdom_shutdown(reason);
> +    }

And how will the rest of the system be halted? Keeping Xen alive
is one thing; keeping domains alive is another (and imo not
acceptable). You don't need to destroy them, but you want to pause
them all.

Jan