[PATCH] Revert "iee1275/datetime: Fix off-by-1 error."

[PATCH] Revert "iee1275/datetime: Fix off-by-1 error."

[Glenn Washburn]

This is causing the test grub_cmd_date to fail because the returned date is
one day more than it should be.

This reverts commit 607d66116a67e5a13eb0d46076f26dedc988e6a4.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
Hi all,

Reverting this commit allows the grub_cmd_date test to pass. It appears that
this commit is (now) causing an off-by-1 error in QEMU and the latest OpenBIOS.
What I'm unsure of is if the original commit is actually correct on real
hardware and that potentially OpenBIOS has a bug.

Adrian and Daniel A, could you test the reverting of this commit on real
hardware and see if date does in fact produce the expected date (and do current
builds show a date one day ahead of what it should be)? Can anyone point to
documentation saying that the original commit is in fact what should
be done? If the issue is in OpenBIOS I'd like to have some documentation
to back up a bug report.

Vladimir, do you have any thoughts on this?

Glenn

---
 grub-core/lib/ieee1275/datetime.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/lib/ieee1275/datetime.c b/grub-core/lib/ieee1275/datetime.c
index b81fba2ed..74578f15a 100644
--- a/grub-core/lib/ieee1275/datetime.c
+++ b/grub-core/lib/ieee1275/datetime.c
@@ -95,7 +95,7 @@ grub_get_datetime (struct grub_datetime *datetime)
 
   datetime->year = args.year;
   datetime->month = args.month;
-  datetime->day = args.day + 1;
+  datetime->day = args.day;
   datetime->hour = args.hour;
   datetime->minute = args.minute;
   datetime->second = args.second;
@@ -140,7 +140,7 @@ grub_set_datetime (struct grub_datetime *datetime)
 
   args.year = datetime->year;
   args.month = datetime->month;
-  args.day = datetime->day - 1;
+  args.day = datetime->day;
   args.hour = datetime->hour;
   args.minute = datetime->minute;
   args.second = datetime->second;
-- 
2.27.0

Re: [PATCH] Revert "iee1275/datetime: Fix off-by-1 error."

[Daniel Axtens]

Hi,

I tested a pseries guest under:
 - qemu + KVM on a Power8 host with a fairly modern qemu
 - qemu + TCG

Here the 'date' command simply reports "error: no cmos found", so
this patch will have no impact on those platforms.

I also tested an LPAR on a Power8 PowerVM machine. In this case, before
the patch, grub printed:

grub> date
2022-02-09 04:31:10 Wednesday

whereas on booting:

[dja@sauce ~]$ date -u
Tue Feb  8 04:46:40 UTC 2022

After applying the patch, grub printed:

grub> date
2022-02-08 04:51:27 Tuesday

It seems the patch makes things better.

Tested-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

Glenn Washburn <development@efficientek.com> writes:

> This is causing the test grub_cmd_date to fail because the returned date is
> one day more than it should be.
>
> This reverts commit 607d66116a67e5a13eb0d46076f26dedc988e6a4.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> Hi all,
>
> Reverting this commit allows the grub_cmd_date test to pass. It appears that
> this commit is (now) causing an off-by-1 error in QEMU and the latest OpenBIOS.
> What I'm unsure of is if the original commit is actually correct on real
> hardware and that potentially OpenBIOS has a bug.
>
> Adrian and Daniel A, could you test the reverting of this commit on real
> hardware and see if date does in fact produce the expected date (and do current
> builds show a date one day ahead of what it should be)? Can anyone point to
> documentation saying that the original commit is in fact what should
> be done? If the issue is in OpenBIOS I'd like to have some documentation
> to back up a bug report.
>
> Vladimir, do you have any thoughts on this?
>
> Glenn
>
> ---
>  grub-core/lib/ieee1275/datetime.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/lib/ieee1275/datetime.c b/grub-core/lib/ieee1275/datetime.c
> index b81fba2ed..74578f15a 100644
> --- a/grub-core/lib/ieee1275/datetime.c
> +++ b/grub-core/lib/ieee1275/datetime.c
> @@ -95,7 +95,7 @@ grub_get_datetime (struct grub_datetime *datetime)
>  
>    datetime->year = args.year;
>    datetime->month = args.month;
> -  datetime->day = args.day + 1;
> +  datetime->day = args.day;
>    datetime->hour = args.hour;
>    datetime->minute = args.minute;
>    datetime->second = args.second;
> @@ -140,7 +140,7 @@ grub_set_datetime (struct grub_datetime *datetime)
>  
>    args.year = datetime->year;
>    args.month = datetime->month;
> -  args.day = datetime->day - 1;
> +  args.day = datetime->day;
>    args.hour = datetime->hour;
>    args.minute = datetime->minute;
>    args.second = datetime->second;
> -- 
> 2.27.0

Re: [PATCH] Revert "iee1275/datetime: Fix off-by-1 error."

[Glenn Washburn]

On Tue, 08 Feb 2022 15:51:41 +1100
Daniel Axtens <dja@axtens.net> wrote:

> Hi,
> 
> I tested a pseries guest under:
>  - qemu + KVM on a Power8 host with a fairly modern qemu
>  - qemu + TCG
> 
> Here the 'date' command simply reports "error: no cmos found", so
> this patch will have no impact on those platforms.

I suspect its the same on KVM and TCG because the machine definition is
the same and doesn't container either an RTC nor a CMOS. What confusing
to me is, don't these machines have clocks?

> 
> I also tested an LPAR on a Power8 PowerVM machine. In this case, before
> the patch, grub printed:
> 
> grub> date
> 2022-02-09 04:31:10 Wednesday
> 
> whereas on booting:
> 
> [dja@sauce ~]$ date -u
> Tue Feb  8 04:46:40 UTC 2022
> 
> After applying the patch, grub printed:
> 
> grub> date
> 2022-02-08 04:51:27 Tuesday
> 
> It seems the patch makes things better.

Great thanks Daniel for confirming this.

Glenn

> 
> Tested-by: Daniel Axtens <dja@axtens.net>
> 
> Kind regards,
> Daniel
> 
> Glenn Washburn <development@efficientek.com> writes:
> 
> > This is causing the test grub_cmd_date to fail because the returned date is
> > one day more than it should be.
> >
> > This reverts commit 607d66116a67e5a13eb0d46076f26dedc988e6a4.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> > Hi all,
> >
> > Reverting this commit allows the grub_cmd_date test to pass. It appears that
> > this commit is (now) causing an off-by-1 error in QEMU and the latest OpenBIOS.
> > What I'm unsure of is if the original commit is actually correct on real
> > hardware and that potentially OpenBIOS has a bug.
> >
> > Adrian and Daniel A, could you test the reverting of this commit on real
> > hardware and see if date does in fact produce the expected date (and do current
> > builds show a date one day ahead of what it should be)? Can anyone point to
> > documentation saying that the original commit is in fact what should
> > be done? If the issue is in OpenBIOS I'd like to have some documentation
> > to back up a bug report.
> >
> > Vladimir, do you have any thoughts on this?
> >
> > Glenn
> >
> > ---
> >  grub-core/lib/ieee1275/datetime.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/grub-core/lib/ieee1275/datetime.c b/grub-core/lib/ieee1275/datetime.c
> > index b81fba2ed..74578f15a 100644
> > --- a/grub-core/lib/ieee1275/datetime.c
> > +++ b/grub-core/lib/ieee1275/datetime.c
> > @@ -95,7 +95,7 @@ grub_get_datetime (struct grub_datetime *datetime)
> >  
> >    datetime->year = args.year;
> >    datetime->month = args.month;
> > -  datetime->day = args.day + 1;
> > +  datetime->day = args.day;
> >    datetime->hour = args.hour;
> >    datetime->minute = args.minute;
> >    datetime->second = args.second;
> > @@ -140,7 +140,7 @@ grub_set_datetime (struct grub_datetime *datetime)
> >  
> >    args.year = datetime->year;
> >    args.month = datetime->month;
> > -  args.day = datetime->day - 1;
> > +  args.day = datetime->day;
> >    args.hour = datetime->hour;
> >    args.minute = datetime->minute;
> >    args.second = datetime->second;
> > -- 
> > 2.27.0

Re: [PATCH] Revert "iee1275/datetime: Fix off-by-1 error."

[Daniel Axtens]

> I suspect its the same on KVM and TCG because the machine definition is
> the same and doesn't container either an RTC nor a CMOS. What confusing
> to me is, don't these machines have clocks?

They do have clocks! Sadly I know very little about the clocks. `date`
works fine in Linux, so I suspect there's something where SLOF doesn't
expose whatever it is that we need in the form we expect, or something
like that. 

I guess someone should hunt it down but given that timeouts and such
work (so I guess we have a working relative timer), I can't say it's
likely to be a huge priority for anyone...

Kind regards,
Daniel

>
>> 
>> I also tested an LPAR on a Power8 PowerVM machine. In this case, before
>> the patch, grub printed:
>> 
>> grub> date
>> 2022-02-09 04:31:10 Wednesday
>> 
>> whereas on booting:
>> 
>> [dja@sauce ~]$ date -u
>> Tue Feb  8 04:46:40 UTC 2022
>> 
>> After applying the patch, grub printed:
>> 
>> grub> date
>> 2022-02-08 04:51:27 Tuesday
>> 
>> It seems the patch makes things better.
>
> Great thanks Daniel for confirming this.
>
> Glenn
>
>> 
>> Tested-by: Daniel Axtens <dja@axtens.net>
>> 
>> Kind regards,
>> Daniel
>> 
>> Glenn Washburn <development@efficientek.com> writes:
>> 
>> > This is causing the test grub_cmd_date to fail because the returned date is
>> > one day more than it should be.
>> >
>> > This reverts commit 607d66116a67e5a13eb0d46076f26dedc988e6a4.
>> >
>> > Signed-off-by: Glenn Washburn <development@efficientek.com>
>> > ---
>> > Hi all,
>> >
>> > Reverting this commit allows the grub_cmd_date test to pass. It appears that
>> > this commit is (now) causing an off-by-1 error in QEMU and the latest OpenBIOS.
>> > What I'm unsure of is if the original commit is actually correct on real
>> > hardware and that potentially OpenBIOS has a bug.
>> >
>> > Adrian and Daniel A, could you test the reverting of this commit on real
>> > hardware and see if date does in fact produce the expected date (and do current
>> > builds show a date one day ahead of what it should be)? Can anyone point to
>> > documentation saying that the original commit is in fact what should
>> > be done? If the issue is in OpenBIOS I'd like to have some documentation
>> > to back up a bug report.
>> >
>> > Vladimir, do you have any thoughts on this?
>> >
>> > Glenn
>> >
>> > ---
>> >  grub-core/lib/ieee1275/datetime.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/grub-core/lib/ieee1275/datetime.c b/grub-core/lib/ieee1275/datetime.c
>> > index b81fba2ed..74578f15a 100644
>> > --- a/grub-core/lib/ieee1275/datetime.c
>> > +++ b/grub-core/lib/ieee1275/datetime.c
>> > @@ -95,7 +95,7 @@ grub_get_datetime (struct grub_datetime *datetime)
>> >  
>> >    datetime->year = args.year;
>> >    datetime->month = args.month;
>> > -  datetime->day = args.day + 1;
>> > +  datetime->day = args.day;
>> >    datetime->hour = args.hour;
>> >    datetime->minute = args.minute;
>> >    datetime->second = args.second;
>> > @@ -140,7 +140,7 @@ grub_set_datetime (struct grub_datetime *datetime)
>> >  
>> >    args.year = datetime->year;
>> >    args.month = datetime->month;
>> > -  args.day = datetime->day - 1;
>> > +  args.day = datetime->day;
>> >    args.hour = datetime->hour;
>> >    args.minute = datetime->minute;
>> >    args.second = datetime->second;
>> > -- 
>> > 2.27.0

Re: [PATCH] Revert "iee1275/datetime: Fix off-by-1 error."

[Daniel Kiper]

On Tue, Feb 08, 2022 at 03:51:41PM +1100, Daniel Axtens wrote:
> Hi,
>
> I tested a pseries guest under:
>  - qemu + KVM on a Power8 host with a fairly modern qemu
>  - qemu + TCG
>
> Here the 'date' command simply reports "error: no cmos found", so
> this patch will have no impact on those platforms.
>
> I also tested an LPAR on a Power8 PowerVM machine. In this case, before
> the patch, grub printed:
>
> grub> date
> 2022-02-09 04:31:10 Wednesday
>
> whereas on booting:
>
> [dja@sauce ~]$ date -u
> Tue Feb  8 04:46:40 UTC 2022
>
> After applying the patch, grub printed:
>
> grub> date
> 2022-02-08 04:51:27 Tuesday
>
> It seems the patch makes things better.
>
> Tested-by: Daniel Axtens <dja@axtens.net>

Daniel, thank you for testing this!

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel