From: Jan Kiszka <jan.kiszka@siemens.com>
To: Jordan Justen <jordan.l.justen@intel.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
Date: Wed, 27 Jul 2011 11:30:24 +0200 [thread overview]
Message-ID: <4E2FDAB0.8080404@siemens.com> (raw)
In-Reply-To: <1311629692-5734-1-git-send-email-jordan.l.justen@intel.com>
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
next prev parent reply other threads:[~2011-07-27 9:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Jan Kiszka [this message]
2011-07-27 15:38 ` [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4E2FDAB0.8080404@siemens.com \
--to=jan.kiszka@siemens.com \
--cc=aliguori@us.ibm.com \
--cc=aurelien@aurel32.net \
--cc=jordan.l.justen@intel.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.