Re: [Qemu-block] [PATCH 1/5] hw/block/pflash_cfi01: Removed an unused timer
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
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> 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
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(>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;
>>
>
Re: [Qemu-block] [PATCH 1/5] hw/block/pflash_cfi01: Removed an unused timer
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".
> 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
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> 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.
For this patch, with the commit references fixed:
Reviewed-by: Laszlo Ersek
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(>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;
>
Re: [Qemu-block] [PATCH 1/5] hw/block/pflash_cfi01: Removed an unused timer
On Sun, May 05, 2019 at 10:05:58PM +0200, 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
>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
>Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Wei Yang
>---
>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,
>2/ discuss on list whether or not use timings for the Virt flash.
>---
> 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(>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;
>--
>2.20.1
--
Wei Yang
Help you, Help me
