* [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
@ 2011-07-25 21:34 Jordan Justen
2011-07-25 21:34 ` [Qemu-devel] [PATCH 2/2] pc: Support system flash memory with pflash Jordan Justen
2011-07-27 9:30 ` [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode Jan Kiszka
0 siblings, 2 replies; 10+ messages in thread
From: Jordan Justen @ 2011-07-25 21:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Jordan Justen, Anthony Liguori, Aurelien Jarno, Jan Kiszka
Read-only mode is indicated by bdrv_is_read_only
When read-only mode is enabled, no changes will be made
to the flash image in memory, and no bdrv_write calls will be
made.
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Anthony Liguori <aliguori@us.ibm.com>
---
blockdev.c | 3 +-
hw/pflash_cfi01.c | 36 ++++++++++++++---------
hw/pflash_cfi02.c | 82 ++++++++++++++++++++++++++++------------------------
3 files changed, 68 insertions(+), 53 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 0b8d3a4..566a502 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -521,7 +521,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
/* CDROM is fine for any interface, don't check. */
ro = 1;
} else if (ro == 1) {
- if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
+ if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
+ type != IF_NONE && type != IF_PFLASH) {
error_report("readonly not supported by this bus type");
goto err;
}
diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 90fdc84..11ac490 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -284,8 +284,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
TARGET_FMT_plx "\n",
__func__, offset, pfl->sector_len);
- memset(p + offset, 0xff, pfl->sector_len);
- pflash_update(pfl, offset, pfl->sector_len);
+ if (!pfl->ro) {
+ memset(p + offset, 0xff, pfl->sector_len);
+ pflash_update(pfl, offset, pfl->sector_len);
+ }
pfl->status |= 0x80; /* Ready! */
break;
case 0x50: /* Clear status bits */
@@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
case 0x10: /* Single Byte Program */
case 0x40: /* Single Byte Program */
DPRINTF("%s: Single Byte Program\n", __func__);
- pflash_data_write(pfl, offset, value, width, be);
- pflash_update(pfl, offset, width);
+ if (!pfl->ro) {
+ pflash_data_write(pfl, offset, value, width, be);
+ pflash_update(pfl, offset, width);
+ }
pfl->status |= 0x80; /* Ready! */
pfl->wcycle = 0;
break;
@@ -373,7 +377,9 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
case 2:
switch (pfl->cmd) {
case 0xe8: /* Block write */
- pflash_data_write(pfl, offset, value, width, be);
+ if (!pfl->ro) {
+ pflash_data_write(pfl, offset, value, width, be);
+ }
pfl->status |= 0x80;
@@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
DPRINTF("%s: block write finished\n", __func__);
pfl->wcycle++;
- /* Flush the entire write buffer onto backing storage. */
- pflash_update(pfl, offset & mask, pfl->writeblock_size);
+ if (!pfl->ro) {
+ /* Flush the entire write buffer onto backing storage. */
+ pflash_update(pfl, offset & mask, pfl->writeblock_size);
+ }
}
pfl->counter--;
@@ -621,13 +629,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
return NULL;
}
}
-#if 0 /* XXX: there should be a bit to set up read-only,
- * the same way the hardware does (with WP pin).
- */
- pfl->ro = 1;
-#else
- pfl->ro = 0;
-#endif
+
+ if (pfl->bs) {
+ pfl->ro = bdrv_is_read_only(pfl->bs);
+ } else {
+ pfl->ro = 0;
+ }
+
pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
pfl->base = base;
pfl->sector_len = sector_len;
diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 725cd1e..920209d 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -315,35 +315,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
__func__, offset, value, width);
p = pfl->storage;
- switch (width) {
- case 1:
- p[offset] &= value;
- pflash_update(pfl, offset, 1);
- break;
- case 2:
- if (be) {
- p[offset] &= value >> 8;
- p[offset + 1] &= value;
- } else {
+ if (!pfl->ro) {
+ switch (width) {
+ case 1:
p[offset] &= value;
- p[offset + 1] &= value >> 8;
+ pflash_update(pfl, offset, 1);
+ break;
+ case 2:
+ if (be) {
+ p[offset] &= value >> 8;
+ p[offset + 1] &= value;
+ } else {
+ p[offset] &= value;
+ p[offset + 1] &= value >> 8;
+ }
+ pflash_update(pfl, offset, 2);
+ break;
+ case 4:
+ if (be) {
+ p[offset] &= value >> 24;
+ p[offset + 1] &= value >> 16;
+ p[offset + 2] &= value >> 8;
+ p[offset + 3] &= value;
+ } else {
+ p[offset] &= value;
+ p[offset + 1] &= value >> 8;
+ p[offset + 2] &= value >> 16;
+ p[offset + 3] &= value >> 24;
+ }
+ pflash_update(pfl, offset, 4);
+ break;
}
- pflash_update(pfl, offset, 2);
- break;
- case 4:
- if (be) {
- p[offset] &= value >> 24;
- p[offset + 1] &= value >> 16;
- p[offset + 2] &= value >> 8;
- p[offset + 3] &= value;
- } else {
- p[offset] &= value;
- p[offset + 1] &= value >> 8;
- p[offset + 2] &= value >> 16;
- p[offset + 3] &= value >> 24;
- }
- pflash_update(pfl, offset, 4);
- break;
}
pfl->status = 0x00 | ~(value & 0x80);
/* Let's pretend write is immediate */
@@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
}
/* Chip erase */
DPRINTF("%s: start chip erase\n", __func__);
- memset(pfl->storage, 0xFF, pfl->chip_len);
+ if (!pfl->ro) {
+ memset(pfl->storage, 0xFF, pfl->chip_len);
+ pflash_update(pfl, 0, pfl->chip_len);
+ }
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_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
@@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
offset &= ~(pfl->sector_len - 1);
DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
offset);
- memset(p + offset, 0xFF, pfl->sector_len);
- pflash_update(pfl, offset, pfl->sector_len);
+ if (!pfl->ro) {
+ memset(p + offset, 0xFF, pfl->sector_len);
+ 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,
@@ -644,13 +650,13 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
return NULL;
}
}
-#if 0 /* XXX: there should be a bit to set up read-only,
- * the same way the hardware does (with WP pin).
- */
- pfl->ro = 1;
-#else
- pfl->ro = 0;
-#endif
+
+ if (pfl->bs) {
+ pfl->ro = bdrv_is_read_only(pfl->bs);
+ } else {
+ pfl->ro = 0;
+ }
+
pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
pfl->sector_len = sector_len;
pfl->width = width;
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH 2/2] pc: Support system flash memory with pflash
2011-07-25 21:34 [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode Jordan Justen
@ 2011-07-25 21:34 ` Jordan Justen
2011-07-29 13:06 ` Anthony Liguori
2011-07-27 9:30 ` [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode Jan Kiszka
1 sibling, 1 reply; 10+ messages in thread
From: Jordan Justen @ 2011-07-25 21:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Jordan Justen, Anthony Liguori, Aurelien Jarno
If a pflash image is found, then it is used for the system
firmware image.
If a pflash image is not initially found, then a read-only
pflash device is created using the -bios filename.
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
---
Makefile.target | 2 +-
default-configs/i386-softmmu.mak | 1 +
default-configs/x86_64-softmmu.mak | 1 +
hw/boards.h | 1 +
hw/pc.c | 46 +------------
hw/pc.h | 2 +
hw/pcflash.c | 130 ++++++++++++++++++++++++++++++++++++
vl.c | 2 +-
8 files changed, 141 insertions(+), 44 deletions(-)
create mode 100644 hw/pcflash.c
diff --git a/Makefile.target b/Makefile.target
index cde509b..4d8821f 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -223,7 +223,7 @@ obj-$(CONFIG_IVSHMEM) += ivshmem.o
# Hardware support
obj-i386-y += vga.o
-obj-i386-y += mc146818rtc.o i8259.o pc.o
+obj-i386-y += mc146818rtc.o i8259.o pc.o pcflash.o
obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
obj-i386-y += vmport.o
obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 55589fa..8697cd4 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -21,3 +21,4 @@ CONFIG_PIIX_PCI=y
CONFIG_SOUND=y
CONFIG_HPET=y
CONFIG_APPLESMC=y
+CONFIG_PFLASH_CFI01=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 8895028..eca9284 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -21,3 +21,4 @@ CONFIG_PIIX_PCI=y
CONFIG_SOUND=y
CONFIG_HPET=y
CONFIG_APPLESMC=y
+CONFIG_PFLASH_CFI01=y
diff --git a/hw/boards.h b/hw/boards.h
index 716fd7b..45a31a1 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -33,6 +33,7 @@ typedef struct QEMUMachine {
} QEMUMachine;
int qemu_register_machine(QEMUMachine *m);
+QEMUMachine *find_default_machine(void);
extern QEMUMachine *current_machine;
diff --git a/hw/pc.c b/hw/pc.c
index a3e8539..cf7257b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -55,10 +55,6 @@
#define DPRINTF(fmt, ...)
#endif
-#define BIOS_FILENAME "bios.bin"
-
-#define PC_MAX_BIOS_SIZE (4 * 1024 * 1024)
-
/* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables. */
#define ACPI_DATA_SIZE 0x10000
#define BIOS_CFG_IOPORT 0x510
@@ -963,10 +959,8 @@ void pc_memory_init(const char *kernel_filename,
ram_addr_t below_4g_mem_size,
ram_addr_t above_4g_mem_size)
{
- char *filename;
- int ret, linux_boot, i;
- ram_addr_t ram_addr, bios_offset, option_rom_offset;
- int bios_size, isa_bios_size;
+ int linux_boot, i;
+ ram_addr_t ram_addr, option_rom_offset;
void *fw_cfg;
linux_boot = (kernel_filename != NULL);
@@ -983,44 +977,12 @@ void pc_memory_init(const char *kernel_filename,
ram_addr + below_4g_mem_size);
}
- /* BIOS load */
- if (bios_name == NULL)
- bios_name = BIOS_FILENAME;
- filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
- if (filename) {
- bios_size = get_image_size(filename);
- } else {
- bios_size = -1;
- }
- if (bios_size <= 0 ||
- (bios_size % 65536) != 0) {
- goto bios_error;
- }
- bios_offset = qemu_ram_alloc(NULL, "pc.bios", bios_size);
- ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
- if (ret != 0) {
- bios_error:
- fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
- exit(1);
- }
- if (filename) {
- qemu_free(filename);
- }
- /* map the last 128KB of the BIOS in ISA space */
- isa_bios_size = bios_size;
- if (isa_bios_size > (128 * 1024))
- isa_bios_size = 128 * 1024;
- cpu_register_physical_memory(0x100000 - isa_bios_size,
- isa_bios_size,
- (bios_offset + bios_size - isa_bios_size) | IO_MEM_ROM);
+ /* Initialize ROM or flash ranges for PC firmware */
+ pc_system_firmware_init();
option_rom_offset = qemu_ram_alloc(NULL, "pc.rom", PC_ROM_SIZE);
cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
- /* map all the bios at the top of memory */
- cpu_register_physical_memory((uint32_t)(-bios_size),
- bios_size, bios_offset | IO_MEM_ROM);
-
fw_cfg = bochs_bios_init();
rom_set_fw(fw_cfg);
diff --git a/hw/pc.h b/hw/pc.h
index 6d5730b..114816d 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -238,4 +238,6 @@ static inline bool isa_ne2000_init(int base, int irq, NICInfo *nd)
int e820_add_entry(uint64_t, uint64_t, uint32_t);
+void pc_system_firmware_init(void);
+
#endif
diff --git a/hw/pcflash.c b/hw/pcflash.c
new file mode 100644
index 0000000..514b1ed
--- /dev/null
+++ b/hw/pcflash.c
@@ -0,0 +1,130 @@
+/*
+ * QEMU PC System Flash
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ * Copyright (c) 2011 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw.h"
+#include "pc.h"
+#include "hw/boards.h"
+#include "loader.h"
+#include "sysemu.h"
+#include "flash.h"
+
+#define BIOS_FILENAME "bios.bin"
+
+#define PC_MAX_BIOS_SIZE (4 * 1024 * 1024)
+
+static void pc_isa_bios_init(ram_addr_t ram_offset, int ram_size)
+{
+ int isa_bios_size;
+
+ /* map the last 128KB of the BIOS in ISA space */
+ isa_bios_size = ram_size;
+ if (isa_bios_size > (128 * 1024)) {
+ isa_bios_size = 128 * 1024;
+ }
+ ram_offset = ram_offset + ram_size - isa_bios_size;
+ cpu_register_physical_memory(0x100000 - isa_bios_size,
+ isa_bios_size,
+ ram_offset | IO_MEM_ROM);
+}
+
+static void pc_default_system_flash_init(void)
+{
+ QemuOpts *opts;
+ QEMUMachine *machine;
+ char *filename;
+
+ if (bios_name == NULL) {
+ bios_name = BIOS_FILENAME;
+ }
+ filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+ fprintf(stderr, "ROM: %s\n", filename);
+
+ opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
+ if (opts == NULL) {
+ return;
+ }
+
+ machine = find_default_machine();
+ if (machine == NULL) {
+ return;
+ }
+
+ drive_init(opts, machine->use_scsi);
+}
+
+static void pc_system_flash_init(DriveInfo *pflash_drv)
+{
+ BlockDriverState *bdrv;
+ int64_t size;
+ target_phys_addr_t phys_addr;
+ ram_addr_t addr;
+ int sector_bits, sector_size;
+
+ bdrv = NULL;
+
+ bdrv = pflash_drv->bdrv;
+ size = bdrv_getlength(pflash_drv->bdrv);
+ sector_bits = 12;
+ sector_size = 1 << sector_bits;
+
+ if ((size % sector_size) != 0) {
+ fprintf(stderr,
+ "qemu: PC system firmware (pflash) must be a multiple of 0x%x\n",
+ sector_size);
+ exit(1);
+ }
+
+ phys_addr = 0x100000000ULL - size;
+ addr = qemu_ram_alloc(NULL, "system.flash", size);
+ pflash_cfi01_register(phys_addr, addr, bdrv,
+ sector_size, size >> sector_bits,
+ 4, 0x0000, 0x0000, 0x0000, 0x0000, 0);
+
+ pc_isa_bios_init(addr, size);
+}
+
+void pc_system_firmware_init(void)
+{
+ int flash_present;
+ DriveInfo *pflash_drv;
+
+ pflash_drv = drive_get(IF_PFLASH, 0, 0);
+ flash_present = (pflash_drv != NULL);
+
+ if (!flash_present) {
+ pc_default_system_flash_init();
+ pflash_drv = drive_get(IF_PFLASH, 0, 0);
+ flash_present = (pflash_drv != NULL);
+ }
+
+ if (!flash_present) {
+ fprintf(stderr, "qemu: PC system firmware (pflash) not available\n");
+ exit(1);
+ }
+
+ pc_system_flash_init(pflash_drv);
+}
+
+
diff --git a/vl.c b/vl.c
index 4b6688b..00d8fcb 100644
--- a/vl.c
+++ b/vl.c
@@ -1063,7 +1063,7 @@ static QEMUMachine *find_machine(const char *name)
return NULL;
}
-static QEMUMachine *find_default_machine(void)
+QEMUMachine *find_default_machine(void)
{
QEMUMachine *m;
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
2011-07-25 21:34 [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode Jordan Justen
2011-07-25 21:34 ` [Qemu-devel] [PATCH 2/2] pc: Support system flash memory with pflash Jordan Justen
@ 2011-07-27 9:30 ` Jan Kiszka
2011-07-27 15:38 ` Jordan Justen
1 sibling, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2011-07-27 9:30 UTC (permalink / raw)
To: Jordan Justen; +Cc: Anthony Liguori, qemu-devel@nongnu.org, Aurelien Jarno
On 2011-07-25 23:34, Jordan Justen wrote:
> Read-only mode is indicated by bdrv_is_read_only
>
> When read-only mode is enabled, no changes will be made
> to the flash image in memory, and no bdrv_write calls will be
> made.
>
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> ---
> blockdev.c | 3 +-
> hw/pflash_cfi01.c | 36 ++++++++++++++---------
> hw/pflash_cfi02.c | 82 ++++++++++++++++++++++++++++------------------------
> 3 files changed, 68 insertions(+), 53 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0b8d3a4..566a502 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -521,7 +521,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> /* CDROM is fine for any interface, don't check. */
> ro = 1;
> } else if (ro == 1) {
> - if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
> + type != IF_NONE && type != IF_PFLASH) {
> error_report("readonly not supported by this bus type");
> goto err;
> }
> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
> index 90fdc84..11ac490 100644
> --- a/hw/pflash_cfi01.c
> +++ b/hw/pflash_cfi01.c
> @@ -284,8 +284,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
> TARGET_FMT_plx "\n",
> __func__, offset, pfl->sector_len);
>
> - memset(p + offset, 0xff, pfl->sector_len);
> - pflash_update(pfl, offset, pfl->sector_len);
> + if (!pfl->ro) {
> + memset(p + offset, 0xff, pfl->sector_len);
> + pflash_update(pfl, offset, pfl->sector_len);
> + }
> pfl->status |= 0x80; /* Ready! */
> break;
> case 0x50: /* Clear status bits */
> @@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
> case 0x10: /* Single Byte Program */
> case 0x40: /* Single Byte Program */
> DPRINTF("%s: Single Byte Program\n", __func__);
> - pflash_data_write(pfl, offset, value, width, be);
> - pflash_update(pfl, offset, width);
> + if (!pfl->ro) {
> + pflash_data_write(pfl, offset, value, width, be);
> + pflash_update(pfl, offset, width);
> + }
> pfl->status |= 0x80; /* Ready! */
> pfl->wcycle = 0;
> break;
> @@ -373,7 +377,9 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
> case 2:
> switch (pfl->cmd) {
> case 0xe8: /* Block write */
> - pflash_data_write(pfl, offset, value, width, be);
> + if (!pfl->ro) {
> + pflash_data_write(pfl, offset, value, width, be);
> + }
>
> pfl->status |= 0x80;
>
> @@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>
> DPRINTF("%s: block write finished\n", __func__);
> pfl->wcycle++;
> - /* Flush the entire write buffer onto backing storage. */
> - pflash_update(pfl, offset & mask, pfl->writeblock_size);
> + if (!pfl->ro) {
> + /* Flush the entire write buffer onto backing storage. */
> + pflash_update(pfl, offset & mask, pfl->writeblock_size);
> + }
> }
>
> pfl->counter--;
> @@ -621,13 +629,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
> return NULL;
> }
> }
> -#if 0 /* XXX: there should be a bit to set up read-only,
> - * the same way the hardware does (with WP pin).
> - */
> - pfl->ro = 1;
> -#else
> - pfl->ro = 0;
> -#endif
> +
> + if (pfl->bs) {
> + pfl->ro = bdrv_is_read_only(pfl->bs);
> + } else {
> + pfl->ro = 0;
> + }
> +
> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
> pfl->base = base;
> pfl->sector_len = sector_len;
> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index 725cd1e..920209d 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -315,35 +315,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
> DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
> __func__, offset, value, width);
> p = pfl->storage;
> - switch (width) {
> - case 1:
> - p[offset] &= value;
> - pflash_update(pfl, offset, 1);
> - break;
> - case 2:
> - if (be) {
> - p[offset] &= value >> 8;
> - p[offset + 1] &= value;
> - } else {
> + if (!pfl->ro) {
> + switch (width) {
> + case 1:
> p[offset] &= value;
> - p[offset + 1] &= value >> 8;
> + pflash_update(pfl, offset, 1);
> + break;
> + case 2:
> + if (be) {
> + p[offset] &= value >> 8;
> + p[offset + 1] &= value;
> + } else {
> + p[offset] &= value;
> + p[offset + 1] &= value >> 8;
> + }
> + pflash_update(pfl, offset, 2);
> + break;
> + case 4:
> + if (be) {
> + p[offset] &= value >> 24;
> + p[offset + 1] &= value >> 16;
> + p[offset + 2] &= value >> 8;
> + p[offset + 3] &= value;
> + } else {
> + p[offset] &= value;
> + p[offset + 1] &= value >> 8;
> + p[offset + 2] &= value >> 16;
> + p[offset + 3] &= value >> 24;
> + }
> + pflash_update(pfl, offset, 4);
> + break;
> }
> - pflash_update(pfl, offset, 2);
> - break;
> - case 4:
> - if (be) {
> - p[offset] &= value >> 24;
> - p[offset + 1] &= value >> 16;
> - p[offset + 2] &= value >> 8;
> - p[offset + 3] &= value;
> - } else {
> - p[offset] &= value;
> - p[offset + 1] &= value >> 8;
> - p[offset + 2] &= value >> 16;
> - p[offset + 3] &= value >> 24;
> - }
> - pflash_update(pfl, offset, 4);
> - break;
> }
> pfl->status = 0x00 | ~(value & 0x80);
> /* Let's pretend write is immediate */
> @@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
> }
> /* Chip erase */
> DPRINTF("%s: start chip erase\n", __func__);
> - memset(pfl->storage, 0xFF, pfl->chip_len);
> + if (!pfl->ro) {
> + memset(pfl->storage, 0xFF, pfl->chip_len);
> + pflash_update(pfl, 0, pfl->chip_len);
> + }
> 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_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
> @@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
> offset &= ~(pfl->sector_len - 1);
> DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
> offset);
> - memset(p + offset, 0xFF, pfl->sector_len);
> - pflash_update(pfl, offset, pfl->sector_len);
> + if (!pfl->ro) {
> + memset(p + offset, 0xFF, pfl->sector_len);
> + 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,
> @@ -644,13 +650,13 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
> return NULL;
> }
> }
> -#if 0 /* XXX: there should be a bit to set up read-only,
> - * the same way the hardware does (with WP pin).
> - */
> - pfl->ro = 1;
> -#else
> - pfl->ro = 0;
> -#endif
> +
> + if (pfl->bs) {
> + pfl->ro = bdrv_is_read_only(pfl->bs);
> + } else {
> + pfl->ro = 0;
> + }
> +
> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
> pfl->sector_len = sector_len;
> pfl->width = width;
Looks good in general. I'm just wondering if real hw gives any feedback
on writes to flashes in read-only mode or silently ignores them as
above? Or am I missing the feedback bits?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
2011-07-27 9:30 ` [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode Jan Kiszka
@ 2011-07-27 15:38 ` Jordan Justen
2011-07-28 18:26 ` Jan Kiszka
0 siblings, 1 reply; 10+ messages in thread
From: Jordan Justen @ 2011-07-27 15:38 UTC (permalink / raw)
To: Jan Kiszka
Cc: Jordan Justen, Anthony Liguori, qemu-devel@nongnu.org,
Aurelien Jarno
On Wed, Jul 27, 2011 at 02:30, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-07-25 23:34, Jordan Justen wrote:
>> Read-only mode is indicated by bdrv_is_read_only
>>
>> When read-only mode is enabled, no changes will be made
>> to the flash image in memory, and no bdrv_write calls will be
>> made.
>>
>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>> blockdev.c | 3 +-
>> hw/pflash_cfi01.c | 36 ++++++++++++++---------
>> hw/pflash_cfi02.c | 82 ++++++++++++++++++++++++++++------------------------
>> 3 files changed, 68 insertions(+), 53 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 0b8d3a4..566a502 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -521,7 +521,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>> /* CDROM is fine for any interface, don't check. */
>> ro = 1;
>> } else if (ro == 1) {
>> - if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
>> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
>> + type != IF_NONE && type != IF_PFLASH) {
>> error_report("readonly not supported by this bus type");
>> goto err;
>> }
>> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
>> index 90fdc84..11ac490 100644
>> --- a/hw/pflash_cfi01.c
>> +++ b/hw/pflash_cfi01.c
>> @@ -284,8 +284,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>> TARGET_FMT_plx "\n",
>> __func__, offset, pfl->sector_len);
>>
>> - memset(p + offset, 0xff, pfl->sector_len);
>> - pflash_update(pfl, offset, pfl->sector_len);
>> + if (!pfl->ro) {
>> + memset(p + offset, 0xff, pfl->sector_len);
>> + pflash_update(pfl, offset, pfl->sector_len);
>> + }
>> pfl->status |= 0x80; /* Ready! */
>> break;
>> case 0x50: /* Clear status bits */
>> @@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>> case 0x10: /* Single Byte Program */
>> case 0x40: /* Single Byte Program */
>> DPRINTF("%s: Single Byte Program\n", __func__);
>> - pflash_data_write(pfl, offset, value, width, be);
>> - pflash_update(pfl, offset, width);
>> + if (!pfl->ro) {
>> + pflash_data_write(pfl, offset, value, width, be);
>> + pflash_update(pfl, offset, width);
>> + }
>> pfl->status |= 0x80; /* Ready! */
>> pfl->wcycle = 0;
>> break;
>> @@ -373,7 +377,9 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>> case 2:
>> switch (pfl->cmd) {
>> case 0xe8: /* Block write */
>> - pflash_data_write(pfl, offset, value, width, be);
>> + if (!pfl->ro) {
>> + pflash_data_write(pfl, offset, value, width, be);
>> + }
>>
>> pfl->status |= 0x80;
>>
>> @@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>
>> DPRINTF("%s: block write finished\n", __func__);
>> pfl->wcycle++;
>> - /* Flush the entire write buffer onto backing storage. */
>> - pflash_update(pfl, offset & mask, pfl->writeblock_size);
>> + if (!pfl->ro) {
>> + /* Flush the entire write buffer onto backing storage. */
>> + pflash_update(pfl, offset & mask, pfl->writeblock_size);
>> + }
>> }
>>
>> pfl->counter--;
>> @@ -621,13 +629,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
>> return NULL;
>> }
>> }
>> -#if 0 /* XXX: there should be a bit to set up read-only,
>> - * the same way the hardware does (with WP pin).
>> - */
>> - pfl->ro = 1;
>> -#else
>> - pfl->ro = 0;
>> -#endif
>> +
>> + if (pfl->bs) {
>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>> + } else {
>> + pfl->ro = 0;
>> + }
>> +
>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>> pfl->base = base;
>> pfl->sector_len = sector_len;
>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>> index 725cd1e..920209d 100644
>> --- a/hw/pflash_cfi02.c
>> +++ b/hw/pflash_cfi02.c
>> @@ -315,35 +315,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>> DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
>> __func__, offset, value, width);
>> p = pfl->storage;
>> - switch (width) {
>> - case 1:
>> - p[offset] &= value;
>> - pflash_update(pfl, offset, 1);
>> - break;
>> - case 2:
>> - if (be) {
>> - p[offset] &= value >> 8;
>> - p[offset + 1] &= value;
>> - } else {
>> + if (!pfl->ro) {
>> + switch (width) {
>> + case 1:
>> p[offset] &= value;
>> - p[offset + 1] &= value >> 8;
>> + pflash_update(pfl, offset, 1);
>> + break;
>> + case 2:
>> + if (be) {
>> + p[offset] &= value >> 8;
>> + p[offset + 1] &= value;
>> + } else {
>> + p[offset] &= value;
>> + p[offset + 1] &= value >> 8;
>> + }
>> + pflash_update(pfl, offset, 2);
>> + break;
>> + case 4:
>> + if (be) {
>> + p[offset] &= value >> 24;
>> + p[offset + 1] &= value >> 16;
>> + p[offset + 2] &= value >> 8;
>> + p[offset + 3] &= value;
>> + } else {
>> + p[offset] &= value;
>> + p[offset + 1] &= value >> 8;
>> + p[offset + 2] &= value >> 16;
>> + p[offset + 3] &= value >> 24;
>> + }
>> + pflash_update(pfl, offset, 4);
>> + break;
>> }
>> - pflash_update(pfl, offset, 2);
>> - break;
>> - case 4:
>> - if (be) {
>> - p[offset] &= value >> 24;
>> - p[offset + 1] &= value >> 16;
>> - p[offset + 2] &= value >> 8;
>> - p[offset + 3] &= value;
>> - } else {
>> - p[offset] &= value;
>> - p[offset + 1] &= value >> 8;
>> - p[offset + 2] &= value >> 16;
>> - p[offset + 3] &= value >> 24;
>> - }
>> - pflash_update(pfl, offset, 4);
>> - break;
>> }
>> pfl->status = 0x00 | ~(value & 0x80);
>> /* Let's pretend write is immediate */
>> @@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>> }
>> /* Chip erase */
>> DPRINTF("%s: start chip erase\n", __func__);
>> - memset(pfl->storage, 0xFF, pfl->chip_len);
>> + if (!pfl->ro) {
>> + memset(pfl->storage, 0xFF, pfl->chip_len);
>> + pflash_update(pfl, 0, pfl->chip_len);
>> + }
>> 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_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
>> @@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>> offset &= ~(pfl->sector_len - 1);
>> DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
>> offset);
>> - memset(p + offset, 0xFF, pfl->sector_len);
>> - pflash_update(pfl, offset, pfl->sector_len);
>> + if (!pfl->ro) {
>> + memset(p + offset, 0xFF, pfl->sector_len);
>> + 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,
>> @@ -644,13 +650,13 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
>> return NULL;
>> }
>> }
>> -#if 0 /* XXX: there should be a bit to set up read-only,
>> - * the same way the hardware does (with WP pin).
>> - */
>> - pfl->ro = 1;
>> -#else
>> - pfl->ro = 0;
>> -#endif
>> +
>> + if (pfl->bs) {
>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>> + } else {
>> + pfl->ro = 0;
>> + }
>> +
>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>> pfl->sector_len = sector_len;
>> pfl->width = width;
>
> Looks good in general. I'm just wondering if real hw gives any feedback
> on writes to flashes in read-only mode or silently ignores them as
> above? Or am I missing the feedback bits?
I have the same concern.
Unfortunately, I don't have access to real hardware to investigate.
I tried looking for something in the CFI specification, but I found no
guidance there.
-Jordan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
2011-07-27 15:38 ` Jordan Justen
@ 2011-07-28 18:26 ` Jan Kiszka
2011-07-28 21:05 ` Jordan Justen
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2011-07-28 18:26 UTC (permalink / raw)
To: Jordan Justen
Cc: Jordan Justen, Anthony Liguori, qemu-devel@nongnu.org,
Aurelien Jarno
On 2011-07-27 17:38, Jordan Justen wrote:
> On Wed, Jul 27, 2011 at 02:30, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-07-25 23:34, Jordan Justen wrote:
>>> Read-only mode is indicated by bdrv_is_read_only
>>>
>>> When read-only mode is enabled, no changes will be made
>>> to the flash image in memory, and no bdrv_write calls will be
>>> made.
>>>
>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>> blockdev.c | 3 +-
>>> hw/pflash_cfi01.c | 36 ++++++++++++++---------
>>> hw/pflash_cfi02.c | 82 ++++++++++++++++++++++++++++------------------------
>>> 3 files changed, 68 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 0b8d3a4..566a502 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -521,7 +521,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>> /* CDROM is fine for any interface, don't check. */
>>> ro = 1;
>>> } else if (ro == 1) {
>>> - if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
>>> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
>>> + type != IF_NONE && type != IF_PFLASH) {
>>> error_report("readonly not supported by this bus type");
>>> goto err;
>>> }
>>> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
>>> index 90fdc84..11ac490 100644
>>> --- a/hw/pflash_cfi01.c
>>> +++ b/hw/pflash_cfi01.c
>>> @@ -284,8 +284,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>> TARGET_FMT_plx "\n",
>>> __func__, offset, pfl->sector_len);
>>>
>>> - memset(p + offset, 0xff, pfl->sector_len);
>>> - pflash_update(pfl, offset, pfl->sector_len);
>>> + if (!pfl->ro) {
>>> + memset(p + offset, 0xff, pfl->sector_len);
>>> + pflash_update(pfl, offset, pfl->sector_len);
>>> + }
>>> pfl->status |= 0x80; /* Ready! */
>>> break;
>>> case 0x50: /* Clear status bits */
>>> @@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>> case 0x10: /* Single Byte Program */
>>> case 0x40: /* Single Byte Program */
>>> DPRINTF("%s: Single Byte Program\n", __func__);
>>> - pflash_data_write(pfl, offset, value, width, be);
>>> - pflash_update(pfl, offset, width);
>>> + if (!pfl->ro) {
>>> + pflash_data_write(pfl, offset, value, width, be);
>>> + pflash_update(pfl, offset, width);
>>> + }
>>> pfl->status |= 0x80; /* Ready! */
>>> pfl->wcycle = 0;
>>> break;
>>> @@ -373,7 +377,9 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>> case 2:
>>> switch (pfl->cmd) {
>>> case 0xe8: /* Block write */
>>> - pflash_data_write(pfl, offset, value, width, be);
>>> + if (!pfl->ro) {
>>> + pflash_data_write(pfl, offset, value, width, be);
>>> + }
>>>
>>> pfl->status |= 0x80;
>>>
>>> @@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>
>>> DPRINTF("%s: block write finished\n", __func__);
>>> pfl->wcycle++;
>>> - /* Flush the entire write buffer onto backing storage. */
>>> - pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>> + if (!pfl->ro) {
>>> + /* Flush the entire write buffer onto backing storage. */
>>> + pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>> + }
>>> }
>>>
>>> pfl->counter--;
>>> @@ -621,13 +629,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
>>> return NULL;
>>> }
>>> }
>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>> - * the same way the hardware does (with WP pin).
>>> - */
>>> - pfl->ro = 1;
>>> -#else
>>> - pfl->ro = 0;
>>> -#endif
>>> +
>>> + if (pfl->bs) {
>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>> + } else {
>>> + pfl->ro = 0;
>>> + }
>>> +
>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>> pfl->base = base;
>>> pfl->sector_len = sector_len;
>>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>>> index 725cd1e..920209d 100644
>>> --- a/hw/pflash_cfi02.c
>>> +++ b/hw/pflash_cfi02.c
>>> @@ -315,35 +315,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>> DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
>>> __func__, offset, value, width);
>>> p = pfl->storage;
>>> - switch (width) {
>>> - case 1:
>>> - p[offset] &= value;
>>> - pflash_update(pfl, offset, 1);
>>> - break;
>>> - case 2:
>>> - if (be) {
>>> - p[offset] &= value >> 8;
>>> - p[offset + 1] &= value;
>>> - } else {
>>> + if (!pfl->ro) {
>>> + switch (width) {
>>> + case 1:
>>> p[offset] &= value;
>>> - p[offset + 1] &= value >> 8;
>>> + pflash_update(pfl, offset, 1);
>>> + break;
>>> + case 2:
>>> + if (be) {
>>> + p[offset] &= value >> 8;
>>> + p[offset + 1] &= value;
>>> + } else {
>>> + p[offset] &= value;
>>> + p[offset + 1] &= value >> 8;
>>> + }
>>> + pflash_update(pfl, offset, 2);
>>> + break;
>>> + case 4:
>>> + if (be) {
>>> + p[offset] &= value >> 24;
>>> + p[offset + 1] &= value >> 16;
>>> + p[offset + 2] &= value >> 8;
>>> + p[offset + 3] &= value;
>>> + } else {
>>> + p[offset] &= value;
>>> + p[offset + 1] &= value >> 8;
>>> + p[offset + 2] &= value >> 16;
>>> + p[offset + 3] &= value >> 24;
>>> + }
>>> + pflash_update(pfl, offset, 4);
>>> + break;
>>> }
>>> - pflash_update(pfl, offset, 2);
>>> - break;
>>> - case 4:
>>> - if (be) {
>>> - p[offset] &= value >> 24;
>>> - p[offset + 1] &= value >> 16;
>>> - p[offset + 2] &= value >> 8;
>>> - p[offset + 3] &= value;
>>> - } else {
>>> - p[offset] &= value;
>>> - p[offset + 1] &= value >> 8;
>>> - p[offset + 2] &= value >> 16;
>>> - p[offset + 3] &= value >> 24;
>>> - }
>>> - pflash_update(pfl, offset, 4);
>>> - break;
>>> }
>>> pfl->status = 0x00 | ~(value & 0x80);
>>> /* Let's pretend write is immediate */
>>> @@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>> }
>>> /* Chip erase */
>>> DPRINTF("%s: start chip erase\n", __func__);
>>> - memset(pfl->storage, 0xFF, pfl->chip_len);
>>> + if (!pfl->ro) {
>>> + memset(pfl->storage, 0xFF, pfl->chip_len);
>>> + pflash_update(pfl, 0, pfl->chip_len);
>>> + }
>>> 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_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
>>> @@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>> offset &= ~(pfl->sector_len - 1);
>>> DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
>>> offset);
>>> - memset(p + offset, 0xFF, pfl->sector_len);
>>> - pflash_update(pfl, offset, pfl->sector_len);
>>> + if (!pfl->ro) {
>>> + memset(p + offset, 0xFF, pfl->sector_len);
>>> + 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,
>>> @@ -644,13 +650,13 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
>>> return NULL;
>>> }
>>> }
>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>> - * the same way the hardware does (with WP pin).
>>> - */
>>> - pfl->ro = 1;
>>> -#else
>>> - pfl->ro = 0;
>>> -#endif
>>> +
>>> + if (pfl->bs) {
>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>> + } else {
>>> + pfl->ro = 0;
>>> + }
>>> +
>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>> pfl->sector_len = sector_len;
>>> pfl->width = width;
>>
>> Looks good in general. I'm just wondering if real hw gives any feedback
>> on writes to flashes in read-only mode or silently ignores them as
>> above? Or am I missing the feedback bits?
>
> I have the same concern.
>
> Unfortunately, I don't have access to real hardware to investigate.
>
> I tried looking for something in the CFI specification, but I found no
> guidance there.
I've discussed this with some friends, and it actually seems like there
is no clean write protection in the "real world". The obvious approach
to cut the write enable line to the chip also has some ugly side effects
that we probably don't want to emulate. See e.g.
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/100746
As long as there is no guest code depending on a particular behaviour if
the chip is write-protected in hardware, we should be free to model
something reasonable and simple.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
2011-07-28 18:26 ` Jan Kiszka
@ 2011-07-28 21:05 ` Jordan Justen
2011-08-11 17:57 ` Jordan Justen
0 siblings, 1 reply; 10+ messages in thread
From: Jordan Justen @ 2011-07-28 21:05 UTC (permalink / raw)
To: Jan Kiszka
Cc: Jordan Justen, Anthony Liguori, qemu-devel@nongnu.org,
Aurelien Jarno
On Thu, Jul 28, 2011 at 11:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-07-27 17:38, Jordan Justen wrote:
>> On Wed, Jul 27, 2011 at 02:30, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2011-07-25 23:34, Jordan Justen wrote:
>>>> Read-only mode is indicated by bdrv_is_read_only
>>>>
>>>> When read-only mode is enabled, no changes will be made
>>>> to the flash image in memory, and no bdrv_write calls will be
>>>> made.
>>>>
>>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>>>> ---
>>>> blockdev.c | 3 +-
>>>> hw/pflash_cfi01.c | 36 ++++++++++++++---------
>>>> hw/pflash_cfi02.c | 82 ++++++++++++++++++++++++++++------------------------
>>>> 3 files changed, 68 insertions(+), 53 deletions(-)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index 0b8d3a4..566a502 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -521,7 +521,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>> /* CDROM is fine for any interface, don't check. */
>>>> ro = 1;
>>>> } else if (ro == 1) {
>>>> - if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
>>>> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
>>>> + type != IF_NONE && type != IF_PFLASH) {
>>>> error_report("readonly not supported by this bus type");
>>>> goto err;
>>>> }
>>>> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
>>>> index 90fdc84..11ac490 100644
>>>> --- a/hw/pflash_cfi01.c
>>>> +++ b/hw/pflash_cfi01.c
>>>> @@ -284,8 +284,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>> TARGET_FMT_plx "\n",
>>>> __func__, offset, pfl->sector_len);
>>>>
>>>> - memset(p + offset, 0xff, pfl->sector_len);
>>>> - pflash_update(pfl, offset, pfl->sector_len);
>>>> + if (!pfl->ro) {
>>>> + memset(p + offset, 0xff, pfl->sector_len);
>>>> + pflash_update(pfl, offset, pfl->sector_len);
>>>> + }
>>>> pfl->status |= 0x80; /* Ready! */
>>>> break;
>>>> case 0x50: /* Clear status bits */
>>>> @@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>> case 0x10: /* Single Byte Program */
>>>> case 0x40: /* Single Byte Program */
>>>> DPRINTF("%s: Single Byte Program\n", __func__);
>>>> - pflash_data_write(pfl, offset, value, width, be);
>>>> - pflash_update(pfl, offset, width);
>>>> + if (!pfl->ro) {
>>>> + pflash_data_write(pfl, offset, value, width, be);
>>>> + pflash_update(pfl, offset, width);
>>>> + }
>>>> pfl->status |= 0x80; /* Ready! */
>>>> pfl->wcycle = 0;
>>>> break;
>>>> @@ -373,7 +377,9 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>> case 2:
>>>> switch (pfl->cmd) {
>>>> case 0xe8: /* Block write */
>>>> - pflash_data_write(pfl, offset, value, width, be);
>>>> + if (!pfl->ro) {
>>>> + pflash_data_write(pfl, offset, value, width, be);
>>>> + }
>>>>
>>>> pfl->status |= 0x80;
>>>>
>>>> @@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>
>>>> DPRINTF("%s: block write finished\n", __func__);
>>>> pfl->wcycle++;
>>>> - /* Flush the entire write buffer onto backing storage. */
>>>> - pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>>> + if (!pfl->ro) {
>>>> + /* Flush the entire write buffer onto backing storage. */
>>>> + pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>>> + }
>>>> }
>>>>
>>>> pfl->counter--;
>>>> @@ -621,13 +629,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
>>>> return NULL;
>>>> }
>>>> }
>>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>>> - * the same way the hardware does (with WP pin).
>>>> - */
>>>> - pfl->ro = 1;
>>>> -#else
>>>> - pfl->ro = 0;
>>>> -#endif
>>>> +
>>>> + if (pfl->bs) {
>>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>>> + } else {
>>>> + pfl->ro = 0;
>>>> + }
>>>> +
>>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>>> pfl->base = base;
>>>> pfl->sector_len = sector_len;
>>>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>>>> index 725cd1e..920209d 100644
>>>> --- a/hw/pflash_cfi02.c
>>>> +++ b/hw/pflash_cfi02.c
>>>> @@ -315,35 +315,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>> DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
>>>> __func__, offset, value, width);
>>>> p = pfl->storage;
>>>> - switch (width) {
>>>> - case 1:
>>>> - p[offset] &= value;
>>>> - pflash_update(pfl, offset, 1);
>>>> - break;
>>>> - case 2:
>>>> - if (be) {
>>>> - p[offset] &= value >> 8;
>>>> - p[offset + 1] &= value;
>>>> - } else {
>>>> + if (!pfl->ro) {
>>>> + switch (width) {
>>>> + case 1:
>>>> p[offset] &= value;
>>>> - p[offset + 1] &= value >> 8;
>>>> + pflash_update(pfl, offset, 1);
>>>> + break;
>>>> + case 2:
>>>> + if (be) {
>>>> + p[offset] &= value >> 8;
>>>> + p[offset + 1] &= value;
>>>> + } else {
>>>> + p[offset] &= value;
>>>> + p[offset + 1] &= value >> 8;
>>>> + }
>>>> + pflash_update(pfl, offset, 2);
>>>> + break;
>>>> + case 4:
>>>> + if (be) {
>>>> + p[offset] &= value >> 24;
>>>> + p[offset + 1] &= value >> 16;
>>>> + p[offset + 2] &= value >> 8;
>>>> + p[offset + 3] &= value;
>>>> + } else {
>>>> + p[offset] &= value;
>>>> + p[offset + 1] &= value >> 8;
>>>> + p[offset + 2] &= value >> 16;
>>>> + p[offset + 3] &= value >> 24;
>>>> + }
>>>> + pflash_update(pfl, offset, 4);
>>>> + break;
>>>> }
>>>> - pflash_update(pfl, offset, 2);
>>>> - break;
>>>> - case 4:
>>>> - if (be) {
>>>> - p[offset] &= value >> 24;
>>>> - p[offset + 1] &= value >> 16;
>>>> - p[offset + 2] &= value >> 8;
>>>> - p[offset + 3] &= value;
>>>> - } else {
>>>> - p[offset] &= value;
>>>> - p[offset + 1] &= value >> 8;
>>>> - p[offset + 2] &= value >> 16;
>>>> - p[offset + 3] &= value >> 24;
>>>> - }
>>>> - pflash_update(pfl, offset, 4);
>>>> - break;
>>>> }
>>>> pfl->status = 0x00 | ~(value & 0x80);
>>>> /* Let's pretend write is immediate */
>>>> @@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>> }
>>>> /* Chip erase */
>>>> DPRINTF("%s: start chip erase\n", __func__);
>>>> - memset(pfl->storage, 0xFF, pfl->chip_len);
>>>> + if (!pfl->ro) {
>>>> + memset(pfl->storage, 0xFF, pfl->chip_len);
>>>> + pflash_update(pfl, 0, pfl->chip_len);
>>>> + }
>>>> 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_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
>>>> @@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>> offset &= ~(pfl->sector_len - 1);
>>>> DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
>>>> offset);
>>>> - memset(p + offset, 0xFF, pfl->sector_len);
>>>> - pflash_update(pfl, offset, pfl->sector_len);
>>>> + if (!pfl->ro) {
>>>> + memset(p + offset, 0xFF, pfl->sector_len);
>>>> + 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,
>>>> @@ -644,13 +650,13 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
>>>> return NULL;
>>>> }
>>>> }
>>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>>> - * the same way the hardware does (with WP pin).
>>>> - */
>>>> - pfl->ro = 1;
>>>> -#else
>>>> - pfl->ro = 0;
>>>> -#endif
>>>> +
>>>> + if (pfl->bs) {
>>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>>> + } else {
>>>> + pfl->ro = 0;
>>>> + }
>>>> +
>>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>>> pfl->sector_len = sector_len;
>>>> pfl->width = width;
>>>
>>> Looks good in general. I'm just wondering if real hw gives any feedback
>>> on writes to flashes in read-only mode or silently ignores them as
>>> above? Or am I missing the feedback bits?
>>
>> I have the same concern.
>>
>> Unfortunately, I don't have access to real hardware to investigate.
>>
>> I tried looking for something in the CFI specification, but I found no
>> guidance there.
>
> I've discussed this with some friends, and it actually seems like there
> is no clean write protection in the "real world". The obvious approach
> to cut the write enable line to the chip also has some ugly side effects
> that we probably don't want to emulate. See e.g.
>
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/100746
>
> As long as there is no guest code depending on a particular behavior if
> the chip is write-protected in hardware, we should be free to model
> something reasonable and simple.
If we want to ensure that no guest code can depend on this behavior,
then it might be safer to force pflash to act as a plain ROM when in
read-only mode. Would this be preferred? (I doubt this would be done
on real CFI hardware. The CFI spec does not seem to indicate an
expected behavior.)
I would be writing some OVMF code to support UEFI variables via pflash
if these changes are committed. And, in that case, I will try to
detect if the flash writes are working.
Thanks,
-Jordan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pc: Support system flash memory with pflash
2011-07-25 21:34 ` [Qemu-devel] [PATCH 2/2] pc: Support system flash memory with pflash Jordan Justen
@ 2011-07-29 13:06 ` Anthony Liguori
0 siblings, 0 replies; 10+ messages in thread
From: Anthony Liguori @ 2011-07-29 13:06 UTC (permalink / raw)
To: Jordan Justen; +Cc: qemu-devel, Aurelien Jarno
On 07/25/2011 04:34 PM, Jordan Justen wrote:
> If a pflash image is found, then it is used for the system
> firmware image.
>
> If a pflash image is not initially found, then a read-only
> pflash device is created using the -bios filename.
>
> Signed-off-by: Jordan Justen<jordan.l.justen@intel.com>
> Cc: Anthony Liguori<aliguori@us.ibm.com>
> Cc: Aurelien Jarno<aurelien@aurel32.net>
> ---
> Makefile.target | 2 +-
> default-configs/i386-softmmu.mak | 1 +
> default-configs/x86_64-softmmu.mak | 1 +
> hw/boards.h | 1 +
> hw/pc.c | 46 +------------
> hw/pc.h | 2 +
> hw/pcflash.c | 130 ++++++++++++++++++++++++++++++++++++
> vl.c | 2 +-
> 8 files changed, 141 insertions(+), 44 deletions(-)
> create mode 100644 hw/pcflash.c
>
> diff --git a/Makefile.target b/Makefile.target
> index cde509b..4d8821f 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -223,7 +223,7 @@ obj-$(CONFIG_IVSHMEM) += ivshmem.o
>
> # Hardware support
> obj-i386-y += vga.o
> -obj-i386-y += mc146818rtc.o i8259.o pc.o
> +obj-i386-y += mc146818rtc.o i8259.o pc.o pcflash.o
This should be able to live in hw-obj-y in Makefile.objs since it
doesn't depend on any TARGET_ defines or CPUState.
> diff --git a/hw/pc.c b/hw/pc.c
> index a3e8539..cf7257b 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -55,10 +55,6 @@
> #define DPRINTF(fmt, ...)
> #endif
>
> -#define BIOS_FILENAME "bios.bin"
> -
> -#define PC_MAX_BIOS_SIZE (4 * 1024 * 1024)
> -
> /* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables. */
> #define ACPI_DATA_SIZE 0x10000
> #define BIOS_CFG_IOPORT 0x510
> @@ -963,10 +959,8 @@ void pc_memory_init(const char *kernel_filename,
> ram_addr_t below_4g_mem_size,
> ram_addr_t above_4g_mem_size)
> {
> - char *filename;
> - int ret, linux_boot, i;
> - ram_addr_t ram_addr, bios_offset, option_rom_offset;
> - int bios_size, isa_bios_size;
> + int linux_boot, i;
> + ram_addr_t ram_addr, option_rom_offset;
> void *fw_cfg;
>
> linux_boot = (kernel_filename != NULL);
> @@ -983,44 +977,12 @@ void pc_memory_init(const char *kernel_filename,
> ram_addr + below_4g_mem_size);
> }
>
> - /* BIOS load */
> - if (bios_name == NULL)
> - bios_name = BIOS_FILENAME;
> - filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> - if (filename) {
> - bios_size = get_image_size(filename);
> - } else {
> - bios_size = -1;
> - }
> - if (bios_size<= 0 ||
> - (bios_size % 65536) != 0) {
> - goto bios_error;
> - }
> - bios_offset = qemu_ram_alloc(NULL, "pc.bios", bios_size);
> - ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
> - if (ret != 0) {
> - bios_error:
> - fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
> - exit(1);
> - }
> - if (filename) {
> - qemu_free(filename);
> - }
> - /* map the last 128KB of the BIOS in ISA space */
> - isa_bios_size = bios_size;
> - if (isa_bios_size> (128 * 1024))
> - isa_bios_size = 128 * 1024;
> - cpu_register_physical_memory(0x100000 - isa_bios_size,
> - isa_bios_size,
> - (bios_offset + bios_size - isa_bios_size) | IO_MEM_ROM);
> + /* Initialize ROM or flash ranges for PC firmware */
> + pc_system_firmware_init();
>
> option_rom_offset = qemu_ram_alloc(NULL, "pc.rom", PC_ROM_SIZE);
> cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
>
> - /* map all the bios at the top of memory */
> - cpu_register_physical_memory((uint32_t)(-bios_size),
> - bios_size, bios_offset | IO_MEM_ROM);
> -
> fw_cfg = bochs_bios_init();
> rom_set_fw(fw_cfg);
>
> diff --git a/hw/pc.h b/hw/pc.h
> index 6d5730b..114816d 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -238,4 +238,6 @@ static inline bool isa_ne2000_init(int base, int irq, NICInfo *nd)
>
> int e820_add_entry(uint64_t, uint64_t, uint32_t);
>
> +void pc_system_firmware_init(void);
> +
> #endif
> diff --git a/hw/pcflash.c b/hw/pcflash.c
> new file mode 100644
> index 0000000..514b1ed
> --- /dev/null
> +++ b/hw/pcflash.c
> @@ -0,0 +1,130 @@
> +/*
> + * QEMU PC System Flash
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + * Copyright (c) 2011 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw.h"
> +#include "pc.h"
> +#include "hw/boards.h"
> +#include "loader.h"
> +#include "sysemu.h"
> +#include "flash.h"
> +
> +#define BIOS_FILENAME "bios.bin"
> +
> +#define PC_MAX_BIOS_SIZE (4 * 1024 * 1024)
> +
> +static void pc_isa_bios_init(ram_addr_t ram_offset, int ram_size)
> +{
> + int isa_bios_size;
> +
> + /* map the last 128KB of the BIOS in ISA space */
> + isa_bios_size = ram_size;
> + if (isa_bios_size> (128 * 1024)) {
> + isa_bios_size = 128 * 1024;
> + }
> + ram_offset = ram_offset + ram_size - isa_bios_size;
> + cpu_register_physical_memory(0x100000 - isa_bios_size,
> + isa_bios_size,
> + ram_offset | IO_MEM_ROM);
> +}
> +
> +static void pc_default_system_flash_init(void)
> +{
> + QemuOpts *opts;
> + QEMUMachine *machine;
> + char *filename;
> +
> + if (bios_name == NULL) {
> + bios_name = BIOS_FILENAME;
> + }
> + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> + fprintf(stderr, "ROM: %s\n", filename);
> +
> + opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
> + if (opts == NULL) {
> + return;
> + }
> +
> + machine = find_default_machine();
> + if (machine == NULL) {
> + return;
> + }
> +
> + drive_init(opts, machine->use_scsi);
> +}
> +
> +static void pc_system_flash_init(DriveInfo *pflash_drv)
> +{
> + BlockDriverState *bdrv;
> + int64_t size;
> + target_phys_addr_t phys_addr;
> + ram_addr_t addr;
> + int sector_bits, sector_size;
> +
> + bdrv = NULL;
> +
> + bdrv = pflash_drv->bdrv;
> + size = bdrv_getlength(pflash_drv->bdrv);
> + sector_bits = 12;
> + sector_size = 1<< sector_bits;
> +
> + if ((size % sector_size) != 0) {
> + fprintf(stderr,
> + "qemu: PC system firmware (pflash) must be a multiple of 0x%x\n",
> + sector_size);
> + exit(1);
> + }
> +
> + phys_addr = 0x100000000ULL - size;
> + addr = qemu_ram_alloc(NULL, "system.flash", size);
> + pflash_cfi01_register(phys_addr, addr, bdrv,
> + sector_size, size>> sector_bits,
> + 4, 0x0000, 0x0000, 0x0000, 0x0000, 0);
> +
> + pc_isa_bios_init(addr, size);
> +}
> +
> +void pc_system_firmware_init(void)
> +{
> + int flash_present;
> + DriveInfo *pflash_drv;
> +
> + pflash_drv = drive_get(IF_PFLASH, 0, 0);
> + flash_present = (pflash_drv != NULL);
> +
> + if (!flash_present) {
> + pc_default_system_flash_init();
> + pflash_drv = drive_get(IF_PFLASH, 0, 0);
> + flash_present = (pflash_drv != NULL);
> + }
> +
> + if (!flash_present) {
> + fprintf(stderr, "qemu: PC system firmware (pflash) not available\n");
> + exit(1);
> + }
> +
> + pc_system_flash_init(pflash_drv);
> +}
> +
This didn't quite turn out how I had hoped but without touching every
machine init, I don't see a better way.
Regards,
Anthony Liguori
> diff --git a/vl.c b/vl.c
> index 4b6688b..00d8fcb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1063,7 +1063,7 @@ static QEMUMachine *find_machine(const char *name)
> return NULL;
> }
>
> -static QEMUMachine *find_default_machine(void)
> +QEMUMachine *find_default_machine(void)
> {
> QEMUMachine *m;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
2011-07-28 21:05 ` Jordan Justen
@ 2011-08-11 17:57 ` Jordan Justen
2011-08-15 23:45 ` Jan Kiszka
0 siblings, 1 reply; 10+ messages in thread
From: Jordan Justen @ 2011-08-11 17:57 UTC (permalink / raw)
To: Jan Kiszka
Cc: Jordan Justen, Anthony Liguori, qemu-devel@nongnu.org,
Aurelien Jarno
On Thu, Jul 28, 2011 at 14:05, Jordan Justen <jljusten@gmail.com> wrote:
> On Thu, Jul 28, 2011 at 11:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-07-27 17:38, Jordan Justen wrote:
>>> On Wed, Jul 27, 2011 at 02:30, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2011-07-25 23:34, Jordan Justen wrote:
>>>>> Read-only mode is indicated by bdrv_is_read_only
>>>>>
>>>>> When read-only mode is enabled, no changes will be made
>>>>> to the flash image in memory, and no bdrv_write calls will be
>>>>> made.
>>>>>
>>>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>>>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>>>>> ---
>>>>> blockdev.c | 3 +-
>>>>> hw/pflash_cfi01.c | 36 ++++++++++++++---------
>>>>> hw/pflash_cfi02.c | 82 ++++++++++++++++++++++++++++------------------------
>>>>> 3 files changed, 68 insertions(+), 53 deletions(-)
>>>>>
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index 0b8d3a4..566a502 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -521,7 +521,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>> /* CDROM is fine for any interface, don't check. */
>>>>> ro = 1;
>>>>> } else if (ro == 1) {
>>>>> - if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
>>>>> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
>>>>> + type != IF_NONE && type != IF_PFLASH) {
>>>>> error_report("readonly not supported by this bus type");
>>>>> goto err;
>>>>> }
>>>>> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
>>>>> index 90fdc84..11ac490 100644
>>>>> --- a/hw/pflash_cfi01.c
>>>>> +++ b/hw/pflash_cfi01.c
>>>>> @@ -284,8 +284,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>> TARGET_FMT_plx "\n",
>>>>> __func__, offset, pfl->sector_len);
>>>>>
>>>>> - memset(p + offset, 0xff, pfl->sector_len);
>>>>> - pflash_update(pfl, offset, pfl->sector_len);
>>>>> + if (!pfl->ro) {
>>>>> + memset(p + offset, 0xff, pfl->sector_len);
>>>>> + pflash_update(pfl, offset, pfl->sector_len);
>>>>> + }
>>>>> pfl->status |= 0x80; /* Ready! */
>>>>> break;
>>>>> case 0x50: /* Clear status bits */
>>>>> @@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>> case 0x10: /* Single Byte Program */
>>>>> case 0x40: /* Single Byte Program */
>>>>> DPRINTF("%s: Single Byte Program\n", __func__);
>>>>> - pflash_data_write(pfl, offset, value, width, be);
>>>>> - pflash_update(pfl, offset, width);
>>>>> + if (!pfl->ro) {
>>>>> + pflash_data_write(pfl, offset, value, width, be);
>>>>> + pflash_update(pfl, offset, width);
>>>>> + }
>>>>> pfl->status |= 0x80; /* Ready! */
>>>>> pfl->wcycle = 0;
>>>>> break;
>>>>> @@ -373,7 +377,9 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>> case 2:
>>>>> switch (pfl->cmd) {
>>>>> case 0xe8: /* Block write */
>>>>> - pflash_data_write(pfl, offset, value, width, be);
>>>>> + if (!pfl->ro) {
>>>>> + pflash_data_write(pfl, offset, value, width, be);
>>>>> + }
>>>>>
>>>>> pfl->status |= 0x80;
>>>>>
>>>>> @@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>
>>>>> DPRINTF("%s: block write finished\n", __func__);
>>>>> pfl->wcycle++;
>>>>> - /* Flush the entire write buffer onto backing storage. */
>>>>> - pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>>>> + if (!pfl->ro) {
>>>>> + /* Flush the entire write buffer onto backing storage. */
>>>>> + pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>>>> + }
>>>>> }
>>>>>
>>>>> pfl->counter--;
>>>>> @@ -621,13 +629,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
>>>>> return NULL;
>>>>> }
>>>>> }
>>>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>>>> - * the same way the hardware does (with WP pin).
>>>>> - */
>>>>> - pfl->ro = 1;
>>>>> -#else
>>>>> - pfl->ro = 0;
>>>>> -#endif
>>>>> +
>>>>> + if (pfl->bs) {
>>>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>>>> + } else {
>>>>> + pfl->ro = 0;
>>>>> + }
>>>>> +
>>>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>>>> pfl->base = base;
>>>>> pfl->sector_len = sector_len;
>>>>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>>>>> index 725cd1e..920209d 100644
>>>>> --- a/hw/pflash_cfi02.c
>>>>> +++ b/hw/pflash_cfi02.c
>>>>> @@ -315,35 +315,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>> DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
>>>>> __func__, offset, value, width);
>>>>> p = pfl->storage;
>>>>> - switch (width) {
>>>>> - case 1:
>>>>> - p[offset] &= value;
>>>>> - pflash_update(pfl, offset, 1);
>>>>> - break;
>>>>> - case 2:
>>>>> - if (be) {
>>>>> - p[offset] &= value >> 8;
>>>>> - p[offset + 1] &= value;
>>>>> - } else {
>>>>> + if (!pfl->ro) {
>>>>> + switch (width) {
>>>>> + case 1:
>>>>> p[offset] &= value;
>>>>> - p[offset + 1] &= value >> 8;
>>>>> + pflash_update(pfl, offset, 1);
>>>>> + break;
>>>>> + case 2:
>>>>> + if (be) {
>>>>> + p[offset] &= value >> 8;
>>>>> + p[offset + 1] &= value;
>>>>> + } else {
>>>>> + p[offset] &= value;
>>>>> + p[offset + 1] &= value >> 8;
>>>>> + }
>>>>> + pflash_update(pfl, offset, 2);
>>>>> + break;
>>>>> + case 4:
>>>>> + if (be) {
>>>>> + p[offset] &= value >> 24;
>>>>> + p[offset + 1] &= value >> 16;
>>>>> + p[offset + 2] &= value >> 8;
>>>>> + p[offset + 3] &= value;
>>>>> + } else {
>>>>> + p[offset] &= value;
>>>>> + p[offset + 1] &= value >> 8;
>>>>> + p[offset + 2] &= value >> 16;
>>>>> + p[offset + 3] &= value >> 24;
>>>>> + }
>>>>> + pflash_update(pfl, offset, 4);
>>>>> + break;
>>>>> }
>>>>> - pflash_update(pfl, offset, 2);
>>>>> - break;
>>>>> - case 4:
>>>>> - if (be) {
>>>>> - p[offset] &= value >> 24;
>>>>> - p[offset + 1] &= value >> 16;
>>>>> - p[offset + 2] &= value >> 8;
>>>>> - p[offset + 3] &= value;
>>>>> - } else {
>>>>> - p[offset] &= value;
>>>>> - p[offset + 1] &= value >> 8;
>>>>> - p[offset + 2] &= value >> 16;
>>>>> - p[offset + 3] &= value >> 24;
>>>>> - }
>>>>> - pflash_update(pfl, offset, 4);
>>>>> - break;
>>>>> }
>>>>> pfl->status = 0x00 | ~(value & 0x80);
>>>>> /* Let's pretend write is immediate */
>>>>> @@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>> }
>>>>> /* Chip erase */
>>>>> DPRINTF("%s: start chip erase\n", __func__);
>>>>> - memset(pfl->storage, 0xFF, pfl->chip_len);
>>>>> + if (!pfl->ro) {
>>>>> + memset(pfl->storage, 0xFF, pfl->chip_len);
>>>>> + pflash_update(pfl, 0, pfl->chip_len);
>>>>> + }
>>>>> 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_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
>>>>> @@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>> offset &= ~(pfl->sector_len - 1);
>>>>> DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
>>>>> offset);
>>>>> - memset(p + offset, 0xFF, pfl->sector_len);
>>>>> - pflash_update(pfl, offset, pfl->sector_len);
>>>>> + if (!pfl->ro) {
>>>>> + memset(p + offset, 0xFF, pfl->sector_len);
>>>>> + 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,
>>>>> @@ -644,13 +650,13 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
>>>>> return NULL;
>>>>> }
>>>>> }
>>>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>>>> - * the same way the hardware does (with WP pin).
>>>>> - */
>>>>> - pfl->ro = 1;
>>>>> -#else
>>>>> - pfl->ro = 0;
>>>>> -#endif
>>>>> +
>>>>> + if (pfl->bs) {
>>>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>>>> + } else {
>>>>> + pfl->ro = 0;
>>>>> + }
>>>>> +
>>>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>>>> pfl->sector_len = sector_len;
>>>>> pfl->width = width;
>>>>
>>>> Looks good in general. I'm just wondering if real hw gives any feedback
>>>> on writes to flashes in read-only mode or silently ignores them as
>>>> above? Or am I missing the feedback bits?
>>>
>>> I have the same concern.
>>>
>>> Unfortunately, I don't have access to real hardware to investigate.
>>>
>>> I tried looking for something in the CFI specification, but I found no
>>> guidance there.
>>
>> I've discussed this with some friends, and it actually seems like there
>> is no clean write protection in the "real world". The obvious approach
>> to cut the write enable line to the chip also has some ugly side effects
>> that we probably don't want to emulate. See e.g.
>>
>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/100746
>>
>> As long as there is no guest code depending on a particular behavior if
>> the chip is write-protected in hardware, we should be free to model
>> something reasonable and simple.
>
> If we want to ensure that no guest code can depend on this behavior,
> then it might be safer to force pflash to act as a plain ROM when in
> read-only mode. Would this be preferred? (I doubt this would be done
> on real CFI hardware. The CFI spec does not seem to indicate an
> expected behavior.)
>
> I would be writing some OVMF code to support UEFI variables via pflash
> if these changes are committed. And, in that case, I will try to
> detect if the flash writes are working.
I found this Intel flash doc with CFI references.
http://download.intel.com/design/archives/flash/docs/29067201.pdf
It seems potentially useful for the status register. (See section
4.4) It does seem to match the pflash_cfi01 code for bit7
indicating 'ready'.
Should I update this patch to set bit4 (error in program) & bit5
(error in block erase)?
-Jordan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
2011-08-11 17:57 ` Jordan Justen
@ 2011-08-15 23:45 ` Jan Kiszka
2011-08-16 0:19 ` Jordan Justen
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2011-08-15 23:45 UTC (permalink / raw)
To: Jordan Justen
Cc: Jordan Justen, Anthony Liguori, qemu-devel@nongnu.org,
Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 12939 bytes --]
On 2011-08-11 10:57, Jordan Justen wrote:
> On Thu, Jul 28, 2011 at 14:05, Jordan Justen <jljusten@gmail.com> wrote:
>> On Thu, Jul 28, 2011 at 11:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2011-07-27 17:38, Jordan Justen wrote:
>>>> On Wed, Jul 27, 2011 at 02:30, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2011-07-25 23:34, Jordan Justen wrote:
>>>>>> Read-only mode is indicated by bdrv_is_read_only
>>>>>>
>>>>>> When read-only mode is enabled, no changes will be made
>>>>>> to the flash image in memory, and no bdrv_write calls will be
>>>>>> made.
>>>>>>
>>>>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>>>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>>>>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>>>>>> ---
>>>>>> blockdev.c | 3 +-
>>>>>> hw/pflash_cfi01.c | 36 ++++++++++++++---------
>>>>>> hw/pflash_cfi02.c | 82 ++++++++++++++++++++++++++++------------------------
>>>>>> 3 files changed, 68 insertions(+), 53 deletions(-)
>>>>>>
>>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>>> index 0b8d3a4..566a502 100644
>>>>>> --- a/blockdev.c
>>>>>> +++ b/blockdev.c
>>>>>> @@ -521,7 +521,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>>> /* CDROM is fine for any interface, don't check. */
>>>>>> ro = 1;
>>>>>> } else if (ro == 1) {
>>>>>> - if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
>>>>>> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
>>>>>> + type != IF_NONE && type != IF_PFLASH) {
>>>>>> error_report("readonly not supported by this bus type");
>>>>>> goto err;
>>>>>> }
>>>>>> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
>>>>>> index 90fdc84..11ac490 100644
>>>>>> --- a/hw/pflash_cfi01.c
>>>>>> +++ b/hw/pflash_cfi01.c
>>>>>> @@ -284,8 +284,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>> TARGET_FMT_plx "\n",
>>>>>> __func__, offset, pfl->sector_len);
>>>>>>
>>>>>> - memset(p + offset, 0xff, pfl->sector_len);
>>>>>> - pflash_update(pfl, offset, pfl->sector_len);
>>>>>> + if (!pfl->ro) {
>>>>>> + memset(p + offset, 0xff, pfl->sector_len);
>>>>>> + pflash_update(pfl, offset, pfl->sector_len);
>>>>>> + }
>>>>>> pfl->status |= 0x80; /* Ready! */
>>>>>> break;
>>>>>> case 0x50: /* Clear status bits */
>>>>>> @@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>> case 0x10: /* Single Byte Program */
>>>>>> case 0x40: /* Single Byte Program */
>>>>>> DPRINTF("%s: Single Byte Program\n", __func__);
>>>>>> - pflash_data_write(pfl, offset, value, width, be);
>>>>>> - pflash_update(pfl, offset, width);
>>>>>> + if (!pfl->ro) {
>>>>>> + pflash_data_write(pfl, offset, value, width, be);
>>>>>> + pflash_update(pfl, offset, width);
>>>>>> + }
>>>>>> pfl->status |= 0x80; /* Ready! */
>>>>>> pfl->wcycle = 0;
>>>>>> break;
>>>>>> @@ -373,7 +377,9 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>> case 2:
>>>>>> switch (pfl->cmd) {
>>>>>> case 0xe8: /* Block write */
>>>>>> - pflash_data_write(pfl, offset, value, width, be);
>>>>>> + if (!pfl->ro) {
>>>>>> + pflash_data_write(pfl, offset, value, width, be);
>>>>>> + }
>>>>>>
>>>>>> pfl->status |= 0x80;
>>>>>>
>>>>>> @@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>>
>>>>>> DPRINTF("%s: block write finished\n", __func__);
>>>>>> pfl->wcycle++;
>>>>>> - /* Flush the entire write buffer onto backing storage. */
>>>>>> - pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>>>>> + if (!pfl->ro) {
>>>>>> + /* Flush the entire write buffer onto backing storage. */
>>>>>> + pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> pfl->counter--;
>>>>>> @@ -621,13 +629,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
>>>>>> return NULL;
>>>>>> }
>>>>>> }
>>>>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>>>>> - * the same way the hardware does (with WP pin).
>>>>>> - */
>>>>>> - pfl->ro = 1;
>>>>>> -#else
>>>>>> - pfl->ro = 0;
>>>>>> -#endif
>>>>>> +
>>>>>> + if (pfl->bs) {
>>>>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>>>>> + } else {
>>>>>> + pfl->ro = 0;
>>>>>> + }
>>>>>> +
>>>>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>>>>> pfl->base = base;
>>>>>> pfl->sector_len = sector_len;
>>>>>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>>>>>> index 725cd1e..920209d 100644
>>>>>> --- a/hw/pflash_cfi02.c
>>>>>> +++ b/hw/pflash_cfi02.c
>>>>>> @@ -315,35 +315,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>>> DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
>>>>>> __func__, offset, value, width);
>>>>>> p = pfl->storage;
>>>>>> - switch (width) {
>>>>>> - case 1:
>>>>>> - p[offset] &= value;
>>>>>> - pflash_update(pfl, offset, 1);
>>>>>> - break;
>>>>>> - case 2:
>>>>>> - if (be) {
>>>>>> - p[offset] &= value >> 8;
>>>>>> - p[offset + 1] &= value;
>>>>>> - } else {
>>>>>> + if (!pfl->ro) {
>>>>>> + switch (width) {
>>>>>> + case 1:
>>>>>> p[offset] &= value;
>>>>>> - p[offset + 1] &= value >> 8;
>>>>>> + pflash_update(pfl, offset, 1);
>>>>>> + break;
>>>>>> + case 2:
>>>>>> + if (be) {
>>>>>> + p[offset] &= value >> 8;
>>>>>> + p[offset + 1] &= value;
>>>>>> + } else {
>>>>>> + p[offset] &= value;
>>>>>> + p[offset + 1] &= value >> 8;
>>>>>> + }
>>>>>> + pflash_update(pfl, offset, 2);
>>>>>> + break;
>>>>>> + case 4:
>>>>>> + if (be) {
>>>>>> + p[offset] &= value >> 24;
>>>>>> + p[offset + 1] &= value >> 16;
>>>>>> + p[offset + 2] &= value >> 8;
>>>>>> + p[offset + 3] &= value;
>>>>>> + } else {
>>>>>> + p[offset] &= value;
>>>>>> + p[offset + 1] &= value >> 8;
>>>>>> + p[offset + 2] &= value >> 16;
>>>>>> + p[offset + 3] &= value >> 24;
>>>>>> + }
>>>>>> + pflash_update(pfl, offset, 4);
>>>>>> + break;
>>>>>> }
>>>>>> - pflash_update(pfl, offset, 2);
>>>>>> - break;
>>>>>> - case 4:
>>>>>> - if (be) {
>>>>>> - p[offset] &= value >> 24;
>>>>>> - p[offset + 1] &= value >> 16;
>>>>>> - p[offset + 2] &= value >> 8;
>>>>>> - p[offset + 3] &= value;
>>>>>> - } else {
>>>>>> - p[offset] &= value;
>>>>>> - p[offset + 1] &= value >> 8;
>>>>>> - p[offset + 2] &= value >> 16;
>>>>>> - p[offset + 3] &= value >> 24;
>>>>>> - }
>>>>>> - pflash_update(pfl, offset, 4);
>>>>>> - break;
>>>>>> }
>>>>>> pfl->status = 0x00 | ~(value & 0x80);
>>>>>> /* Let's pretend write is immediate */
>>>>>> @@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>>> }
>>>>>> /* Chip erase */
>>>>>> DPRINTF("%s: start chip erase\n", __func__);
>>>>>> - memset(pfl->storage, 0xFF, pfl->chip_len);
>>>>>> + if (!pfl->ro) {
>>>>>> + memset(pfl->storage, 0xFF, pfl->chip_len);
>>>>>> + pflash_update(pfl, 0, pfl->chip_len);
>>>>>> + }
>>>>>> 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_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
>>>>>> @@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>>> offset &= ~(pfl->sector_len - 1);
>>>>>> DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
>>>>>> offset);
>>>>>> - memset(p + offset, 0xFF, pfl->sector_len);
>>>>>> - pflash_update(pfl, offset, pfl->sector_len);
>>>>>> + if (!pfl->ro) {
>>>>>> + memset(p + offset, 0xFF, pfl->sector_len);
>>>>>> + 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,
>>>>>> @@ -644,13 +650,13 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
>>>>>> return NULL;
>>>>>> }
>>>>>> }
>>>>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>>>>> - * the same way the hardware does (with WP pin).
>>>>>> - */
>>>>>> - pfl->ro = 1;
>>>>>> -#else
>>>>>> - pfl->ro = 0;
>>>>>> -#endif
>>>>>> +
>>>>>> + if (pfl->bs) {
>>>>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>>>>> + } else {
>>>>>> + pfl->ro = 0;
>>>>>> + }
>>>>>> +
>>>>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>>>>> pfl->sector_len = sector_len;
>>>>>> pfl->width = width;
>>>>>
>>>>> Looks good in general. I'm just wondering if real hw gives any feedback
>>>>> on writes to flashes in read-only mode or silently ignores them as
>>>>> above? Or am I missing the feedback bits?
>>>>
>>>> I have the same concern.
>>>>
>>>> Unfortunately, I don't have access to real hardware to investigate.
>>>>
>>>> I tried looking for something in the CFI specification, but I found no
>>>> guidance there.
>>>
>>> I've discussed this with some friends, and it actually seems like there
>>> is no clean write protection in the "real world". The obvious approach
>>> to cut the write enable line to the chip also has some ugly side effects
>>> that we probably don't want to emulate. See e.g.
>>>
>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/100746
>>>
>>> As long as there is no guest code depending on a particular behavior if
>>> the chip is write-protected in hardware, we should be free to model
>>> something reasonable and simple.
>>
>> If we want to ensure that no guest code can depend on this behavior,
>> then it might be safer to force pflash to act as a plain ROM when in
>> read-only mode. Would this be preferred? (I doubt this would be done
>> on real CFI hardware. The CFI spec does not seem to indicate an
>> expected behavior.)
>>
>> I would be writing some OVMF code to support UEFI variables via pflash
>> if these changes are committed. And, in that case, I will try to
>> detect if the flash writes are working.
>
> I found this Intel flash doc with CFI references.
> http://download.intel.com/design/archives/flash/docs/29067201.pdf
>
> It seems potentially useful for the status register. (See section
> 4.4) It does seem to match the pflash_cfi01 code for bit7
> indicating 'ready'.
>
> Should I update this patch to set bit4 (error in program) & bit5
> (error in block erase)?
The key question is if setting any of those bits could be observed on
real hw as well when whatever kind of physical write protection is
active (probably a cut write-enable line). I just wouldn't try to make
the pflash model smarter than reality.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
2011-08-15 23:45 ` Jan Kiszka
@ 2011-08-16 0:19 ` Jordan Justen
0 siblings, 0 replies; 10+ messages in thread
From: Jordan Justen @ 2011-08-16 0:19 UTC (permalink / raw)
To: Jan Kiszka
Cc: Jordan Justen, Anthony Liguori, qemu-devel@nongnu.org,
Aurelien Jarno
On Mon, Aug 15, 2011 at 16:45, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-08-11 10:57, Jordan Justen wrote:
>> On Thu, Jul 28, 2011 at 14:05, Jordan Justen <jljusten@gmail.com> wrote:
>>> On Thu, Jul 28, 2011 at 11:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2011-07-27 17:38, Jordan Justen wrote:
>>>>> On Wed, Jul 27, 2011 at 02:30, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>> On 2011-07-25 23:34, Jordan Justen wrote:
>>>>>>> Read-only mode is indicated by bdrv_is_read_only
>>>>>>>
>>>>>>> When read-only mode is enabled, no changes will be made
>>>>>>> to the flash image in memory, and no bdrv_write calls will be
>>>>>>> made.
>>>>>>>
>>>>>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>>>>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>>>>>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>>>>>>> ---
>>>>>>> blockdev.c | 3 +-
>>>>>>> hw/pflash_cfi01.c | 36 ++++++++++++++---------
>>>>>>> hw/pflash_cfi02.c | 82 ++++++++++++++++++++++++++++------------------------
>>>>>>> 3 files changed, 68 insertions(+), 53 deletions(-)
>>>>>>>
>>>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>>>> index 0b8d3a4..566a502 100644
>>>>>>> --- a/blockdev.c
>>>>>>> +++ b/blockdev.c
>>>>>>> @@ -521,7 +521,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>>>> /* CDROM is fine for any interface, don't check. */
>>>>>>> ro = 1;
>>>>>>> } else if (ro == 1) {
>>>>>>> - if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
>>>>>>> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
>>>>>>> + type != IF_NONE && type != IF_PFLASH) {
>>>>>>> error_report("readonly not supported by this bus type");
>>>>>>> goto err;
>>>>>>> }
>>>>>>> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
>>>>>>> index 90fdc84..11ac490 100644
>>>>>>> --- a/hw/pflash_cfi01.c
>>>>>>> +++ b/hw/pflash_cfi01.c
>>>>>>> @@ -284,8 +284,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>>> TARGET_FMT_plx "\n",
>>>>>>> __func__, offset, pfl->sector_len);
>>>>>>>
>>>>>>> - memset(p + offset, 0xff, pfl->sector_len);
>>>>>>> - pflash_update(pfl, offset, pfl->sector_len);
>>>>>>> + if (!pfl->ro) {
>>>>>>> + memset(p + offset, 0xff, pfl->sector_len);
>>>>>>> + pflash_update(pfl, offset, pfl->sector_len);
>>>>>>> + }
>>>>>>> pfl->status |= 0x80; /* Ready! */
>>>>>>> break;
>>>>>>> case 0x50: /* Clear status bits */
>>>>>>> @@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>>> case 0x10: /* Single Byte Program */
>>>>>>> case 0x40: /* Single Byte Program */
>>>>>>> DPRINTF("%s: Single Byte Program\n", __func__);
>>>>>>> - pflash_data_write(pfl, offset, value, width, be);
>>>>>>> - pflash_update(pfl, offset, width);
>>>>>>> + if (!pfl->ro) {
>>>>>>> + pflash_data_write(pfl, offset, value, width, be);
>>>>>>> + pflash_update(pfl, offset, width);
>>>>>>> + }
>>>>>>> pfl->status |= 0x80; /* Ready! */
>>>>>>> pfl->wcycle = 0;
>>>>>>> break;
>>>>>>> @@ -373,7 +377,9 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>>> case 2:
>>>>>>> switch (pfl->cmd) {
>>>>>>> case 0xe8: /* Block write */
>>>>>>> - pflash_data_write(pfl, offset, value, width, be);
>>>>>>> + if (!pfl->ro) {
>>>>>>> + pflash_data_write(pfl, offset, value, width, be);
>>>>>>> + }
>>>>>>>
>>>>>>> pfl->status |= 0x80;
>>>>>>>
>>>>>>> @@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
>>>>>>>
>>>>>>> DPRINTF("%s: block write finished\n", __func__);
>>>>>>> pfl->wcycle++;
>>>>>>> - /* Flush the entire write buffer onto backing storage. */
>>>>>>> - pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>>>>>> + if (!pfl->ro) {
>>>>>>> + /* Flush the entire write buffer onto backing storage. */
>>>>>>> + pflash_update(pfl, offset & mask, pfl->writeblock_size);
>>>>>>> + }
>>>>>>> }
>>>>>>>
>>>>>>> pfl->counter--;
>>>>>>> @@ -621,13 +629,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base, ram_addr_t off,
>>>>>>> return NULL;
>>>>>>> }
>>>>>>> }
>>>>>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>>>>>> - * the same way the hardware does (with WP pin).
>>>>>>> - */
>>>>>>> - pfl->ro = 1;
>>>>>>> -#else
>>>>>>> - pfl->ro = 0;
>>>>>>> -#endif
>>>>>>> +
>>>>>>> + if (pfl->bs) {
>>>>>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>>>>>> + } else {
>>>>>>> + pfl->ro = 0;
>>>>>>> + }
>>>>>>> +
>>>>>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>>>>>> pfl->base = base;
>>>>>>> pfl->sector_len = sector_len;
>>>>>>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>>>>>>> index 725cd1e..920209d 100644
>>>>>>> --- a/hw/pflash_cfi02.c
>>>>>>> +++ b/hw/pflash_cfi02.c
>>>>>>> @@ -315,35 +315,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>>>> DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
>>>>>>> __func__, offset, value, width);
>>>>>>> p = pfl->storage;
>>>>>>> - switch (width) {
>>>>>>> - case 1:
>>>>>>> - p[offset] &= value;
>>>>>>> - pflash_update(pfl, offset, 1);
>>>>>>> - break;
>>>>>>> - case 2:
>>>>>>> - if (be) {
>>>>>>> - p[offset] &= value >> 8;
>>>>>>> - p[offset + 1] &= value;
>>>>>>> - } else {
>>>>>>> + if (!pfl->ro) {
>>>>>>> + switch (width) {
>>>>>>> + case 1:
>>>>>>> p[offset] &= value;
>>>>>>> - p[offset + 1] &= value >> 8;
>>>>>>> + pflash_update(pfl, offset, 1);
>>>>>>> + break;
>>>>>>> + case 2:
>>>>>>> + if (be) {
>>>>>>> + p[offset] &= value >> 8;
>>>>>>> + p[offset + 1] &= value;
>>>>>>> + } else {
>>>>>>> + p[offset] &= value;
>>>>>>> + p[offset + 1] &= value >> 8;
>>>>>>> + }
>>>>>>> + pflash_update(pfl, offset, 2);
>>>>>>> + break;
>>>>>>> + case 4:
>>>>>>> + if (be) {
>>>>>>> + p[offset] &= value >> 24;
>>>>>>> + p[offset + 1] &= value >> 16;
>>>>>>> + p[offset + 2] &= value >> 8;
>>>>>>> + p[offset + 3] &= value;
>>>>>>> + } else {
>>>>>>> + p[offset] &= value;
>>>>>>> + p[offset + 1] &= value >> 8;
>>>>>>> + p[offset + 2] &= value >> 16;
>>>>>>> + p[offset + 3] &= value >> 24;
>>>>>>> + }
>>>>>>> + pflash_update(pfl, offset, 4);
>>>>>>> + break;
>>>>>>> }
>>>>>>> - pflash_update(pfl, offset, 2);
>>>>>>> - break;
>>>>>>> - case 4:
>>>>>>> - if (be) {
>>>>>>> - p[offset] &= value >> 24;
>>>>>>> - p[offset + 1] &= value >> 16;
>>>>>>> - p[offset + 2] &= value >> 8;
>>>>>>> - p[offset + 3] &= value;
>>>>>>> - } else {
>>>>>>> - p[offset] &= value;
>>>>>>> - p[offset + 1] &= value >> 8;
>>>>>>> - p[offset + 2] &= value >> 16;
>>>>>>> - p[offset + 3] &= value >> 24;
>>>>>>> - }
>>>>>>> - pflash_update(pfl, offset, 4);
>>>>>>> - break;
>>>>>>> }
>>>>>>> pfl->status = 0x00 | ~(value & 0x80);
>>>>>>> /* Let's pretend write is immediate */
>>>>>>> @@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>>>> }
>>>>>>> /* Chip erase */
>>>>>>> DPRINTF("%s: start chip erase\n", __func__);
>>>>>>> - memset(pfl->storage, 0xFF, pfl->chip_len);
>>>>>>> + if (!pfl->ro) {
>>>>>>> + memset(pfl->storage, 0xFF, pfl->chip_len);
>>>>>>> + pflash_update(pfl, 0, pfl->chip_len);
>>>>>>> + }
>>>>>>> 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_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
>>>>>>> @@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>>>>>>> offset &= ~(pfl->sector_len - 1);
>>>>>>> DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
>>>>>>> offset);
>>>>>>> - memset(p + offset, 0xFF, pfl->sector_len);
>>>>>>> - pflash_update(pfl, offset, pfl->sector_len);
>>>>>>> + if (!pfl->ro) {
>>>>>>> + memset(p + offset, 0xFF, pfl->sector_len);
>>>>>>> + 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,
>>>>>>> @@ -644,13 +650,13 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base, ram_addr_t off,
>>>>>>> return NULL;
>>>>>>> }
>>>>>>> }
>>>>>>> -#if 0 /* XXX: there should be a bit to set up read-only,
>>>>>>> - * the same way the hardware does (with WP pin).
>>>>>>> - */
>>>>>>> - pfl->ro = 1;
>>>>>>> -#else
>>>>>>> - pfl->ro = 0;
>>>>>>> -#endif
>>>>>>> +
>>>>>>> + if (pfl->bs) {
>>>>>>> + pfl->ro = bdrv_is_read_only(pfl->bs);
>>>>>>> + } else {
>>>>>>> + pfl->ro = 0;
>>>>>>> + }
>>>>>>> +
>>>>>>> pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>>>>>>> pfl->sector_len = sector_len;
>>>>>>> pfl->width = width;
>>>>>>
>>>>>> Looks good in general. I'm just wondering if real hw gives any feedback
>>>>>> on writes to flashes in read-only mode or silently ignores them as
>>>>>> above? Or am I missing the feedback bits?
>>>>>
>>>>> I have the same concern.
>>>>>
>>>>> Unfortunately, I don't have access to real hardware to investigate.
>>>>>
>>>>> I tried looking for something in the CFI specification, but I found no
>>>>> guidance there.
>>>>
>>>> I've discussed this with some friends, and it actually seems like there
>>>> is no clean write protection in the "real world". The obvious approach
>>>> to cut the write enable line to the chip also has some ugly side effects
>>>> that we probably don't want to emulate. See e.g.
>>>>
>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/100746
>>>>
>>>> As long as there is no guest code depending on a particular behavior if
>>>> the chip is write-protected in hardware, we should be free to model
>>>> something reasonable and simple.
>>>
>>> If we want to ensure that no guest code can depend on this behavior,
>>> then it might be safer to force pflash to act as a plain ROM when in
>>> read-only mode. Would this be preferred? (I doubt this would be done
>>> on real CFI hardware. The CFI spec does not seem to indicate an
>>> expected behavior.)
>>>
>>> I would be writing some OVMF code to support UEFI variables via pflash
>>> if these changes are committed. And, in that case, I will try to
>>> detect if the flash writes are working.
>>
>> I found this Intel flash doc with CFI references.
>> http://download.intel.com/design/archives/flash/docs/29067201.pdf
>>
>> It seems potentially useful for the status register. (See section
>> 4.4) It does seem to match the pflash_cfi01 code for bit7
>> indicating 'ready'.
>>
>> Should I update this patch to set bit4 (error in program) & bit5
>> (error in block erase)?
>
> The key question is if setting any of those bits could be observed on
> real hw as well when whatever kind of physical write protection is
> active (probably a cut write-enable line). I just wouldn't try to make
> the pflash model smarter than reality.
I looked closer at the datasheet that I'd linked, and it seemed pretty
clear that SR.1 (bit1) of the status register should be set when
trying to unlock a block if the WP pin is pulled.
I'm not sure what device we are trying to emulate here, so it is
difficult to know what to do. (Checking the history on
hw/pflash_cfi01.c didn't help either.)
But, I did also check the Intel firmware hub datasheet, which does not
mention CFI, but is very similar in its programming model.
http://download.intel.com/design/chipsets/datashts/29065804.pdf
This datasheet has the same definition for the status register, which
indicates to me that setting the SR.1 is most likely the right thing
to do.
But, what about cfi02 (AMD)? Ugh.
-Jordan
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-08-16 0:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-25 21:34 [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode Jordan Justen
2011-07-25 21:34 ` [Qemu-devel] [PATCH 2/2] pc: Support system flash memory with pflash Jordan Justen
2011-07-29 13:06 ` Anthony Liguori
2011-07-27 9:30 ` [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode Jan Kiszka
2011-07-27 15:38 ` Jordan Justen
2011-07-28 18:26 ` Jan Kiszka
2011-07-28 21:05 ` Jordan Justen
2011-08-11 17:57 ` Jordan Justen
2011-08-15 23:45 ` Jan Kiszka
2011-08-16 0:19 ` Jordan Justen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).