From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51690) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qm0Rv-0007Xo-Av for qemu-devel@nongnu.org; Wed, 27 Jul 2011 05:30:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qm0Rt-00022A-Ny for qemu-devel@nongnu.org; Wed, 27 Jul 2011 05:30:43 -0400 Received: from david.siemens.de ([192.35.17.14]:23793) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qm0Rt-000224-5B for qemu-devel@nongnu.org; Wed, 27 Jul 2011 05:30:41 -0400 Message-ID: <4E2FDAB0.8080404@siemens.com> Date: Wed, 27 Jul 2011 11:30:24 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1311629692-5734-1-git-send-email-jordan.l.justen@intel.com> In-Reply-To: <1311629692-5734-1-git-send-email-jordan.l.justen@intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 > Cc: Jan Kiszka > Cc: Aurelien Jarno > Cc: Anthony Liguori > --- > 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