On Thu, Jul 28, 2011 at 14:05, Jordan Justen <jljus...@gmail.com> wrote: > On Thu, Jul 28, 2011 at 11:26, Jan Kiszka <jan.kis...@siemens.com> wrote: >> On 2011-07-27 17:38, Jordan Justen wrote: >>> On Wed, Jul 27, 2011 at 02:30, Jan Kiszka <jan.kis...@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.jus...@intel.com> >>>>> Cc: Jan Kiszka <jan.kis...@siemens.com> >>>>> Cc: Aurelien Jarno <aurel...@aurel32.net> >>>>> Cc: Anthony Liguori <aligu...@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