Re: [Qemu-devel] [PATCH 03/14] timer: ds1338 persist 12-hour mode selection

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2997 bytes --]

On Thu, Jul 05, 2018 at 09:35:50PM -0700, Michael Davidsaver wrote:
> On 07/05/2018 08:39 PM, David Gibson wrote:
> > On Thu, Jul 05, 2018 at 11:19:50AM -0700, Michael Davidsaver wrote:
> > 11;rgb:ffff/ffff/ffff> Need to save HOUR[HOUR12] bit to keep
> >> track of guest selection of 12-hour mode.
> >> Write through current time registers to
> >> achieve this.  Will be overwritten
> >> by the next read/latch.
> >>
> >> This was only being done in two of three
> >> arms of this conditional block.
> >>
> >> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> > 
> > This looks dubious to me, or at least the explanation of it does.  The
> > other branch of the conditional is covering different registers in the
> > device, which are part of the RTC component, rather than the NVRAM
> > area.  I wouldn't necessarily expect them to persist data as a rule
> > the way the rest of the block does, even if this specific bit does
> > need to be preserved.
> 
> The fact that the above capture_current_time() included the line
> 
> >     if (s->nvram[2] & HOURS_12) {
> 
> was enough to convince me that the original author intended to persist
> the 12/24 hour mode in this way.  There are certainly other ways to
> accomplish this, but they would involved adding to the vmstate,
> which I've tried to avoid in this iteration.

Ah, yes, I see your point.  I was more unsure about whether it was
safe to also persist the other early bytes of the region, which this
patch also does.  But I can't really see how it would do any harm, so:

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> 
> 
> Also, I though I had test coverage of this bug.  That's actually how I
> noticed it to begin with.  But it seems my later change to allow for a
> slow test runner also stopped testing readback of the 12/24 hour mode bit.
> It just silently uses whichever it reads.  I'll be re-issuing an updated
> version which restores this check.  Then you will be able to easily
> see the effect of reverting 'timer: ds1338 persist 12-hour mode selection'.
> 
> 
> 
> >> ---
> >>  hw/timer/ds1338.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
> >> index 7298c5af43..b56db5852e 100644
> >> --- a/hw/timer/ds1338.c
> >> +++ b/hw/timer/ds1338.c
> >> @@ -220,10 +220,8 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
> >>             value unchanged. */
> >>          data = (data & ~CTRL_OSF) | (data & s->nvram[s->ptr] & CTRL_OSF);
> >>  
> >> -        s->nvram[s->ptr] = data;
> >> -    } else {
> >> -        s->nvram[s->ptr] = data;
> >>      }
> >> +    s->nvram[s->ptr] = data;
> >>      inc_regptr(s);
> >>      return 0;
> >>  }
> > 
> 
> 




-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]