[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 10/21] aspeed/timer: Provide back-pressure in
Cédric Le Goater
Re: [Qemu-devel] [PATCH v2 10/21] aspeed/timer: Provide back-pressure information for short periods
Mon, 1 Jul 2019 15:38:14 +0200
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2
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
>> 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
>> + * 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.
> -- PMM