Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode

[Jan Kiszka]

On 2011-07-27 17:38, Jordan Justen wrote:
> On Wed, Jul 27, 2011 at 02:30, Jan Kiszka <address@hidden> 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 <address@hidden>
>>> Cc: Jan Kiszka <address@hidden>
>>> Cc: Aurelien Jarno <address@hidden>
>>> Cc: Anthony Liguori <address@hidden>
>>> ---
>>>  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 behaviour if
the chip is write-protected in hardware, we should be free to model
something reasonable and simple.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux