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 <ler...@redhat.com> 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 >>
