Re: [Qemu-devel] [PATCH 08/10] cuda: factor out timebase-derived counter value and load time

[Mark Cave-Ayland]

On 05/02/18 19:44, Philippe Mathieu-Daudé wrote:

> On 02/03/2018 07:37 AM, Mark Cave-Ayland wrote:
> > Commit b981289c49 "PPC: Cuda: Use cuda timer to expose tbfreq to guest" altered
> > the timer calculations from those based upon the hardware CUDA clock frequency
> > to those based upon the CPU timebase frequency.
> >
> > In fact we can isolate the differences to 2 simple changes: one to the counter
> > read value and another to the counter load time. Move these changes into
> > separate functions so the implementation can be swapped later.
> >
> > Signed-off-by: Mark Cave-Ayland <address@hidden>
> > ---
> >   hw/misc/macio/cuda.c | 25 ++++++++++++++++---------
> >   1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> > index 00e71fcd5e..184d151702 100644
> > --- a/hw/misc/macio/cuda.c
> > +++ b/hw/misc/macio/cuda.c
> > @@ -145,21 +145,29 @@ static void cuda_update_irq(CUDAState *s)
> >       }
> >   }
> >   
> > -static uint64_t get_tb(uint64_t time, uint64_t freq)
> > +static uint64_t get_counter_value(CUDAState *s, CUDATimer *ti)
> >   {
> > -    return muldiv64(time, freq, NANOSECONDS_PER_SECOND);
> > +    /* Reverse of the tb calculation algorithm that Mac OS X uses on bootup */
> > +    uint64_t tb_diff = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > +                                s->tb_frequency, NANOSECONDS_PER_SECOND) -
> > +                           ti->load_time;
>
> Easier to read imho:
>
>         uint64_t tb_diff = get_counter_load_time(s, ti) - ti->load_time;

Yes - I'm in two minds about this one, although I feel as if I should 
keep these functions completely separate since they form separate 
virtual methods in the device class (see patch 10). Does looking at that 
particular patch change your opinion at all?

> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>
> > +
> > +    return (tb_diff * 0xBF401675E5DULL) / (s->tb_frequency << 24);
> > +}
> > +
> > +static uint64_t get_counter_load_time(CUDAState *s, CUDATimer *ti)
> > +{
> > +    uint64_t load_time = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > +                                  s->tb_frequency, NANOSECONDS_PER_SECOND);
> > +    return load_time;
> >   }
> >   
> >   static unsigned int get_counter(CUDAState *s, CUDATimer *ti)
> >   {
> >       int64_t d;
> >       unsigned int counter;
> > -    uint64_t tb_diff;
> > -    uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> >   
> > -    /* Reverse of the tb calculation algorithm that Mac OS X uses on bootup. */
> > -    tb_diff = get_tb(current_time, s->tb_frequency) - ti->load_time;
> > -    d = (tb_diff * 0xBF401675E5DULL) / (s->tb_frequency << 24);
> > +    d = get_counter_value(s, ti);
> >   
> >       if (ti->index == 0) {
> >           /* the timer goes down from latch to -1 (period of latch + 2) */
> > @@ -178,8 +186,7 @@ static unsigned int get_counter(CUDAState *s, CUDATimer *ti)
> >   static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
> >   {
> >       CUDA_DPRINTF("T%d.counter=%d\n", 1 + ti->index, val);
> > -    ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > -                           s->tb_frequency);
> > +    ti->load_time = get_counter_load_time(s, ti);
> >       ti->counter_value = val;
> >       cuda_timer_update(s, ti, ti->load_time);
> >   }


ATB,

Mark.