[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode
From: |
Jordan Justen |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode |
Date: |
Thu, 11 Aug 2011 10:57:23 -0700 |
On Thu, Jul 28, 2011 at 14:05, Jordan Justen <address@hidden> wrote:
> On Thu, Jul 28, 2011 at 11:26, Jan Kiszka <address@hidden> wrote:
>> 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 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.
I found this Intel flash doc with CFI references.
http://download.intel.com/design/archives/flash/docs/29067201.pdf
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'.
Should I update this patch to set bit4 (error in program) & bit5
(error in block erase)?
-Jordan
- Re: [Qemu-devel] [PATCH 1/2] pflash: Support read-only mode,
Jordan Justen <=