[Qemu-devel] Commit 9c9bb6c89d4 breaks code execution from flash

[Qemu-devel] Commit 9c9bb6c89d4 breaks code execution from flash

[Michael Walle]

Hi Jan,

your commit "Optimize consecutive CFI02 writes by remapping memory lazily" 
breaks the code execution from flash.

If you write to the flash, the flash will switch into I/O mode. Now if code is 
executed from this flash, a cpu_abort will be raised ("Trying to execute code 
outside RAM or ROM").


-- 
wkr michael

---
Don't cry because it is over, smile because it happened.

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Jan Kiszka]

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

Michael Walle wrote:
> Hi Jan,
> 
> your commit "Optimize consecutive CFI02 writes by remapping memory lazily" 
> breaks the code execution from flash.
> 
> If you write to the flash, the flash will switch into I/O mode. Now if code is 
> executed from this flash, a cpu_abort will be raised ("Trying to execute code 
> outside RAM or ROM").
> 

Hmm, guess I didn't test execute-in-place back then. Do you happen to
have a test case for this scenario? I'll look into this.

Jan


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

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Michael Walle]

[sorry didn't see the CC to the mailinglist]

Am Friday 23 April 2010 09:23:49 schrieb Jan Kiszka:
> Michael Walle wrote:
> > Hi Jan,
> >
> > your commit "Optimize consecutive CFI02 writes by remapping memory
> > lazily" breaks the code execution from flash.
> >
> > If you write to the flash, the flash will switch into I/O mode. Now if
> > code is executed from this flash, a cpu_abort will be raised ("Trying to
> > execute code outside RAM or ROM").
>
> Hmm, guess I didn't test execute-in-place back then. Do you happen to
> have a test case for this scenario? I'll look into this.
Only for my qemu-lm32 port.. But reading the flash id, while executing this 
code from flash should trigger the bug.

-- 
michael

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Jan Kiszka]

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

Michael Walle wrote:
> [sorry didn't see the CC to the mailinglist]
> 
> Am Friday 23 April 2010 09:23:49 schrieb Jan Kiszka:
>> Michael Walle wrote:
>>> Hi Jan,
>>>
>>> your commit "Optimize consecutive CFI02 writes by remapping memory
>>> lazily" breaks the code execution from flash.
>>>
>>> If you write to the flash, the flash will switch into I/O mode. Now if
>>> code is executed from this flash, a cpu_abort will be raised ("Trying to
>>> execute code outside RAM or ROM").
>> Hmm, guess I didn't test execute-in-place back then. Do you happen to
>> have a test case for this scenario? I'll look into this.
> Only for my qemu-lm32 port.. But reading the flash id, while executing this 
> code from flash should trigger the bug.
> 

OK, that was a hard nut. After various dead ends, I think I found an
possible solution. Can you give this a try?

diff --git a/exec-all.h b/exec-all.h
index 1016de2..b070da9 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -329,6 +329,10 @@ static inline tb_page_addr_t
get_page_addr_code(CPUState *env1, target_ulong add
     if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code !=
                  (addr & TARGET_PAGE_MASK))) {
         ldub_code(addr);
+        if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code &
+                     TLB_INVALID_MASK)) {
+            ldub_code(addr);
+        }
     }
     pd = env1->tlb_table[mmu_idx][page_index].addr_code &
~TARGET_PAGE_MASK;
     if (pd > IO_MEM_ROM && !(pd & IO_MEM_ROMD)) {
diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index f3d3f41..201e410 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -40,7 +40,7 @@
 #include "qemu-timer.h"
 #include "block.h"

-//#define PFLASH_DEBUG
+#define PFLASH_DEBUG
 #ifdef PFLASH_DEBUG
 #define DPRINTF(fmt, ...)                          \
 do {                                               \
@@ -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);
@@ -185,7 +185,7 @@ static uint32_t pflash_read (pflash_t *pfl,
target_phys_addr_t offset,
         default:
             goto flash_read;
         }
-        DPRINTF("%s: ID " TARGET_FMT_pld " %x\n", __func__, boff, ret);
+        DPRINTF("%s: ID " TARGET_FMT_plx " %x\n", __func__, boff, ret);
         break;
     case 0xA0:
     case 0x10:


Still requires proper patch split up, and I need to think about possible
side effects.

Jan


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

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Michael Walle]

Am Wednesday 12 May 2010 09:56:31 schrieb Jan Kiszka:
> OK, that was a hard nut. After various dead ends, I think I found an
> possible solution. Can you give this a try?
[..]
> Still requires proper patch split up, and I need to think about possible
> side effects.
Thanks, the patch is working.

But i noticed another minor bug. The cfi02 doesn't handle 'read flash id' on 
16bit accesses correctly. It always returns 8 bit. I used something like

if (width == 2)
    ret = pfl->ident[0] << 8 | pfl->ident[1];  /* rsp. ident[1]/ident[2] */

within the 0x90 reading as a quick workaround.


-- 
michael

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Jan Kiszka]

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

Michael Walle wrote:
> Am Wednesday 12 May 2010 09:56:31 schrieb Jan Kiszka:
>> OK, that was a hard nut. After various dead ends, I think I found an
>> possible solution. Can you give this a try?
> [..]
>> Still requires proper patch split up, and I need to think about possible
>> side effects.
> Thanks, the patch is working.

Unfortunately, now that resetting the mode on read is fixed, my whole
optimization does not work any, writing to flash takes decades again.
Back to the drawing board...

> 
> But i noticed another minor bug. The cfi02 doesn't handle 'read flash id' on 
> 16bit accesses correctly. It always returns 8 bit. I used something like
> 
> if (width == 2)
>     ret = pfl->ident[0] << 8 | pfl->ident[1];  /* rsp. ident[1]/ident[2] */
> 
> within the 0x90 reading as a quick workaround.

Are you sure that this is valid? The whole cfi_table is also only
provided byte-wise, same in cfi01.

Jan


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

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Michael Walle]

Am Thursday 13 May 2010 09:38:43 schrieb Jan Kiszka:
> > But i noticed another minor bug. The cfi02 doesn't handle 'read flash id'
> > on 16bit accesses correctly. It always returns 8 bit. I used something
> > like
> >
> > if (width == 2)
> >     ret = pfl->ident[0] << 8 | pfl->ident[1];  /* rsp. ident[1]/ident[2]
> > */
> >
> > within the 0x90 reading as a quick workaround.
>
> Are you sure that this is valid? The whole cfi_table is also only
> provided byte-wise, same in cfi01.

At least the JEDEC ID read returns 16 bit values with x16 devices. Have a look 
at:
  http://www.spansion.com/Support/Datasheets/s29gl128_256n_sp_a2_e.pdf
  Table II on page 51

micromonitor (the program i tested with) and uboot uses 16bit reads to read 
the flash id. Have a look at
http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/mtd/cfi_flash.c;h=3267c5de36d1b12a190f93f9a3048ded598f84aa;hb=HEAD#l1535

-- 
wkr michael

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Jan Kiszka]

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

Michael Walle wrote:
> Am Thursday 13 May 2010 09:38:43 schrieb Jan Kiszka:
>>> But i noticed another minor bug. The cfi02 doesn't handle 'read flash id'
>>> on 16bit accesses correctly. It always returns 8 bit. I used something
>>> like
>>>
>>> if (width == 2)
>>>     ret = pfl->ident[0] << 8 | pfl->ident[1];  /* rsp. ident[1]/ident[2]
>>> */
>>>
>>> within the 0x90 reading as a quick workaround.
>> Are you sure that this is valid? The whole cfi_table is also only
>> provided byte-wise, same in cfi01.
> 
> At least the JEDEC ID read returns 16 bit values with x16 devices. Have a look 
> at:
>   http://www.spansion.com/Support/Datasheets/s29gl128_256n_sp_a2_e.pdf
>   Table II on page 51
> 
> micromonitor (the program i tested with) and uboot uses 16bit reads to read 
> the flash id. Have a look at
> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/mtd/cfi_flash.c;h=3267c5de36d1b12a190f93f9a3048ded598f84aa;hb=HEAD#l1535
> 

Right, I came to the same conclusion based on chip I'm using for the
Musicpal model. Working on a proper fix - now that I think to have found
a solution for the XIP vs. mode switch conflict.

Jan


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

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Jan Kiszka]

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

Jan Kiszka wrote:
> Michael Walle wrote:
>> Am Thursday 13 May 2010 09:38:43 schrieb Jan Kiszka:
>>>> But i noticed another minor bug. The cfi02 doesn't handle 'read flash id'
>>>> on 16bit accesses correctly. It always returns 8 bit. I used something
>>>> like
>>>>
>>>> if (width == 2)
>>>>     ret = pfl->ident[0] << 8 | pfl->ident[1];  /* rsp. ident[1]/ident[2]
>>>> */
>>>>
>>>> within the 0x90 reading as a quick workaround.
>>> Are you sure that this is valid? The whole cfi_table is also only
>>> provided byte-wise, same in cfi01.
>> At least the JEDEC ID read returns 16 bit values with x16 devices. Have a look 
>> at:
>>   http://www.spansion.com/Support/Datasheets/s29gl128_256n_sp_a2_e.pdf
>>   Table II on page 51
>>
>> micromonitor (the program i tested with) and uboot uses 16bit reads to read 
>> the flash id. Have a look at
>> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/mtd/cfi_flash.c;h=3267c5de36d1b12a190f93f9a3048ded598f84aa;hb=HEAD#l1535
>>
> 
> Right, I came to the same conclusion based on chip I'm using for the
> Musicpal model. Working on a proper fix - now that I think to have found
> a solution for the XIP vs. mode switch conflict.

Wait! Access to ident[0..3] is already correct as those fields are
stored and returned as 16-bit values. I guess you just did not pass the
proper IDs when registering your pflash_cfi02 instance.

But this still leaves us with the 8-bit entries of the cfi_table.

Jan


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

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Jan Kiszka]

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

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Michael Walle wrote:
>>> Am Thursday 13 May 2010 09:38:43 schrieb Jan Kiszka:
>>>>> But i noticed another minor bug. The cfi02 doesn't handle 'read flash id'
>>>>> on 16bit accesses correctly. It always returns 8 bit. I used something
>>>>> like
>>>>>
>>>>> if (width == 2)
>>>>>     ret = pfl->ident[0] << 8 | pfl->ident[1];  /* rsp. ident[1]/ident[2]
>>>>> */
>>>>>
>>>>> within the 0x90 reading as a quick workaround.
>>>> Are you sure that this is valid? The whole cfi_table is also only
>>>> provided byte-wise, same in cfi01.
>>> At least the JEDEC ID read returns 16 bit values with x16 devices. Have a look 
>>> at:
>>>   http://www.spansion.com/Support/Datasheets/s29gl128_256n_sp_a2_e.pdf
>>>   Table II on page 51
>>>
>>> micromonitor (the program i tested with) and uboot uses 16bit reads to read 
>>> the flash id. Have a look at
>>> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/mtd/cfi_flash.c;h=3267c5de36d1b12a190f93f9a3048ded598f84aa;hb=HEAD#l1535
>>>
>> Right, I came to the same conclusion based on chip I'm using for the
>> Musicpal model. Working on a proper fix - now that I think to have found
>> a solution for the XIP vs. mode switch conflict.
> 
> Wait! Access to ident[0..3] is already correct as those fields are
> stored and returned as 16-bit values. I guess you just did not pass the
> proper IDs when registering your pflash_cfi02 instance.

... or you suffered from a be/le issue. In contrast to data, the ID is
not swapped according to the byte order that was specified during init.
Does your target byte order differ from your host?

> 
> But this still leaves us with the 8-bit entries of the cfi_table.
> 

The CFI table is only returned byte-wise, even in 16- or 32-bit mode.
But I guess it should be properly swapped to the right byte order
nevertheless.

Jan


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

[Qemu-devel] Re: Commit 9c9bb6c89d4 breaks code execution from flash

[Michael Walle]

Am Thursday 13 May 2010 14:38:07 schrieben Sie:
> >> Right, I came to the same conclusion based on chip I'm using for the
> >> Musicpal model. Working on a proper fix - now that I think to have found
> >> a solution for the XIP vs. mode switch conflict.
nice :) i'll try it as soon as it's ready.

> > Wait! Access to ident[0..3] is already correct as those fields are
> > stored and returned as 16-bit values. I guess you just did not pass the
> > proper IDs when registering your pflash_cfi02 instance.
> ... or you suffered from a be/le issue. In contrast to data, the ID is
> not swapped according to the byte order that was specified during init.
> Does your target byte order differ from your host?
oh sorry, i wrongly assumed id[0..3] were the bytes for the 
manufacturer/device id. using the correct 16 bit values for id0 and id2 works 
perfectly.

-- 
michael