On Mon, Sep 16, 2024 at 03:20:56PM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Sep 16, 2024 at 02:11:08PM +0100, Andrew Cooper wrote:
> > On 13/09/2024 8:59 am, Roger Pau Monne wrote:
> > > diff --git a/docs/misc/xen-command-line.pandoc
> > > b/docs/misc/xen-command-line.pandoc
> > > index 959cf45b55d9..2a9b3b9b8975 100644
> > > --- a/docs/misc/xen-command-line.pandoc
> > > +++ b/docs/misc/xen-command-line.pandoc
> > > @@ -2816,6 +2816,27 @@ vwfi to `native` reduces irq latency
> > > significantly. It can also lead to
> > > suboptimal scheduling decisions, but only when the system is
> > > oversubscribed (i.e., in total there are more vCPUs than pCPUs).
> > >
> > > +### wallclock (x86)
> > > +> `= auto | xen | cmos | efi`
> > > +
> > > +> Default: `auto`
> > > +
> > > +Allow forcing the usage of a specific wallclock source.
> > > +
> > > + * `auto` let the hypervisor select the clocksource based on internal
> > > + heuristics.
> > > +
> > > + * `xen` force usage of the Xen shared_info wallclock when booted as a
> > > Xen
> > > + guest. This option is only available if the hypervisor was compiled
> > > with
> > > + `CONFIG_XEN_GUEST` enabled.
> > > +
> > > + * `cmos` force usage of the CMOS RTC wallclock.
> > > +
> > > + * `efi` force usage of the EFI_GET_TIME run-time method when booted
> > > from EFI
> > > + firmware.
> >
> > For both `xen` and `efi`, something should be said about "if selected
> > and not satisfied, Xen falls back to other heuristics".
There's a line just below that notes this: "If the selected option is
invalid or not available Xen will default to `auto`." I think it's
clearer than having to comment on every specific option.
> > > +
> > > +If the selected option is invalid or not available Xen will default to
> > > `auto`.
> >
> > I'm afraid that I'm firmly of the opinion that "auto" on the cmdline is
> > unnecessary complexity. Auto is the default, and doesn't need
> > specifying explicitly. That in turn simplifies ...
>
> What about overriding earlier choice? For example overriding a built-in
> cmdline? That said, with the current code, the same can be achieved with
> wallclock=foo, and living with the warning in boot log...
It's IMO weird to ask the users to use wallclock=foo to override a
previous command line wallclock option and fallback to the default
behavior.
> > > +
> > > ### watchdog (x86)
> > > > `= force | <boolean>`
> > >
> > > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > > index 29b026735e5d..e4751684951e 100644
> > > --- a/xen/arch/x86/time.c
> > > +++ b/xen/arch/x86/time.c
> > > @@ -1552,6 +1552,37 @@ static const char *__init
> > > wallclock_type_to_string(void)
> > > return "";
> > > }
> > >
> > > +static int __init cf_check parse_wallclock(const char *arg)
> > > +{
> > > + wallclock_source = WALLCLOCK_UNSET;
> > > +
> > > + if ( !arg )
> > > + return -EINVAL;
> > > +
> > > + if ( !strcmp("auto", arg) )
> > > + ASSERT(wallclock_source == WALLCLOCK_UNSET);
> >
> > ... this.
> >
> > Hitting this assert will manifest as a system reboot/hang with no
> > information on serial/VGA, because all of this runs prior to getting up
> > the console. You only get to see it on a PVH boot because we bodge the
> > Xen console into default-existence.
>
> This assert is no-op as wallclock_source is unconditionally set to
> WALLCLOCK_UNSET few lines above.
As mentioned to Jan - I find it nicer than adding an empty statement.
> > So, ASSERT()/etc really need avoiding wherever possible in cmdline parsing.
> >
> > In this case, all it serves to do is break examples like `wallclock=xen
> > wallclock=auto` case, which is unlike our latest-takes-precedence
> > behaviour everywhere else.
> >
> > > + else if ( !strcmp("xen", arg) )
> > > + {
> > > + if ( !xen_guest )
> >
> > We don't normally treat this path as an error when parsing (we know what
> > it is, even if we can't action it). Instead, there's no_config_param()
> > to be more friendly (for PVH at least).
> >
> > It's a bit awkward, but this should do:
> >
> > {
> > #ifdef CONFIG_XEN_GUEST
> > wallclock_source = WALLCLOCK_XEN;
> > #else
> > no_config_param("XEN_GUEST", "wallclock", s, ss);
> > #endif
> > }
>
> Can you boot the binary build with CONFIG_XEN_GUEST=y as native? If so,
> the above will not be enough, a runtime check is needed anyway.
>
> > There probably wants to be something similar for EFI, although it's not
> > a plain CONFIG so it might be more tricky.
>
> It needs to be runtime check here even more. Not only because of
> different boot modes, but due to interaction with efi=no-rs (or any
> other reason for not having runtime services). See the comment there.
I agree with Marek, no_config_param() is not enough here: Xen needs to
ensure it has been booted as a Xen guest, or that EFI run-time
services are enabled in order to ensure the selected clock source is
available.
Thanks, Roger.
