Re: [Qemu-devel] [PATCH 1/5] hw/block/pflash_cfi01: Removed an unused timer

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/6/19 4:39 PM, Laszlo Ersek wrote:
> On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
>> The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
>> timing modelled. One year later, the CFI02 model was introduced
>> (commit 05ee37ebf630) based on the CFI01 model. As noted in the
> 
> You got those commit references backwards, I believe:
> 
> * Commit 29133e9a0fff ("AMD NOR flash device support (initial patch by
> Jocelyn Mayer)", 2006-06-25) introduced "hw/pflash_cfi02.c".
> 
> * Commit 05ee37ebf630 ("Gumstix 'connex' board support by Thorsten
> Zitterell.", 2007-11-17) introduced "hw/pflash_cfi01.c".

Argh yes, thank you!

>> header, "It does not support timings". 12 years later, we never
>> had to model the device timings. Time to remove the unused timer,
>> we can still add it back if required.
>>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Yes, I plan to model those timings later. Actually I have a series
>> working, but I'd rather first
>> 1/ refactor common code between the both CFI implementations,
> 
> Good idea.
> 
>> 2/ discuss on list whether or not use timings for the Virt flash.
> 
> What would the timer buy us (specifically wrt. cfi01 / OVMF / ArmVirt)?
> 
> Being faithful to actual hardware is always good... except when it runs
> a significant risk of regressions. :) By that I don't mean "programming
> errors"; I mean that guest code would now have to conform to various
> timeouts, and that always makes me a bit concerned.

I'm glat you feel concerned :)
My goal is to model enough of the device to be able to run 'Capsule
Based Firmware Updates' [*], but I haven't investigated much yet.
Embedded firmware usually care about such timings. Anyway if this is
implemented as a feature, it would be disabled by default for the Virt
flash (I name the Virt flash the one used by the Virt X86/Aarch64 machines).

> 
> For this patch, with the commit references fixed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Will do, thanks!

> 
> Thanks,
> Laszlo
> 
>> ---
>>  hw/block/pflash_cfi01.c | 15 ---------------
>>  1 file changed, 15 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 16dfae14b80..6dc04f156a7 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -42,7 +42,6 @@
>>  #include "hw/block/flash.h"
>>  #include "sysemu/block-backend.h"
>>  #include "qapi/error.h"
>> -#include "qemu/timer.h"
>>  #include "qemu/bitops.h"
>>  #include "qemu/host-utils.h"
>>  #include "qemu/log.h"
>> @@ -86,7 +85,6 @@ struct PFlashCFI01 {
>>      uint8_t cfi_table[0x52];
>>      uint64_t counter;
>>      unsigned int writeblock_size;
>> -    QEMUTimer *timer;
>>      MemoryRegion mem;
>>      char *name;
>>      void *storage;
>> @@ -110,18 +108,6 @@ static const VMStateDescription vmstate_pflash = {
>>      }
>>  };
>>  
>> -static void pflash_timer (void *opaque)
>> -{
>> -    PFlashCFI01 *pfl = opaque;
>> -
>> -    trace_pflash_timer_expired(pfl->cmd);
>> -    /* Reset flash */
>> -    pfl->status ^= 0x80;
>> -    memory_region_rom_device_set_romd(&pfl->mem, true);
>> -    pfl->wcycle = 0;
>> -    pfl->cmd = 0;
>> -}
>> -
>>  /* Perform a CFI query based on the bank width of the flash.
>>   * If this code is called we know we have a device_width set for
>>   * this flash.
>> @@ -771,7 +757,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>          pfl->max_device_width = pfl->device_width;
>>      }
>>  
>> -    pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>>      pfl->wcycle = 0;
>>      pfl->cmd = 0;
>>      pfl->status = 0;
>>
>