Re: [Qemu-devel] [PATCH 05/14] timer: ds1338 change write handling and fix wday_offset handling

[David Gibson]

On Thu, Jul 05, 2018 at 11:19:52AM -0700, Michael Davidsaver wrote:
> instead of a read-modify-write, do direct translation
> of device registers to struct tm members.
> 
> This new ds1338_update() is the reverse of
> the existing capture_current_time().
> 
> Simplifies later handling of CENTURY bit in
> similar Dallas RTC chips.
> 
> Correctly handle different real weekday in
> guest and host timezones.
> Allow guest to use any day as start of week
> (day 1).  eg. Monday instead of Sunday.
> 
> Signed-off-by: Michael Davidsaver 
> ---
>  hw/timer/ds1338.c | 95 
> ---
>  1 file changed, 48 insertions(+), 47 deletions(-)
> 
> diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
> index 637a0f4246..0134ffd72a 100644
> --- a/hw/timer/ds1338.c
> +++ b/hw/timer/ds1338.c
> @@ -110,7 +110,10 @@ static void capture_current_time(DS1338State *s)
>  /* 24 hour mode. */
>  s->nvram[R_HOUR] = to_bcd(now.tm_hour);
>  }
> -s->nvram[R_WDAY] = (now.tm_wday + s->wday_offset) % 7 + 1;
> +s->nvram[R_WDAY] = (now.tm_wday + s->wday_offset) % 7;
> +if (s->nvram[R_WDAY] == 0) {
> +s->nvram[R_WDAY] = 7;
> +}

This looks a bit odd - you've removed the + 1 here, but...

>  s->nvram[R_DATE] = to_bcd(now.tm_mday);
>  s->nvram[R_MONTH] = to_bcd(now.tm_mon + 1);
>  s->nvram[R_YEAR] = to_bcd(now.tm_year - 100);
> @@ -161,6 +164,46 @@ static int ds1338_recv(I2CSlave *i2c)
>  return res;
>  }
>  
> +/* call after guest writes to current time registers
> + * to re-compute our offset from host time.
> + */
> +static void ds1338_update(DS1338State *s)
> +{
> +
> +struct tm now = {};
> +
> +/* TODO: Implement CH (stop) bit?  */
> +now.tm_sec = from_bcd(s->nvram[R_SEC] & 0x7f);
> +now.tm_min = from_bcd(s->nvram[R_MIN] & 0x7f);
> +if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) {
> +/* 12 hour (1-12) */
> +/* read and wrap 1-12 -> 0-11 */
> +now.tm_hour = from_bcd(ARRAY_FIELD_EX32(s->nvram, HOUR, HOUR12)) % 
> 12u;
> +if (ARRAY_FIELD_EX32(s->nvram, HOUR, AMPM)) {
> +now.tm_hour += 12;
> +}
> +
> +} else {
> +now.tm_hour = from_bcd(ARRAY_FIELD_EX32(s->nvram, HOUR, HOUR24));
> +}
> +now.tm_wday = from_bcd(s->nvram[R_WDAY]) - 1u;

.. you still have the -1 here.  Am I missing something?

> +now.tm_mday = from_bcd(s->nvram[R_DATE] & 0x3f);
> +now.tm_mon = from_bcd(s->nvram[R_MONTH] & 0x1f) - 1;
> +now.tm_year = from_bcd(s->nvram[R_YEAR]) + 100;
> +s->offset = qemu_timedate_diff();
> +
> +{
> +/* Round trip to get real wday_offset based on time delta and
> + * ref. timezone.
> + * Race if midnight (in ref. timezone) happens here.
> + */
> +int user_wday = now.tm_wday;
> +qemu_get_timedate(, s->offset);
> +
> +s->wday_offset = (user_wday - now.tm_wday) % 7 + 1;
> +}
> +}
> +
>  static int ds1338_send(I2CSlave *i2c, uint8_t data)
>  {
>  DS1338State *s = DS1338(i2c);
> @@ -170,52 +213,7 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
>  s->addr_byte = false;
>  return 0;
>  }
> -if (s->ptr < 7) {
> -/* Time register. */
> -struct tm now;
> -qemu_get_timedate(, s->offset);
> -switch(s->ptr) {
> -case R_SEC:
> -/* TODO: Implement CH (stop) bit.  */
> -now.tm_sec = from_bcd(data & 0x7f);
> -break;
> -case R_MIN:
> -now.tm_min = from_bcd(data & 0x7f);
> -break;
> -case R_HOUR:
> -if (FIELD_EX32(data, HOUR, SET12)) {
> -/* 12 hour (1-12) */
> -/* read and wrap 1-12 -> 0-11 */
> -now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u;
> -if (FIELD_EX32(data, HOUR, AMPM)) {
> -now.tm_hour += 12;
> -}
> -
> -} else {
> -now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24));
> -}
> -break;
> -case R_WDAY:
> -{
> -/* The day field is supposed to contain a value in
> -   the range 1-7. Otherwise behavior is undefined.
> - */
> -int user_wday = (data & 7) - 1;
> -s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
> -}
> -break;
> -case R_DATE:
> -now.tm_mday = from_bcd(data & 0x3f);
> -break;
> -case R_MONTH:
> -now.tm_mon = from_bcd(data & 0x1f) - 1;
> -break;
> -case R_YEAR:
> -now.tm_year = from_bcd(data) + 100;
> -break;
> -}
> -s->offset = qemu_timedate_diff();
> -} else if (s->ptr == R_CTRL) {
> +if (s->ptr == R_CTRL) {
>  /* Control register. */
>  
>  /* Ensure bits 2, 3 and 6 will read back as zero. */
> @@ 

[Qemu-devel] [PATCH 05/14] timer: ds1338 change write handling and fix wday_offset handling

[Michael Davidsaver]

instead of a read-modify-write, do direct translation
of device registers to struct tm members.

This new ds1338_update() is the reverse of
the existing capture_current_time().

Simplifies later handling of CENTURY bit in
similar Dallas RTC chips.

Correctly handle different real weekday in
guest and host timezones.
Allow guest to use any day as start of week
(day 1).  eg. Monday instead of Sunday.

Signed-off-by: Michael Davidsaver 
---
 hw/timer/ds1338.c | 95 ---
 1 file changed, 48 insertions(+), 47 deletions(-)

diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
index 637a0f4246..0134ffd72a 100644
--- a/hw/timer/ds1338.c
+++ b/hw/timer/ds1338.c
@@ -110,7 +110,10 @@ static void capture_current_time(DS1338State *s)
 /* 24 hour mode. */
 s->nvram[R_HOUR] = to_bcd(now.tm_hour);
 }
-s->nvram[R_WDAY] = (now.tm_wday + s->wday_offset) % 7 + 1;
+s->nvram[R_WDAY] = (now.tm_wday + s->wday_offset) % 7;
+if (s->nvram[R_WDAY] == 0) {
+s->nvram[R_WDAY] = 7;
+}
 s->nvram[R_DATE] = to_bcd(now.tm_mday);
 s->nvram[R_MONTH] = to_bcd(now.tm_mon + 1);
 s->nvram[R_YEAR] = to_bcd(now.tm_year - 100);
@@ -161,6 +164,46 @@ static int ds1338_recv(I2CSlave *i2c)
 return res;
 }
 
+/* call after guest writes to current time registers
+ * to re-compute our offset from host time.
+ */
+static void ds1338_update(DS1338State *s)
+{
+
+struct tm now = {};
+
+/* TODO: Implement CH (stop) bit?  */
+now.tm_sec = from_bcd(s->nvram[R_SEC] & 0x7f);
+now.tm_min = from_bcd(s->nvram[R_MIN] & 0x7f);
+if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) {
+/* 12 hour (1-12) */
+/* read and wrap 1-12 -> 0-11 */
+now.tm_hour = from_bcd(ARRAY_FIELD_EX32(s->nvram, HOUR, HOUR12)) % 12u;
+if (ARRAY_FIELD_EX32(s->nvram, HOUR, AMPM)) {
+now.tm_hour += 12;
+}
+
+} else {
+now.tm_hour = from_bcd(ARRAY_FIELD_EX32(s->nvram, HOUR, HOUR24));
+}
+now.tm_wday = from_bcd(s->nvram[R_WDAY]) - 1u;
+now.tm_mday = from_bcd(s->nvram[R_DATE] & 0x3f);
+now.tm_mon = from_bcd(s->nvram[R_MONTH] & 0x1f) - 1;
+now.tm_year = from_bcd(s->nvram[R_YEAR]) + 100;
+s->offset = qemu_timedate_diff();
+
+{
+/* Round trip to get real wday_offset based on time delta and
+ * ref. timezone.
+ * Race if midnight (in ref. timezone) happens here.
+ */
+int user_wday = now.tm_wday;
+qemu_get_timedate(, s->offset);
+
+s->wday_offset = (user_wday - now.tm_wday) % 7 + 1;
+}
+}
+
 static int ds1338_send(I2CSlave *i2c, uint8_t data)
 {
 DS1338State *s = DS1338(i2c);
@@ -170,52 +213,7 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 s->addr_byte = false;
 return 0;
 }
-if (s->ptr < 7) {
-/* Time register. */
-struct tm now;
-qemu_get_timedate(, s->offset);
-switch(s->ptr) {
-case R_SEC:
-/* TODO: Implement CH (stop) bit.  */
-now.tm_sec = from_bcd(data & 0x7f);
-break;
-case R_MIN:
-now.tm_min = from_bcd(data & 0x7f);
-break;
-case R_HOUR:
-if (FIELD_EX32(data, HOUR, SET12)) {
-/* 12 hour (1-12) */
-/* read and wrap 1-12 -> 0-11 */
-now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u;
-if (FIELD_EX32(data, HOUR, AMPM)) {
-now.tm_hour += 12;
-}
-
-} else {
-now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24));
-}
-break;
-case R_WDAY:
-{
-/* The day field is supposed to contain a value in
-   the range 1-7. Otherwise behavior is undefined.
- */
-int user_wday = (data & 7) - 1;
-s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
-}
-break;
-case R_DATE:
-now.tm_mday = from_bcd(data & 0x3f);
-break;
-case R_MONTH:
-now.tm_mon = from_bcd(data & 0x1f) - 1;
-break;
-case R_YEAR:
-now.tm_year = from_bcd(data) + 100;
-break;
-}
-s->offset = qemu_timedate_diff();
-} else if (s->ptr == R_CTRL) {
+if (s->ptr == R_CTRL) {
 /* Control register. */
 
 /* Ensure bits 2, 3 and 6 will read back as zero. */
@@ -227,6 +225,9 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 
 }
 s->nvram[s->ptr] = data;
+if (s->ptr <= R_YEAR) {
+ds1338_update(s);
+}
 inc_regptr(s);
 return 0;
 }
-- 
2.11.0

Re: [Qemu-devel] [PATCH 05/14] timer: ds1338 change write handling

[Peter Maydell]

On 24 March 2018 at 19:24, Michael Davidsaver  wrote:
> instead of a read-modify-write, do direct translation
> of device registers to struct tm members.
>
> This new ds1338_update() is the reverse of
> the existing capture_current_time().
>
> Simplifies later handling of CENTURY bit in
> similar Dallas RTC chips.
>
> Signed-off-by: Michael Davidsaver 
> ---
>  hw/timer/ds1338.c | 86 
> ++-
>  1 file changed, 40 insertions(+), 46 deletions(-)
>
> diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
> index 9bcda26e51..071c031563 100644
> --- a/hw/timer/ds1338.c
> +++ b/hw/timer/ds1338.c
> @@ -159,6 +159,42 @@ static int ds1338_recv(I2CSlave *i2c)
>  return res;
>  }
>
> +/* call after guest writes to current time registers
> + * to re-compute our offset from host time.
> + */
> +static void ds1338_update(DS1338State *s)
> +{
> +
> +struct tm now;
> +memset(, 0, sizeof(now));

You can zero initialize a struct with just
   struct tm now = {};

so you don't need the memset.

> +
> +/* TODO: Implement CH (stop) bit?  */
> +now.tm_sec = from_bcd(s->nvram[R_SEC] & 0x7f);
> +now.tm_min = from_bcd(s->nvram[R_MIN] & 0x7f);
> +if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) {
> +/* 12 hour (1-12) */
> +/* read and wrap 1-12 -> 0-11 */
> +now.tm_hour = from_bcd(ARRAY_FIELD_EX32(s->nvram, HOUR, HOUR12)) % 
> 12u;
> +if (ARRAY_FIELD_EX32(s->nvram, HOUR, AMPM)) {
> +now.tm_hour += 12;
> +}
> +
> +} else {
> +now.tm_hour = from_bcd(ARRAY_FIELD_EX32(s->nvram, HOUR, HOUR24));
> +}
> +{
> +/* The day field is supposed to contain a value in
> +   the range 1-7. Otherwise behavior is undefined.
> + */

This patch is a good opportunity to bring the format of this
comment in line with the others in the file, ie
  /* multiple lines
   * all have a star on the left
   */

> +int user_wday = (s->nvram[R_WDAY] & 7) - 1;

I would just declare user_wday at the top of the function, and
then you can drop the local {} scope.

> +s->wday_offset = (user_wday - now.tm_wday + 7) % 7;

This line copied from the previous code is still assuming that
'now' contains valid date information, so it won't produce the
right answer. What you need to do is
 (1) move this down to after you set s->offset,
 so that we have the new offset value, and then
 (2) call qemu_get_timedate(, s->offset);
 to get an updated 'now' struct with a valid now.tm_wday;
 (3) then you can do
s->wday_offset = (user_wday - now.tm_wday + 7) % 7;

(I think that's right, anyway -- you should check my working...)

> +}
> +now.tm_mday = from_bcd(s->nvram[R_DATE] & 0x3f);
> +now.tm_mon = from_bcd(s->nvram[R_MONTH] & 0x1f) - 1;
> +now.tm_year = from_bcd(s->nvram[R_YEAR]) + 100;
> +s->offset = qemu_timedate_diff();
> +

The rest of this looks good.

thanks
-- PMM

[Qemu-devel] [PATCH 05/14] timer: ds1338 change write handling

[Michael Davidsaver]

instead of a read-modify-write, do direct translation
of device registers to struct tm members.

This new ds1338_update() is the reverse of
the existing capture_current_time().

Simplifies later handling of CENTURY bit in
similar Dallas RTC chips.

Signed-off-by: Michael Davidsaver 
---
 hw/timer/ds1338.c | 86 ++-
 1 file changed, 40 insertions(+), 46 deletions(-)

diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
index 9bcda26e51..071c031563 100644
--- a/hw/timer/ds1338.c
+++ b/hw/timer/ds1338.c
@@ -159,6 +159,42 @@ static int ds1338_recv(I2CSlave *i2c)
 return res;
 }
 
+/* call after guest writes to current time registers
+ * to re-compute our offset from host time.
+ */
+static void ds1338_update(DS1338State *s)
+{
+
+struct tm now;
+memset(, 0, sizeof(now));
+
+/* TODO: Implement CH (stop) bit?  */
+now.tm_sec = from_bcd(s->nvram[R_SEC] & 0x7f);
+now.tm_min = from_bcd(s->nvram[R_MIN] & 0x7f);
+if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) {
+/* 12 hour (1-12) */
+/* read and wrap 1-12 -> 0-11 */
+now.tm_hour = from_bcd(ARRAY_FIELD_EX32(s->nvram, HOUR, HOUR12)) % 12u;
+if (ARRAY_FIELD_EX32(s->nvram, HOUR, AMPM)) {
+now.tm_hour += 12;
+}
+
+} else {
+now.tm_hour = from_bcd(ARRAY_FIELD_EX32(s->nvram, HOUR, HOUR24));
+}
+{
+/* The day field is supposed to contain a value in
+   the range 1-7. Otherwise behavior is undefined.
+ */
+int user_wday = (s->nvram[R_WDAY] & 7) - 1;
+s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
+}
+now.tm_mday = from_bcd(s->nvram[R_DATE] & 0x3f);
+now.tm_mon = from_bcd(s->nvram[R_MONTH] & 0x1f) - 1;
+now.tm_year = from_bcd(s->nvram[R_YEAR]) + 100;
+s->offset = qemu_timedate_diff();
+}
+
 static int ds1338_send(I2CSlave *i2c, uint8_t data)
 {
 DS1338State *s = DS1338(i2c);
@@ -168,52 +204,7 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 s->addr_byte = false;
 return 0;
 }
-if (s->ptr < 7) {
-/* Time register. */
-struct tm now;
-qemu_get_timedate(, s->offset);
-switch(s->ptr) {
-case R_SEC:
-/* TODO: Implement CH (stop) bit.  */
-now.tm_sec = from_bcd(data & 0x7f);
-break;
-case R_MIN:
-now.tm_min = from_bcd(data & 0x7f);
-break;
-case R_HOUR:
-if (FIELD_EX32(data, HOUR, SET12)) {
-/* 12 hour (1-12) */
-/* read and wrap 1-12 -> 0-11 */
-now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u;
-if (FIELD_EX32(data, HOUR, AMPM)) {
-now.tm_hour += 12;
-}
-
-} else {
-now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24));
-}
-break;
-case R_WDAY:
-{
-/* The day field is supposed to contain a value in
-   the range 1-7. Otherwise behavior is undefined.
- */
-int user_wday = (data & 7) - 1;
-s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
-}
-break;
-case R_DATE:
-now.tm_mday = from_bcd(data & 0x3f);
-break;
-case R_MONTH:
-now.tm_mon = from_bcd(data & 0x1f) - 1;
-break;
-case R_YEAR:
-now.tm_year = from_bcd(data) + 100;
-break;
-}
-s->offset = qemu_timedate_diff();
-} else if (s->ptr == R_CTRL) {
+if (s->ptr == R_CTRL) {
 /* Control register. */
 
 /* Ensure bits 2, 3 and 6 will read back as zero. */
@@ -225,6 +216,9 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 
 }
 s->nvram[s->ptr] = data;
+if (s->ptr <= R_YEAR) {
+ds1338_update(s);
+}
 inc_regptr(s);
 return 0;
 }
-- 
2.11.0