Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] pflash: Macro PFLASH_BUG() is used just once, expand

[Laszlo Ersek]

On 02/21/19 17:39, Philippe Mathieu-Daudé wrote:
> On 2/21/19 1:46 PM, Laszlo Ersek wrote:
>> On 02/21/19 13:38, Peter Maydell wrote:
>>> On Thu, 21 Feb 2019 at 12:07, Laszlo Ersek <address@hidden> wrote:
>>>> since we're talking "reset_flash", I'll note that there is no actual
>>>> reset handler for cfi.pflash01. I found out recently, via:
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>>>
>>> Yes; this isn't uncommon for some of the really old
>>> device models. It should definitely have one added.
>>>
>>> You are correct also that the timer in the pflash_cfi01
>>> model is dead code -- it has always been so, since the
>>> device was added in 2007. The reason it is there is that
>>> pflash_cfi01 was created as a copy-and-hack of the
>>> cfi02 device. In cfi02 we do use the timer, as a way
>>> of simulating "make full-chip and sector erases take a
>>> guest-visible amount of time rather than completing
>>> instantaneously". cfi01 doesn't do that (and I think
>>> may not implement anything other than block erase),
> 
> This time is flash-device specific and is currently hardcoded.
> 
> The guest learn from the CFI table how long it should wait
> before polling/accessing the flash, or take measures in case
> of timeout.
> 
> We set these values in _realize():
> 
>     ...
>     /* Typical timeout for block erase (512 ms) */
>     pfl->cfi_table[0x21] = 0x09;
>     /* Typical timeout for full chip erase (4096 ms) */
>     pfl->cfi_table[0x22] = 0x0C;
>     ...
>     /* Max timeout for block erase */
>     pfl->cfi_table[0x25] = 0x0A;
>     /* Max timeout for chip erase */
>     pfl->cfi_table[0x26] = 0x0D;
> 
> The timer is triggered in _write(), where we use other hardcoded
> values (which are luckily in range with the CFI announced ones):
> 
>         /* Let's wait 5 seconds before chip erase is done */
>         timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                   (NANOSECONDS_PER_SECOND * 5));
> 
>         /* Let's wait 1/2 second before sector erase is done */
>         timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                   (NANOSECONDS_PER_SECOND / 2));
> 
> When emulating embedded devices, it is very desirable to have those
> timers working, and I'd prefer we fix that on CFI01 rather than simply
> removing the unused timer.
> 
> Now for the case of "Virt" machines, it is probably pointless to add
> flash delay and we should remove the timer use.

If the timer logic can be completed in such a way that existent OVMF
code sees no change at all, on the machine types that it currently runs
on (that is, on *unversioned* i440fx and q35), I'm OK with the idea.

Thanks,
Laszlo

> 
>>> but the timer initialization code was left in rather
>>> than being deleted as part of the copy-and-hack.
>>
>> Thank you, I've linked this back into the RHBZ.
>>
>> Laszlo
>>