[Qemu-devel] [PATCH 0/4] Fix the lazy CFI mode switch

[Qemu-devel] [PATCH 0/4] Fix the lazy CFI mode switch

[Jan Kiszka]

This series addresses the major problem lazy mode switching of the
pflash_cfi02 currently has: code execution from this ROM can fail.

The reason for this was a conceptual issue that was papered over by a
bug in the original implementation. Both are addressed here by
 - allowing code execution from marked I/O memory regions (specifically
   ROM devices)
 - performing the lazy switch back of cfi02 from reprogramming to ROM
   mode via a timer

To recall why this effort is needed: Programming 7 MB of an 8 MB flash
that does not support the unlock bypass command takes 5:40 minutes with
this optimization and about 3 h (estimated, it became boring to wait for
completion) without it.

Jan Kiszka (4):
  cfi02: Fix a debug print
  Add support for execution from ROMs in IO device mode
  cfi: Mark flash memory executable
  cfi02: Use timer-based ROM mode switch

 cpu-common.h      |    2 ++
 exec-all.h        |    2 +-
 exec.c            |    2 +-
 hw/pflash_cfi01.c |    9 +++++----
 hw/pflash_cfi02.c |   46 ++++++++++++++++++++++++++++++++++------------
 5 files changed, 43 insertions(+), 18 deletions(-)

[Qemu-devel] [PATCH 1/4] cfi02: Fix a debug print

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 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 f3d3f41..8195d91 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -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:
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/4] Add support for execution from ROMs in IO device mode

[Jan Kiszka]

While IO_MEM_ROMD marks an I/O memory region as "read/execute from RAM,
but write to I/O handler", there is no flag indicating that an I/O
region which is fully managed by I/O handlers can still be hosting
executable code. One use case for this are flash device models that
switch to I/O mode during reprogramming. Not all reprogramming states
modify to read data, thus practically allow to continue execution.
Moreover, we need to avoid switching the modes too frequently for
performance reasons which requires fetching opcodes while still in I/O
device mode.

So this patch introduces the IO_MEM_EXEC flag. Flash devices need to set
it independent of their access mode. The flag is propagated from
PhysPageDesc into the TLB, and get_page_addr_code is evaluating it
instead of IO_MEM_ROMD. Testing for the latter was so far a nop anyway
as this flag never made it into the TLB.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 cpu-common.h |    2 ++
 exec-all.h   |    2 +-
 exec.c       |    2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index b24cecc..e106e33 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -125,6 +125,8 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr,
 /* Acts like a ROM when read and like a device when written.  */
 #define IO_MEM_ROMD        (1)
 #define IO_MEM_SUBPAGE     (2)
+/* Can run code from memory-mapped device */
+#define IO_MEM_EXEC        (4)
 
 #endif
 
diff --git a/exec-all.h b/exec-all.h
index 1016de2..7bc2b5b 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -331,7 +331,7 @@ static inline tb_page_addr_t get_page_addr_code(CPUState *env1, target_ulong add
         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)) {
+    if (pd > IO_MEM_ROM && !(pd & IO_MEM_EXEC)) {
 #if defined(TARGET_SPARC) || defined(TARGET_MIPS)
         do_unassigned_access(addr, 0, 1, 0, 4);
 #else
diff --git a/exec.c b/exec.c
index 3416aed..14c1770 100644
--- a/exec.c
+++ b/exec.c
@@ -2227,7 +2227,7 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
     }
 
     if (prot & PAGE_EXEC) {
-        te->addr_code = code_address;
+        te->addr_code = code_address | (pd & IO_MEM_EXEC);
     } else {
         te->addr_code = -1;
     }
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 2/4] Add support for execution from ROMs in IO device mode

[Jamie Lokier]

Jan Kiszka wrote:
> While IO_MEM_ROMD marks an I/O memory region as "read/execute from RAM,
> but write to I/O handler", there is no flag indicating that an I/O
> region which is fully managed by I/O handlers can still be hosting
> executable code. One use case for this are flash device models that
> switch to I/O mode during reprogramming. Not all reprogramming states
> modify to read data, thus practically allow to continue execution.
> Moreover, we need to avoid switching the modes too frequently for
> performance reasons which requires fetching opcodes while still in I/O
> device mode.

I like this change.

Does "fetching opcodes while still in I/O device mode" fetch opcodes
from the RAM backing, or via the I/O read handlers?

If the latter, I'm wondering how KVM would cope with that.

Thanks,
-- Jamie

Re: [Qemu-devel] [PATCH 2/4] Add support for execution from ROMs in IO device mode

[Jan Kiszka]

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

Jamie Lokier wrote:
> Jan Kiszka wrote:
>> While IO_MEM_ROMD marks an I/O memory region as "read/execute from RAM,
>> but write to I/O handler", there is no flag indicating that an I/O
>> region which is fully managed by I/O handlers can still be hosting
>> executable code. One use case for this are flash device models that
>> switch to I/O mode during reprogramming. Not all reprogramming states
>> modify to read data, thus practically allow to continue execution.
>> Moreover, we need to avoid switching the modes too frequently for
>> performance reasons which requires fetching opcodes while still in I/O
>> device mode.
> 
> I like this change.
> 
> Does "fetching opcodes while still in I/O device mode" fetch opcodes
> from the RAM backing, or via the I/O read handlers?

Via the latter. This is most "correct" in that broken guests that jump
to the ROM while it's in some critical write cycle will properly crash.

> 
> If the latter, I'm wondering how KVM would cope with that.

I think it won't work without extending KVM to fetch - as a last resort
- also from I/O devices. Or we give up the "correctness" and keep the
RAM-base KVM slot for IO_MEM_EXEC regions. That should be solvable in a
transparent way in kvm_set_phys_mem.

Jan


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

Re: [Qemu-devel] [PATCH 2/4] Add support for execution from ROMs in IO device mode

[Jan Kiszka]

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

Jan Kiszka wrote:
> Jamie Lokier wrote:
>> Jan Kiszka wrote:
>>> While IO_MEM_ROMD marks an I/O memory region as "read/execute from RAM,
>>> but write to I/O handler", there is no flag indicating that an I/O
>>> region which is fully managed by I/O handlers can still be hosting
>>> executable code. One use case for this are flash device models that
>>> switch to I/O mode during reprogramming. Not all reprogramming states
>>> modify to read data, thus practically allow to continue execution.
>>> Moreover, we need to avoid switching the modes too frequently for
>>> performance reasons which requires fetching opcodes while still in I/O
>>> device mode.
>> I like this change.
>>
>> Does "fetching opcodes while still in I/O device mode" fetch opcodes
>> from the RAM backing, or via the I/O read handlers?
> 
> Via the latter. This is most "correct" in that broken guests that jump
> to the ROM while it's in some critical write cycle will properly crash.
> 
>> If the latter, I'm wondering how KVM would cope with that.
> 
> I think it won't work without extending KVM to fetch - as a last resort
> - also from I/O devices. Or we give up the "correctness" and keep the
> RAM-base KVM slot for IO_MEM_EXEC regions. That should be solvable in a
> transparent way in kvm_set_phys_mem.

Which, of course, would break CFI reads etc. for KVM guest. So this
feature actually requires additional support by KVM to fetch code from
I/O devices.

Jan


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

[Qemu-devel] [PATCH 3/4] cfi: Mark flash memory executable

[Jan Kiszka]

Add the new IO_MEM_EXEC flag to all cfi01/02 memory regions to allow
execution from them in any state.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 hw/pflash_cfi01.c |    9 +++++----
 hw/pflash_cfi02.c |    2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 20fe93d..50f6598 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -90,7 +90,7 @@ static void pflash_timer (void *opaque)
         pfl->wcycle = 2;
     } else {
         cpu_register_physical_memory(pfl->base, pfl->total_len,
-                        pfl->off | IO_MEM_ROMD | pfl->fl_mem);
+                        pfl->off | IO_MEM_ROMD | IO_MEM_EXEC | pfl->fl_mem);
         pfl->wcycle = 0;
     }
     pfl->cmd = 0;
@@ -247,7 +247,8 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
 
     if (!pfl->wcycle) {
         /* Set the device in I/O access mode */
-        cpu_register_physical_memory(pfl->base, pfl->total_len, pfl->fl_mem);
+        cpu_register_physical_memory(pfl->base, pfl->total_len,
+                                     pfl->fl_mem | IO_MEM_EXEC);
     }
 
     switch (pfl->wcycle) {
@@ -403,7 +404,7 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
 
  reset_flash:
     cpu_register_physical_memory(pfl->base, pfl->total_len,
-                    pfl->off | IO_MEM_ROMD | pfl->fl_mem);
+                    pfl->off | IO_MEM_ROMD | IO_MEM_EXEC | pfl->fl_mem);
 
     pfl->bypass = 0;
     pfl->wcycle = 0;
@@ -587,7 +588,7 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
     }
     pfl->off = off;
     cpu_register_physical_memory(base, total_len,
-                    off | pfl->fl_mem | IO_MEM_ROMD);
+                    off | pfl->fl_mem | IO_MEM_ROMD | IO_MEM_EXEC);
 
     pfl->bs = bs;
     if (pfl->bs) {
diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 8195d91..6bc58b4 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -75,7 +75,7 @@ struct pflash_t {
 
 static void pflash_register_memory(pflash_t *pfl, int rom_mode)
 {
-    unsigned long phys_offset = pfl->fl_mem;
+    unsigned long phys_offset = pfl->fl_mem | IO_MEM_EXEC;
     int i;
 
     if (rom_mode)
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/4] cfi02: Use timer-based ROM mode switch

[Jan Kiszka]

Due to a bug in pflash_read, we did not switch back to ROM mode when we
should have. But simply fixing the inverted logic leaves us with slow
word-wise flash programming when the guest verifies each write via a
read.

For this reason, this patch establishes a 100 ms timer to trigger the
ROM mode switch. It also fixes the code to start this sequence from all
places that sets write cycle to 0.

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

diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 6bc58b4..3d5ea9a 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -66,7 +66,8 @@ struct pflash_t {
     uint16_t unlock_addr[2];
     uint8_t cfi_len;
     uint8_t cfi_table[0x52];
-    QEMUTimer *timer;
+    QEMUTimer *cmd_timer;
+    QEMUTimer *mode_timer;
     ram_addr_t off;
     int fl_mem;
     int rom_mode;
@@ -82,12 +83,22 @@ static void pflash_register_memory(pflash_t *pfl, int rom_mode)
         phys_offset |= pfl->off | IO_MEM_ROMD;
     pfl->rom_mode = rom_mode;
 
+    DPRINTF("%s: rom_mode %d\n", __func__, rom_mode);
     for (i = 0; i < pfl->mappings; i++)
         cpu_register_physical_memory(pfl->base + i * pfl->chip_len,
                                      pfl->chip_len, phys_offset);
 }
 
-static void pflash_timer (void *opaque)
+static void pflash_start_rom_mode_timer(pflash_t *pfl)
+{
+    if (!pfl->rom_mode) {
+        /* Defer switch by 100 ms */
+        qemu_mod_timer(pfl->mode_timer,
+                       qemu_get_clock(vm_clock) + get_ticks_per_sec() / 10);
+    }
+}
+
+static void pflash_cmd_timer(void *opaque)
 {
     pflash_t *pfl = opaque;
 
@@ -97,12 +108,21 @@ static void pflash_timer (void *opaque)
     if (pfl->bypass) {
         pfl->wcycle = 2;
     } else {
-        pflash_register_memory(pfl, 1);
+        pflash_start_rom_mode_timer(pfl);
         pfl->wcycle = 0;
     }
     pfl->cmd = 0;
 }
 
+static void pflash_mode_timer(void *opaque)
+{
+    pflash_t *pfl = opaque;
+
+    if (!pfl->rom_mode && pfl->wcycle == 0) {
+        pflash_register_memory(pfl, 1);
+    }
+}
+
 static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
                              int width, int be)
 {
@@ -112,10 +132,9 @@ 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);
+    if (pfl->wcycle == 0) {
+        /* Lazy switch to ROMD mode */
+        pflash_start_rom_mode_timer(pfl);
     }
     offset &= pfl->chip_len - 1;
     boff = offset & 0xFF;
@@ -127,6 +146,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
     default:
         /* This should never happen : reset state & treat it as a read*/
         DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
+        pflash_start_rom_mode_timer(pfl);
         pfl->wcycle = 0;
         pfl->cmd = 0;
     case 0x80:
@@ -389,7 +409,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
             pfl->status = 0x00;
             pflash_update(pfl, 0, pfl->chip_len);
             /* Let's wait 5 seconds before chip erase is done */
-            qemu_mod_timer(pfl->timer,
+            qemu_mod_timer(pfl->cmd_timer,
                            qemu_get_clock(vm_clock) + (get_ticks_per_sec() * 5));
             break;
         case 0x30:
@@ -402,7 +422,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
             pflash_update(pfl, offset, pfl->sector_len);
             pfl->status = 0x00;
             /* Let's wait 1/2 second before sector erase is done */
-            qemu_mod_timer(pfl->timer,
+            qemu_mod_timer(pfl->cmd_timer,
                            qemu_get_clock(vm_clock) + (get_ticks_per_sec() / 2));
             break;
         default:
@@ -440,6 +460,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
 
     /* Reset flash */
  reset_flash:
+    pflash_start_rom_mode_timer(pfl);
     pfl->bypass = 0;
     pfl->wcycle = 0;
     pfl->cmd = 0;
@@ -647,7 +668,8 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
 #else
     pfl->ro = 0;
 #endif
-    pfl->timer = qemu_new_timer(vm_clock, pflash_timer, pfl);
+    pfl->cmd_timer = qemu_new_timer(vm_clock, pflash_cmd_timer, pfl);
+    pfl->mode_timer = qemu_new_timer(vm_clock, pflash_mode_timer, pfl);
     pfl->sector_len = sector_len;
     pfl->width = width;
     pfl->wcycle = 0;
-- 
1.6.0.2