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

[Jordan Justen]

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.

-Jordan