Re: [PATCH 14/21] mac_via: work around underflow in TimeDBRA timing loop in SETUPTIMEK

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

On 06/07/2023 11:10, Philippe Mathieu-Daudé wrote:

> On 5/7/23 21:49, Mark Cave-Ayland wrote:
>> On 03/07/2023 09:30, Philippe Mathieu-Daudé wrote:
>>
>>> On 2/7/23 17:48, Mark Cave-Ayland wrote:
>>>> The MacOS toolbox ROM calculates the number of branches that can be executed
>>>> per millisecond as part of its timer calibration. Since modern hosts are
>>>> considerably quicker than original hardware, the negative counter reaches zero
>>>> before the calibration completes leading to division by zero later in
>>>> CALCULATESLOD.
>>>>
>>>> Instead of trying to fudge the timing loop (which won't work for TimeDBRA/TimeSCCDB
>>>> anyhow), use the pattern of access to the VIA1 registers to detect when SETUPTIMEK
>>>> has finished executing and write some well-known good timer values to TimeDBRA
>>>> and TimeSCCDB taken from real hardware with a suitable scaling factor.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>   hw/misc/mac_via.c         | 115 ++++++++++++++++++++++++++++++++++++++
>>>>   hw/misc/trace-events      |   1 +
>>>>   include/hw/misc/mac_via.h |   3 +
>>>>   3 files changed, 119 insertions(+)
>>>>
>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>> index baeb73eeb3..766a32a95d 100644
>>>> --- a/hw/misc/mac_via.c
>>>> +++ b/hw/misc/mac_via.c
>>>> @@ -16,6 +16,7 @@
>>>>    */
>>>>   #include "qemu/osdep.h"
>>>> +#include "exec/address-spaces.h"
>>>>   #include "migration/vmstate.h"
>>>>   #include "hw/sysbus.h"
>>>>   #include "hw/irq.h"
>>>
>>>
>>>> +/*
>>>> + * Addresses and real values for TimeDBRA/TimeSCCB to allow timer calibration
>>>> + * to succeed (NOTE: both values have been multiplied by 3 to cope with the
>>>> + * speed of QEMU execution on a modern host
>>>> + */
>>>> +#define MACOS_TIMEDBRA        0xd00
>>>> +#define MACOS_TIMESCCB        0xd02
>>>> +
>>>> +#define MACOS_TIMEDBRA_VALUE  (0x2a00 * 3)
>>>> +#define MACOS_TIMESCCB_VALUE  (0x079d * 3)
>>>> +
>>>> +static bool via1_is_toolbox_timer_calibrated(void)
>>>> +{
>>>> +    /*
>>>> +     * Indicate whether the MacOS toolbox has been calibrated by checking
>>>> +     * for the value of our magic constants
>>>> +     */
>>>> +    uint16_t timedbra = lduw_be_phys(&address_space_memory, MACOS_TIMEDBRA);
>>>> +    uint16_t timesccdb = lduw_be_phys(&address_space_memory, MACOS_TIMESCCB);
>>>
>>> Rather than using the global address_space_memory (which we secretly
>>> try to remove entirely), could we pass a MemoryRegion link property
>>> to the VIA1 device?
>>
>> Hmmm good question. It seems to me that we're dispatching a write to the default 
>> address space (which includes all RAM and MMIO etc.) rather than a particular 
>> MemoryRegion so it feels as if AddressSpace is the right approach here. 
>> Unfortunately since AddressSpace is not a QOM type then it isn't possible to pass 
>> it as a link property.
> 
> We pass a MR link.

That could work, but then we'd have to grab the host pointer via 
memory_region_get_ram_ptr() first and switch to using lduw_be_p(). And if we did have 
an MR property then what should it be set to? The obvious candidate would be 
get_system_memory() but should we also be avoiding that for similar reasons? If we 
did use it, then we might as well use get_system_memory() directly rather than having 
to pass in a separate link property.

>> There are existing examples in qtest that use first_cpu->as which seems a better 
>> option unless we want to move away from using first_cpu in the codebase?
> 
> See:
> 
>    $ git grep -E "(PROP.*LINK.*MEMORY_REGION|dma-mr)"
> 
> Anyway I don't object to your patch, we can rework this
> &address_space_memory use later when we'll have discussed the whole
> design.

Agreed. It feels like there are still some discussions needed to understand exactly 
what needs to be done in order to move forward with this (maybe start a separate 
thread?).


ATB,

Mark.