Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/14/25 3:58 PM, Roger Pau Monné wrote:
> On Tue, Jan 14, 2025 at 03:23:21PM +0100, Oleksii Kurochko wrote:
> > On 1/14/25 1:13 PM, Roger Pau Monné wrote:
> > > On Tue, Jan 14, 2025 at 12:44:39PM +0100, Oleksii Kurochko wrote:
> > > > On 1/14/25 12:40 PM, Oleksii Kurochko wrote:
> > > > > On 1/14/25 12:27 PM, Roger Pau Monné wrote:
> > > > > > On Tue, Jan 14, 2025 at 12:12:03PM +0100, Oleksii Kurochko wrote:
> > > > > > > On 1/13/25 6:52 PM, Roger Pau Monné wrote:
> > > > > > > > On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
> > > > > > > > > On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
> > > > > > > > > > Allow setting the used wallclock from the command line.  When the option is set
> > > > > > > > > > to a value different than `auto` the probing is bypassed and the selected
> > > > > > > > > > implementation is used (as long as it's available).
> > > > > > > > > >
> > > > > > > > > > The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
> > > > > > > > > > supported built-in) or from UEFI firmware respectively.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Roger Pau Monné<roger.pau@xxxxxxxxxx>
> > > > > > > > > Reviewed-by: Marek Marczykowski-Górecki<marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > > > > > > Thanks for the review.
> > > > > > > >
> > > > > > > > Oleksii, can I get your opinion as Release Manager about whether this
> > > > > > > > (and the following patch) would be suitable for committing to staging
> > > > > > > > given the current release state?
> > > > > > > >
> > > > > > > > It's a workaround for broken EFI implementations that many downstreams
> > > > > > > > carry on their patch queue.
> > > > > > > Based on your commit message, I understand this as addressing a bug ( but not 
> > > > > > > very critical
> > > > > > > as IIUC downstreams have the similar patch on their side ). Therefore, if it 
> > > > > > > has been properly
> > > > > > > reviewed and tested, we should consider including it in the current release.
> > > > > > IIRC at least Qubes, XenServer and XCP-ng have a patch that achieves
> > > > > > the same behavior as proposed here.
> > > > > >
> > > > > > > IIUC, setting the wallclock to EFI should align with the behavior Xen had 
> > > > > > > previously.
> > > > > > > It might be preferable to use that argument as the default for a while, allowing others 
> > > > > > > to verify the "auto"
> > > > > > > value over time. After that, we could consider making "auto" the default.
> > > > > > > That said, I am not particularly strict about following this approach.
> > > > > > We cannot really set efi as the default, as it would break when
> > > > > > booting on legacy BIOS systems.
> > > > > >
> > > > > > We could take only patch 1 and leave patch 2 after Xen 4.20 has
> > > > > > branched, but at that point I would see little benefit in having just
> > > > > > patch 1.
> > > > > I don't see a lot of benefit of comitting only the one patch either.
> > > > >
> > > > >
> > > > > > I don't have a strong opinion, but downstreams have been complaining
> > > > > > about Xen behavior regarding the usage of EFI_GET_TIME, so it might be
> > > > > > good to not ship yet another release with such allegedly broken
> > > > > > behavior.
> > > > > Agree with that. As  I mentioned above I consider it as a bug and based on
> > > > > that several mentioned above downstreams have the similar patch I could consider
> > > > > that as tested approach so ..
> > > > > > Let me know what you think, as I would need a formal Release-Ack if
> > > > > > this is to be committed.
> > > > > ... R-Acked-by: Oleksii Kurochko<oleksii.kurochko@xxxxxxxxx>.
> > > > Sorry for the noise.
> > > >
> > > > I missed to add that it would also be nice to mention IMO ( that now we have 
> > > > wallclock parameter )
> > > > in CHANGELOG so downstreams will receive "notification" that Xen provides a 
> > > > workaround for the mentioned
> > > > issue in the patch series.
> > > Indeed.  Would you be OK with adding the following chunk to patch 2?
> > >
> > > diff --git a/CHANGELOG.md b/CHANGELOG.md
> > > index 8507e6556a56..1de1d1eca17f 100644
> > > --- a/CHANGELOG.md
> > > +++ b/CHANGELOG.md
> > > @@ -12,6 +12,7 @@ The format is based on [Keep a 
> > > Changelog](https://keepachangelog.com/en/1.0.0/)
> > >       leaving this to the guest kernel to do in guest context.
> > >     - On x86:
> > >       - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
> > > +   - Prefer CMOS over EFI_GET_TIME as time source.
> > >       - Switched the xAPIC flat driver to use physical destination mode for 
> > > external
> > >         interrupts instead of logical destination mode.
> > > @@ -24,6 +25,7 @@ The format is based on [Keep a 
> > > Changelog](https://keepachangelog.com/en/1.0.0/)
> > >       - Support for LLC (Last Level Cache) coloring.
> > >     - On x86:
> > >       - xl suspend/resume subcommands.
> > > +   - `wallclock` command line option to select time source.
> > What about of adding word 'Add' before `wallclock`?
> It's in the "Added" section, so I thought it would be redundant.
>
> > Other LGTM: Acked-By: Oleksii Kurochko<oleksii.kurochko@xxxxxxxxx>
> Let me know if you would still like me to prepend "Add" to the above
> item.
Oh, then it makes sense. We can go without "Add" then.


~ Oleksii

> Thanks, Roger.