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.
Thanks,
-Jordan
