[PULL,59/62] hw/block/pflash_cfi0{1, 2}: Error out if device length isn't a power of two

Commit Message

Daniel Henrique Barboza Oct. 28, 2022, 4:39 p.m. UTC

From: Bernhard Beschow <[e-mail]>

According to the JEDEC standard the device length is communicated to an
OS as an exponent (power of two).

Signed-off-by: Bernhard Beschow <[e-mail]>
Reviewed-by: Bin Meng <[e-mail]>
Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
Message-Id: <[e-mail]>
Signed-off-by: Daniel Henrique Barboza <[e-mail]>
---
 hw/block/pflash_cfi01.c | 8 ++++++--
 hw/block/pflash_cfi02.c | 5 +++++
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Stefan Hajnoczi Nov. 1, 2022, 10:23 p.m. UTC | #1

There is a report that this commit breaks an existing OVMF setup:
https://gitlab.com/qemu-project/qemu/-/issues/1290#note_1156507334

I'm not familiar with pflash. Please find a way to avoid a regression
in QEMU 7.2 here.

Thank you!

Stefan

Philippe Mathieu-Daudé Nov. 1, 2022, 10:49 p.m. UTC | #2

On 1/11/22 23:23, Stefan Hajnoczi wrote:
> There is a report that this commit breaks an existing OVMF setup:
> https://gitlab.com/qemu-project/qemu/-/issues/1290#note_1156507334
> 
> I'm not familiar with pflash. Please find a way to avoid a regression
> in QEMU 7.2 here.

Long-standing problem with pflash and underlying images... i.e:
https://lore.kernel.org/qemu-devel/[e-mail]/

Let's revert for 7.2. Daniel, I can prepare a patch explaining.

Daniel Henrique Barboza Nov. 2, 2022, 7:49 p.m. UTC | #3

On 11/1/22 19:49, Philippe Mathieu-Daudé wrote:
> On 1/11/22 23:23, Stefan Hajnoczi wrote:
>> There is a report that this commit breaks an existing OVMF setup:
>> https://gitlab.com/qemu-project/qemu/-/issues/1290#note_1156507334
>>
>> I'm not familiar with pflash. Please find a way to avoid a regression
>> in QEMU 7.2 here.
> 
> Long-standing problem with pflash and underlying images... i.e:
> https://lore.kernel.org/qemu-devel/[e-mail]/
> 
> Let's revert for 7.2. Daniel, I can prepare a patch explaining.

I appreciate if you can send a revert with the proper explanation. I can make
a PR with it.


Daniel

Daniel Henrique Barboza Nov. 8, 2022, 5:36 p.m. UTC | #4

Phil,

On 11/1/22 19:49, Philippe Mathieu-Daudé wrote:
> On 1/11/22 23:23, Stefan Hajnoczi wrote:
>> There is a report that this commit breaks an existing OVMF setup:
>> https://gitlab.com/qemu-project/qemu/-/issues/1290#note_1156507334
>>
>> I'm not familiar with pflash. Please find a way to avoid a regression
>> in QEMU 7.2 here.
> 
> Long-standing problem with pflash and underlying images... i.e:
> https://lore.kernel.org/qemu-devel/[e-mail]/
> 
> Let's revert for 7.2. Daniel, I can prepare a patch explaining.

Just sent a revert. I'm not sure if the explanation I provided is
good enough. I appreciate if you can review it.

If it's plausible I'll send a pull request ASAP.


Thanks,

Daniel

Patch

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 0cbc2fb4cb..9c235bf66e 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -690,7 +690,7 @@  static const MemoryRegionOps pflash_cfi01_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void pflash_cfi01_fill_cfi_table(PFlashCFI01 *pfl)
+static void pflash_cfi01_fill_cfi_table(PFlashCFI01 *pfl, Error **errp)
 {
     uint64_t blocks_per_device, sector_len_per_device, device_len;
     int num_devices;
@@ -708,6 +708,10 @@  static void pflash_cfi01_fill_cfi_table(PFlashCFI01 *pfl)
         sector_len_per_device = pfl->sector_len / num_devices;
     }
     device_len = sector_len_per_device * blocks_per_device;
+    if (!is_power_of_2(device_len)) {
+        error_setg(errp, "Device size must be a power of two.");
+        return;
+    }
 
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
@@ -865,7 +869,7 @@  static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
      */
     pfl->cmd = 0x00;
     pfl->status = 0x80; /* WSM ready */
-    pflash_cfi01_fill_cfi_table(pfl);
+    pflash_cfi01_fill_cfi_table(pfl, errp);
 }
 
 static void pflash_cfi01_system_reset(DeviceState *dev)
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 2a99b286b0..ff2fe154c1 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -880,6 +880,11 @@  static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (!is_power_of_2(pfl->chip_len)) {
+        error_setg(errp, "Device size must be a power of two.");
+        return;
+    }
+
     memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
                                   &pflash_cfi02_ops, pfl, pfl->name,
                                   pfl->chip_len, errp);