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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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

Thanks.
~ Oleksii
> Thanks, Roger.
>
> > ~ Oleksii
> >
> >
> > > Thanks, Roger.
> > >         
> >       
>