Re: [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 24/08/2021 11:09, Finn Thain wrote:

> This code appears to be unnecessary.
> 
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
>   hw/misc/mos6522.c | 22 +---------------------
>   1 file changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 1c57332b40..a478c1ca43 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
>       }
>   }
>   
> -static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti)
> -{
> -    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
> -
> -    if (ti->index == 0) {
> -        return mdc->get_timer1_load_time(s, ti);
> -    } else {
> -        return mdc->get_timer2_load_time(s, ti);
> -    }
> -}
> -
>   static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
>   {
>       int64_t d;
> @@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
>   static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
>   {
>       trace_mos6522_set_counter(1 + ti->index, val);
> -    ti->load_time = get_load_time(s, ti);
> +    ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>       ti->counter_value = val;
>       if (ti->index == 0) {
>           mos6522_timer1_update(s, ti, ti->load_time);
> @@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
>                       ti->frequency, NANOSECONDS_PER_SECOND);
>   }
>   
> -static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti)
> -{
> -    uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -
> -    return load_time;
> -}
> -
>   static void mos6522_portA_write(MOS6522State *s)
>   {
>       qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
> @@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>       mdc->update_irq = mos6522_update_irq;
>       mdc->get_timer1_counter_value = mos6522_get_counter_value;
>       mdc->get_timer2_counter_value = mos6522_get_counter_value;
> -    mdc->get_timer1_load_time = mos6522_get_load_time;
> -    mdc->get_timer2_load_time = mos6522_get_load_time;
>   }
>   
>   static const TypeInfo mos6522_type_info = {

Both the get_counter_value() and get_load_time() callbacks are used as part of the 
CUDA emulation in hw/misc/macio/cuda.c as per the comment:

/* MacOS uses timer 1 for calibration on startup, so we use
  * the timebase frequency and cuda_get_counter_value() with
  * cuda_get_load_time() to steer MacOS to calculate calibrate its timers
  * correctly for both TCG and KVM (see commit b981289c49 "PPC: Cuda: Use cuda
  * timer to expose tbfreq to guest" for more information) */

Certainly for the 6522 device it is worth configuring with --target-list="ppc-softmmu 
m68k-softmmu" to make sure that you don't inadvertently break anything in the PPC world.

A bit of history here: the original mos6522.c was extracted from hw/misc/macio/cuda.c 
when Laurent presented his initial q800 patches since they also had their own 
implementation of the 6522, and it was better to move the implementation into a 
separate QEMU device so that the logic could be shared.

The Darwin kernel timer calibration loop is quite hard to get right: see 
https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed_asm.s.auto.html 
and 
https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed.c.auto.html. 
Ben/Alex came up with the current mechanism to fool the calibration routine, and I 
simply added in those callbacks to allow it to be implemented as part of the 
now-generic 6522 device.


ATB,

Mark.