From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33040) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qt6qR-00061z-Us for qemu-devel@nongnu.org; Mon, 15 Aug 2011 19:45:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qt6qQ-0006tC-2n for qemu-devel@nongnu.org; Mon, 15 Aug 2011 19:45:23 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]:49382) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qt6qP-0006st-CX for qemu-devel@nongnu.org; Mon, 15 Aug 2011 19:45:22 -0400 Message-ID: <4E49AF86.5050709@web.de> Date: Mon, 15 Aug 2011 16:45:10 -0700 From: Jan Kiszka MIME-Version: 1.0 References: <1311629692-5734-1-git-send-email-jordan.l.justen@intel.com> <4E2FDAB0.8080404@siemens.com> <4E31A9D1.6020509@siemens.com> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig1B8AB1F18D95FAE5FAFDA96C" Sender: jan.kiszka@web.de 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: Jordan Justen , Anthony Liguori , "qemu-devel@nongnu.org" , Aurelien Jarno This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig1B8AB1F18D95FAE5FAFDA96C Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2011-08-11 10:57, Jordan Justen wrote: > On Thu, Jul 28, 2011 at 14:05, Jordan Justen wrote= : >> On Thu, Jul 28, 2011 at 11:26, Jan Kiszka wro= te: >>> On 2011-07-27 17:38, Jordan Justen wrote: >>>> On Wed, Jul 27, 2011 at 02:30, Jan Kiszka w= rote: >>>>> 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 defa= ult_to_scsi) >>>>>> /* CDROM is fine for any interface, don't check. */ >>>>>> ro =3D 1; >>>>>> } else if (ro =3D=3D 1) { >>>>>> - if (type !=3D IF_SCSI && type !=3D IF_VIRTIO && type !=3D= IF_FLOPPY && type !=3D IF_NONE) { >>>>>> + if (type !=3D IF_SCSI && type !=3D IF_VIRTIO && type !=3D= IF_FLOPPY && >>>>>> + type !=3D IF_NONE && type !=3D 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, targe= t_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 |=3D 0x80; /* Ready! */ >>>>>> break; >>>>>> case 0x50: /* Clear status bits */ >>>>>> @@ -324,8 +326,10 @@ static void pflash_write(pflash_t *pfl, targe= t_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 |=3D 0x80; /* Ready! */ >>>>>> pfl->wcycle =3D 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 |=3D 0x80; >>>>>> >>>>>> @@ -383,8 +389,10 @@ static void pflash_write(pflash_t *pfl, targe= t_phys_addr_t offset, >>>>>> >>>>>> DPRINTF("%s: block write finished\n", __func__); >>>>>> pfl->wcycle++; >>>>>> - /* Flush the entire write buffer onto backing sto= rage. */ >>>>>> - 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->writeb= lock_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 =3D 1; >>>>>> -#else >>>>>> - pfl->ro =3D 0; >>>>>> -#endif >>>>>> + >>>>>> + if (pfl->bs) { >>>>>> + pfl->ro =3D bdrv_is_read_only(pfl->bs); >>>>>> + } else { >>>>>> + pfl->ro =3D 0; >>>>>> + } >>>>>> + >>>>>> pfl->timer =3D qemu_new_timer_ns(vm_clock, pflash_timer, pfl)= ; >>>>>> pfl->base =3D base; >>>>>> pfl->sector_len =3D 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, tar= get_phys_addr_t offset, >>>>>> DPRINTF("%s: write data offset " TARGET_FMT_plx " %08= x %d\n", >>>>>> __func__, offset, value, width); >>>>>> p =3D pfl->storage; >>>>>> - switch (width) { >>>>>> - case 1: >>>>>> - p[offset] &=3D value; >>>>>> - pflash_update(pfl, offset, 1); >>>>>> - break; >>>>>> - case 2: >>>>>> - if (be) { >>>>>> - p[offset] &=3D value >> 8; >>>>>> - p[offset + 1] &=3D value; >>>>>> - } else { >>>>>> + if (!pfl->ro) { >>>>>> + switch (width) { >>>>>> + case 1: >>>>>> p[offset] &=3D value; >>>>>> - p[offset + 1] &=3D value >> 8; >>>>>> + pflash_update(pfl, offset, 1); >>>>>> + break; >>>>>> + case 2: >>>>>> + if (be) { >>>>>> + p[offset] &=3D value >> 8; >>>>>> + p[offset + 1] &=3D value; >>>>>> + } else { >>>>>> + p[offset] &=3D value; >>>>>> + p[offset + 1] &=3D value >> 8; >>>>>> + } >>>>>> + pflash_update(pfl, offset, 2); >>>>>> + break; >>>>>> + case 4: >>>>>> + if (be) { >>>>>> + p[offset] &=3D value >> 24; >>>>>> + p[offset + 1] &=3D value >> 16; >>>>>> + p[offset + 2] &=3D value >> 8; >>>>>> + p[offset + 3] &=3D value; >>>>>> + } else { >>>>>> + p[offset] &=3D value; >>>>>> + p[offset + 1] &=3D value >> 8; >>>>>> + p[offset + 2] &=3D value >> 16; >>>>>> + p[offset + 3] &=3D value >> 24; >>>>>> + } >>>>>> + pflash_update(pfl, offset, 4); >>>>>> + break; >>>>>> } >>>>>> - pflash_update(pfl, offset, 2); >>>>>> - break; >>>>>> - case 4: >>>>>> - if (be) { >>>>>> - p[offset] &=3D value >> 24; >>>>>> - p[offset + 1] &=3D value >> 16; >>>>>> - p[offset + 2] &=3D value >> 8; >>>>>> - p[offset + 3] &=3D value; >>>>>> - } else { >>>>>> - p[offset] &=3D value; >>>>>> - p[offset + 1] &=3D value >> 8; >>>>>> - p[offset + 2] &=3D value >> 16; >>>>>> - p[offset + 3] &=3D value >> 24; >>>>>> - } >>>>>> - pflash_update(pfl, offset, 4); >>>>>> - break; >>>>>> } >>>>>> pfl->status =3D 0x00 | ~(value & 0x80); >>>>>> /* Let's pretend write is immediate */ >>>>>> @@ -389,9 +391,11 @@ static void pflash_write (pflash_t *pfl, targ= et_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 =3D 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_tic= ks_per_sec() * 5)); >>>>>> @@ -402,8 +406,10 @@ static void pflash_write (pflash_t *pfl, targ= et_phys_addr_t offset, >>>>>> offset &=3D ~(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 =3D 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 =3D 1; >>>>>> -#else >>>>>> - pfl->ro =3D 0; >>>>>> -#endif >>>>>> + >>>>>> + if (pfl->bs) { >>>>>> + pfl->ro =3D bdrv_is_read_only(pfl->bs); >>>>>> + } else { >>>>>> + pfl->ro =3D 0; >>>>>> + } >>>>>> + >>>>>> pfl->timer =3D qemu_new_timer_ns(vm_clock, pflash_timer, pfl)= ; >>>>>> pfl->sector_len =3D sector_len; >>>>>> pfl->width =3D width; >>>>> >>>>> Looks good in general. I'm just wondering if real hw gives any feed= back >>>>> 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 the= re >>> is no clean write protection in the "real world". The obvious approac= h >>> to cut the write enable line to the chip also has some ugly side effe= cts >>> 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. >=20 > I found this Intel flash doc with CFI references. > http://download.intel.com/design/archives/flash/docs/29067201.pdf >=20 > 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'. >=20 > 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 --------------enig1B8AB1F18D95FAE5FAFDA96C Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk5Jr40ACgkQitSsb3rl5xQoKwCfQWFq1fhQBm7stayY2rakUzr9 8O0AoJjLo2K7lPm9/VqnJU8rT7Ctso0p =Tc+z -----END PGP SIGNATURE----- --------------enig1B8AB1F18D95FAE5FAFDA96C--