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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.
 
 ### Removed
  - On x86: