[Qemu-devel] [PATCH] hw/pflash_cfi02: Fix lazy reset of ROMD mode

[Qemu-devel] [PATCH] hw/pflash_cfi02: Fix lazy reset of ROMD mode

[jordan.l.justen]

From: Jordan Justen <jordan.l.justen@intel.com>

When checking pfl->rom_mode for when to lazily reenter ROMD mode,
the value was check was the opposite of what it should have been.
This prevent the part from returning to ROMD mode after a write
was made to the CFI rom region.
---
 hw/pflash_cfi02.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 3594a36..a936cdb 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 
     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (pfl->rom_mode) {
+    if (!pfl->rom_mode) {
         /* Lazy reset of to ROMD mode */
         if (pfl->wcycle == 0)
             pflash_register_memory(pfl, 1);
-- 
1.7.1

Re: [Qemu-devel] [PATCH] hw/pflash_cfi02: Fix lazy reset of ROMD mode

[Aurelien Jarno]

On Mon, Mar 14, 2011 at 02:10:58PM -0700, jordan.l.justen@intel.com wrote:
> From: Jordan Justen <jordan.l.justen@intel.com>
> 
> When checking pfl->rom_mode for when to lazily reenter ROMD mode,
> the value was check was the opposite of what it should have been.
> This prevent the part from returning to ROMD mode after a write
> was made to the CFI rom region.
> ---
>  hw/pflash_cfi02.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index 3594a36..a936cdb 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
>  
>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>      ret = -1;
> -    if (pfl->rom_mode) {
> +    if (!pfl->rom_mode) {
>          /* Lazy reset of to ROMD mode */
>          if (pfl->wcycle == 0)
>              pflash_register_memory(pfl, 1);

The patch looks correct, but is missing a Signed-off-by: line. Can you
please resend it?

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH] hw/pflash_cfi02: Fix lazy reset of ROMD mode

[Jordan Justen]

When checking pfl->rom_mode for when to lazily reenter ROMD mode,
the value was check was the opposite of what it should have been.
This prevent the part from returning to ROMD mode after a write
was made to the CFI rom region.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 hw/pflash_cfi02.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 30c8aa4..370c5ee 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 
     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (pfl->rom_mode) {
+    if (!pfl->rom_mode) {
         /* Lazy reset of to ROMD mode */
         if (pfl->wcycle == 0)
             pflash_register_memory(pfl, 1);
-- 
1.7.1

Re: [Qemu-devel] [PATCH] hw/pflash_cfi02: Fix lazy reset of ROMD mode

[Aurelien Jarno]

On Sun, Apr 03, 2011 at 01:16:26PM -0700, Jordan Justen wrote:
> When checking pfl->rom_mode for when to lazily reenter ROMD mode,
> the value was check was the opposite of what it should have been.
> This prevent the part from returning to ROMD mode after a write
> was made to the CFI rom region.
> 
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>  hw/pflash_cfi02.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Thanks, applied.

> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index 30c8aa4..370c5ee 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
>  
>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>      ret = -1;
> -    if (pfl->rom_mode) {
> +    if (!pfl->rom_mode) {
>          /* Lazy reset of to ROMD mode */
>          if (pfl->wcycle == 0)
>              pflash_register_memory(pfl, 1);
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] Re: [PATCH] hw/pflash_cfi02: Fix lazy reset of ROMD mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

On 2011-04-03 22:16, Jordan Justen wrote:
> When checking pfl->rom_mode for when to lazily reenter ROMD mode,
> the value was check was the opposite of what it should have been.
> This prevent the part from returning to ROMD mode after a write
> was made to the CFI rom region.
> 
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>  hw/pflash_cfi02.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index 30c8aa4..370c5ee 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
>  
>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>      ret = -1;
> -    if (pfl->rom_mode) {
> +    if (!pfl->rom_mode) {
>          /* Lazy reset of to ROMD mode */
>          if (pfl->wcycle == 0)
>              pflash_register_memory(pfl, 1);

Indeed, that block looks weird to its author today as well. But
inverting the logic completely defeats the purpose of lazy mode
switching (will likely file a patch to remove the block). We should
instead switch back using the timer.

So the question is: Did you actually see a problem that the flash was
stuck in write mode, or did you just stumble over this strange code? In
the former case, please explain the sequence or provide a trace.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH] pflash: Restore & fix lazy ROMD switching

[Jan Kiszka]

On 2011-04-10 10:38, Jan Kiszka wrote:
> On 2011-04-03 22:16, Jordan Justen wrote:
>> When checking pfl->rom_mode for when to lazily reenter ROMD mode,
>> the value was check was the opposite of what it should have been.
>> This prevent the part from returning to ROMD mode after a write
>> was made to the CFI rom region.
>>
>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>> ---
>>  hw/pflash_cfi02.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>> index 30c8aa4..370c5ee 100644
>> --- a/hw/pflash_cfi02.c
>> +++ b/hw/pflash_cfi02.c
>> @@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
>>  
>>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>>      ret = -1;
>> -    if (pfl->rom_mode) {
>> +    if (!pfl->rom_mode) {
>>          /* Lazy reset of to ROMD mode */
>>          if (pfl->wcycle == 0)
>>              pflash_register_memory(pfl, 1);
> 
> Indeed, that block looks weird to its author today as well. But
> inverting the logic completely defeats the purpose of lazy mode
> switching (will likely file a patch to remove the block). We should
> instead switch back using the timer.

That was wishful thinking. We were actually lacking a switch-back on
read, something that the old twisted logic was papering over. Patch
below should resolve that more gracefully, at least it does so here.

Jan

------8<------

Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but
resolved it by breaking that feature. This approach addresses the issue
by switching back to ROMD after a certain amount of read accesses
without further unlock sequences.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 hw/pflash_cfi02.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 370c5ee..14bbc34 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -50,6 +50,8 @@ do {                                               \
 #define DPRINTF(fmt, ...) do { } while (0)
 #endif
 
+#define PFLASH_LAZY_ROMD_THRESHOLD 42
+
 struct pflash_t {
     BlockDriverState *bs;
     target_phys_addr_t base;
@@ -70,6 +72,7 @@ struct pflash_t {
     ram_addr_t off;
     int fl_mem;
     int rom_mode;
+    int read_counter; /* used for lazy switch-back to rom mode */
     void *storage;
 };
 
@@ -112,10 +115,10 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 
     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (!pfl->rom_mode) {
-        /* Lazy reset of to ROMD mode */
-        if (pfl->wcycle == 0)
-            pflash_register_memory(pfl, 1);
+    /* Lazy reset to ROMD mode after a certain amount of read accesses */
+    if (!pfl->rom_mode && pfl->wcycle == 0 &&
+        ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
+        pflash_register_memory(pfl, 1);
     }
     offset &= pfl->chip_len - 1;
     boff = offset & 0xFF;
@@ -254,6 +257,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
         /* Set the device in I/O access mode if required */
         if (pfl->rom_mode)
             pflash_register_memory(pfl, 0);
+        pfl->read_counter = 0;
         /* We're in read mode */
     check_unlock0:
         if (boff == 0x55 && cmd == 0x98) {
-- 
1.7.1

Re: [Qemu-devel] Re: [PATCH] hw/pflash_cfi02: Fix lazy reset of ROMD mode

[Jordan Justen]

On Sun, Apr 10, 2011 at 01:38, Jan Kiszka <jan.kiszka@web.de> wrote:
> Indeed, that block looks weird to its author today as well. But
> inverting the logic completely defeats the purpose of lazy mode
> switching (will likely file a patch to remove the block).

Looking at the 2nd parameter to the call, and the
pflash_register_memory code, it seems it only makes sense with the
!pfl->rom_mode.

I thought that the goal was to allow multiple write operations before
exiting to rom mode, but that a read will return it to rom mode.  In
that case, this change seemed to fix it.

> We should
> instead switch back using the timer.
>
> So the question is: Did you actually see a problem that the flash was
> stuck in write mode, or did you just stumble over this strange code? In
> the former case, please explain the sequence or provide a trace.

I had enabled the debug trace messages and was probing the flash from
the efi shell on x86-64.  I found that I would always see the debug
messages for reads, after I started writing to the flash.

This change allowed the flash to go back to a rom mode where the debug
prints would then disappear for subsequent reads.

-Jordan

Re: [Qemu-devel] [PATCH] pflash: Restore & fix lazy ROMD switching

[Jordan Justen]

On Sun, Apr 10, 2011 at 03:53, Jan Kiszka <jan.kiszka@web.de> wrote:
> Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but
> resolved it by breaking that feature. This approach addresses the issue
> by switching back to ROMD after a certain amount of read accesses
> without further unlock sequences.

Without this change, the code will stay in flash mode until a single
read occurs.  The code sequence you are wanting to support using will
issue a read before trying to unlock again?

Actually, I suppose it will want to verify the written data before
moving on, so this does make sense.

Is the overhead of switching the modes significant enough to justify
the lazy switch-back?  (If so, maybe I'll look at this for cfi01 too.)

Thanks,

-Jordan

Re: [Qemu-devel] [PATCH] pflash: Restore & fix lazy ROMD switching

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

On 2011-04-10 21:33, Jordan Justen wrote:
> On Sun, Apr 10, 2011 at 03:53, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but
>> resolved it by breaking that feature. This approach addresses the issue
>> by switching back to ROMD after a certain amount of read accesses
>> without further unlock sequences.
> 
> Without this change, the code will stay in flash mode until a single
> read occurs.  The code sequence you are wanting to support using will
> issue a read before trying to unlock again?
> 
> Actually, I suppose it will want to verify the written data before
> moving on, so this does make sense.

Precisely. The drivers (e.g. Linux) compare two successive reads for bit
flips. In their absence, the result was written. The guest I'm
optimizing for does 5 reads per write.

> 
> Is the overhead of switching the modes significant enough to justify
> the lazy switch-back?  (If so, maybe I'll look at this for cfi01 too.)

As we call into cpu_register_physical_memory, the overhead is
tremendous, an order of magnitude IIRC, which will sum up to huge delays
when reflashing a complete multi-MB firmware image e.g.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH] pflash: Restore & fix lazy ROMD switching

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3715 bytes --]

On 2011-04-10 12:53, Jan Kiszka wrote:
> On 2011-04-10 10:38, Jan Kiszka wrote:
>> On 2011-04-03 22:16, Jordan Justen wrote:
>>> When checking pfl->rom_mode for when to lazily reenter ROMD mode,
>>> the value was check was the opposite of what it should have been.
>>> This prevent the part from returning to ROMD mode after a write
>>> was made to the CFI rom region.
>>>
>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>> ---
>>>  hw/pflash_cfi02.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>>> index 30c8aa4..370c5ee 100644
>>> --- a/hw/pflash_cfi02.c
>>> +++ b/hw/pflash_cfi02.c
>>> @@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
>>>  
>>>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>>>      ret = -1;
>>> -    if (pfl->rom_mode) {
>>> +    if (!pfl->rom_mode) {
>>>          /* Lazy reset of to ROMD mode */
>>>          if (pfl->wcycle == 0)
>>>              pflash_register_memory(pfl, 1);
>>
>> Indeed, that block looks weird to its author today as well. But
>> inverting the logic completely defeats the purpose of lazy mode
>> switching (will likely file a patch to remove the block). We should
>> instead switch back using the timer.
> 
> That was wishful thinking. We were actually lacking a switch-back on
> read, something that the old twisted logic was papering over. Patch
> below should resolve that more gracefully, at least it does so here.
> 
> Jan
> 
> ------8<------
> 
> Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but
> resolved it by breaking that feature. This approach addresses the issue
> by switching back to ROMD after a certain amount of read accesses
> without further unlock sequences.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> ---
>  hw/pflash_cfi02.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index 370c5ee..14bbc34 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -50,6 +50,8 @@ do {                                               \
>  #define DPRINTF(fmt, ...) do { } while (0)
>  #endif
>  
> +#define PFLASH_LAZY_ROMD_THRESHOLD 42
> +
>  struct pflash_t {
>      BlockDriverState *bs;
>      target_phys_addr_t base;
> @@ -70,6 +72,7 @@ struct pflash_t {
>      ram_addr_t off;
>      int fl_mem;
>      int rom_mode;
> +    int read_counter; /* used for lazy switch-back to rom mode */
>      void *storage;
>  };
>  
> @@ -112,10 +115,10 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
>  
>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>      ret = -1;
> -    if (!pfl->rom_mode) {
> -        /* Lazy reset of to ROMD mode */
> -        if (pfl->wcycle == 0)
> -            pflash_register_memory(pfl, 1);
> +    /* Lazy reset to ROMD mode after a certain amount of read accesses */
> +    if (!pfl->rom_mode && pfl->wcycle == 0 &&
> +        ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
> +        pflash_register_memory(pfl, 1);
>      }
>      offset &= pfl->chip_len - 1;
>      boff = offset & 0xFF;
> @@ -254,6 +257,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>          /* Set the device in I/O access mode if required */
>          if (pfl->rom_mode)
>              pflash_register_memory(pfl, 0);
> +        pfl->read_counter = 0;
>          /* We're in read mode */
>      check_unlock0:
>          if (boff == 0x55 && cmd == 0x98) {

Please apply unless concerns remain.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH] pflash: Restore & fix lazy ROMD switching

[Aurelien Jarno]

On Tue, Apr 26, 2011 at 09:42:04AM +0200, Jan Kiszka wrote:
> On 2011-04-10 12:53, Jan Kiszka wrote:
> > On 2011-04-10 10:38, Jan Kiszka wrote:
> >> On 2011-04-03 22:16, Jordan Justen wrote:
> >>> When checking pfl->rom_mode for when to lazily reenter ROMD mode,
> >>> the value was check was the opposite of what it should have been.
> >>> This prevent the part from returning to ROMD mode after a write
> >>> was made to the CFI rom region.
> >>>
> >>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> >>> ---
> >>>  hw/pflash_cfi02.c |    2 +-
> >>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> >>> index 30c8aa4..370c5ee 100644
> >>> --- a/hw/pflash_cfi02.c
> >>> +++ b/hw/pflash_cfi02.c
> >>> @@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
> >>>  
> >>>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
> >>>      ret = -1;
> >>> -    if (pfl->rom_mode) {
> >>> +    if (!pfl->rom_mode) {
> >>>          /* Lazy reset of to ROMD mode */
> >>>          if (pfl->wcycle == 0)
> >>>              pflash_register_memory(pfl, 1);
> >>
> >> Indeed, that block looks weird to its author today as well. But
> >> inverting the logic completely defeats the purpose of lazy mode
> >> switching (will likely file a patch to remove the block). We should
> >> instead switch back using the timer.
> > 
> > That was wishful thinking. We were actually lacking a switch-back on
> > read, something that the old twisted logic was papering over. Patch
> > below should resolve that more gracefully, at least it does so here.
> > 
> > Jan
> > 
> > ------8<------
> > 
> > Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but
> > resolved it by breaking that feature. This approach addresses the issue
> > by switching back to ROMD after a certain amount of read accesses
> > without further unlock sequences.
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> > ---
> >  hw/pflash_cfi02.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> > index 370c5ee..14bbc34 100644
> > --- a/hw/pflash_cfi02.c
> > +++ b/hw/pflash_cfi02.c
> > @@ -50,6 +50,8 @@ do {                                               \
> >  #define DPRINTF(fmt, ...) do { } while (0)
> >  #endif
> >  
> > +#define PFLASH_LAZY_ROMD_THRESHOLD 42
> > +
> >  struct pflash_t {
> >      BlockDriverState *bs;
> >      target_phys_addr_t base;
> > @@ -70,6 +72,7 @@ struct pflash_t {
> >      ram_addr_t off;
> >      int fl_mem;
> >      int rom_mode;
> > +    int read_counter; /* used for lazy switch-back to rom mode */
> >      void *storage;
> >  };
> >  
> > @@ -112,10 +115,10 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
> >  
> >      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
> >      ret = -1;
> > -    if (!pfl->rom_mode) {
> > -        /* Lazy reset of to ROMD mode */
> > -        if (pfl->wcycle == 0)
> > -            pflash_register_memory(pfl, 1);
> > +    /* Lazy reset to ROMD mode after a certain amount of read accesses */
> > +    if (!pfl->rom_mode && pfl->wcycle == 0 &&
> > +        ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
> > +        pflash_register_memory(pfl, 1);
> >      }
> >      offset &= pfl->chip_len - 1;
> >      boff = offset & 0xFF;
> > @@ -254,6 +257,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
> >          /* Set the device in I/O access mode if required */
> >          if (pfl->rom_mode)
> >              pflash_register_memory(pfl, 0);
> > +        pfl->read_counter = 0;
> >          /* We're in read mode */
> >      check_unlock0:
> >          if (boff == 0x55 && cmd == 0x98) {
> 
> Please apply unless concerns remain.
> 

Done.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net