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 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.

Thanks, Roger.