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 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).