Re: [Qemu-arm] [PATCH v2 10/21] aspeed/timer: Provide back-pressure information for short periods

[Cédric Le Goater]

On 01/07/2019 14:59, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 17:54, Cédric Le Goater <address@hidden> wrote:
>>
>> From: Andrew Jeffery <address@hidden>
>>
>> First up: This is not the way the hardware behaves.
>>
>> However, it helps resolve real-world problems with short periods being
>> used under Linux. Commit 4451d3f59f2a ("clocksource/drivers/fttmr010:
>> Fix set_next_event handler") in Linux fixed the timer driver to
>> correctly schedule the next event for the Aspeed controller, and in
>> combination with 5daa8212c08e ("ARM: dts: aspeed: Describe random number
>> device") Linux will now set a timer with a period as low as 1us.
>>
>> Configuring a qemu timer with such a short period results in spending
>> time handling the interrupt in the model rather than executing guest
>> code, leading to noticeable "sticky" behaviour in the guest.
>>
>> The behaviour of Linux is correct with respect to the hardware, so we
>> need to improve our handling under emulation. The approach chosen is to
>> provide back-pressure information by calculating an acceptable minimum
>> number of ticks to be set on the model. Under Linux an additional read
>> is added in the timer configuration path to detect back-pressure, which
>> will never occur on hardware. However if back-pressure is observed, the
>> driver alerts the clock event subsystem, which then performs its own
>> next event dilation via a config option - d1748302f70b ("clockevents:
>> Make minimum delay adjustments configurable")
>>
>> A minimum period of 5us was experimentally determined on a Lenovo
>> T480s, which I've increased to 20us for "safety".
>>
>> Signed-off-by: Andrew Jeffery <address@hidden>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
>> --- a/hw/misc/aspeed_scu.c
>> +++ b/hw/misc/aspeed_scu.c
>> @@ -423,6 +423,12 @@ static void aspeed_scu_realize(DeviceState *dev, Error 
>> **errp)
>>                            TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);
>>
>>      sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    /*
>> +     * Reset on realize to ensure the APB clock value is calculated in time 
>> for
>> +     * use by the timer model, which is reset before the SCU.
>> +     */
>> +    aspeed_scu_reset(dev);
> 
> This looks wrong. QEMU should always be resetting devices
> after realize and before actually running anything.

Ah yes ... We need to rework SCU with class data to provide a cleaner 
model for all SoCs. AST2600 will require it anyhow. 

Thanks,

C. 
> 
>>  }
> 
> thanks
> -- PMM
>