[Qemu-devel] [PATCH-for-4.1] Revert "hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit"

Philippe Mathieu-Daudé posted 1 patch 4 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190705161144.18533-1-philmd@redhat.com
Test docker-clang@ubuntu passed
Test s390x passed
Test asan passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test checkpatch passed
Maintainers: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>
hw/block/pflash_cfi02.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH-for-4.1] Revert "hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit"
Posted by Philippe Mathieu-Daudé 4 years, 9 months ago
Stephen Checkoway noticed commit 3ae0343db69 is incorrect.
This commit state all parallel flashes are limited to 16-bit
accesses, however the x32 configuration exists in some models,
such the Cypress S29CL032J, which CFI Device Geometry Definition
announces:

  CFI ADDR     DATA
  0x28,0x29 = 0x0003 (x32-only asynchronous interface)

Guests should not be affected by the previous change, because
QEMU does not announce itself as x32 capable:

    /* Flash device interface (8 & 16 bits) */
    pfl->cfi_table[0x28] = 0x02;
    pfl->cfi_table[0x29] = 0x00;

Commit 3ae0343db69 does not restrict the bus to 16-bit accesses,
but restrict the implementation as 16-bit access max, so a guest
32-bit access will result in 2x 16-bit calls.

Now, we have 2 boards that register the flash device in 32-bit
access:

- PPC: taihu_405ep

  The CFI id matches the S29AL008J that is a 1MB in x16, while
  the code QEMU forces it to be 2MB, and checking Linux it expects
  a 4MB flash.

- ARM: Digic4

  While the comment says "Samsung K8P3215UQB 64M Bit (4Mx16)",
  this flash is 32Mb (2MB). Also note the CFI id does not match
  the comment.

To avoid unexpected side effect, we revert commit 3ae0343db69,
and will clean the board code later.

Reported-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi02.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 5392290c72..83084b9d72 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -317,6 +317,8 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
     boff = offset & 0xFF;
     if (pfl->width == 2) {
         boff = boff >> 1;
+    } else if (pfl->width == 4) {
+        boff = boff >> 2;
     }
     switch (pfl->cmd) {
     default:
@@ -447,6 +449,8 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
     boff = offset;
     if (pfl->width == 2) {
         boff = boff >> 1;
+    } else if (pfl->width == 4) {
+        boff = boff >> 2;
     }
     /* Only the least-significant 11 bits are used in most cases. */
     boff &= 0x7FF;
@@ -706,7 +710,6 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
 static const MemoryRegionOps pflash_cfi02_ops = {
     .read = pflash_read,
     .write = pflash_write,
-    .impl.max_access_size = 2,
     .valid.min_access_size = 1,
     .valid.max_access_size = 4,
     .endianness = DEVICE_NATIVE_ENDIAN,
-- 
2.20.1


Re: [Qemu-devel] [PATCH-for-4.1] Revert "hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit"
Posted by Markus Armbruster 4 years, 9 months ago
Your subject matches the one git-revert creates (good), but you don't
have the rest of its commit message:

  This reverts commit 3ae0343db69c379beb5750b4ed70794bbed51b85.

Please add that line.

The patch is a clean revert.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH-for-4.1] Revert "hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit"
Posted by Philippe Mathieu-Daudé 4 years, 9 months ago
On 7/9/19 1:34 PM, Markus Armbruster wrote:
> Your subject matches the one git-revert creates (good), but you don't

I was wondering whether rewrite it as "Restore 32-bit I/O accesses",
keeping the original subject is better?

> have the rest of its commit message:
> 
>   This reverts commit 3ae0343db69c379beb5750b4ed70794bbed51b85.
> 
> Please add that line.

OK.

> The patch is a clean revert.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks!

Re: [Qemu-devel] [PATCH-for-4.1] Revert "hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit"
Posted by Markus Armbruster 4 years, 9 months ago
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/9/19 1:34 PM, Markus Armbruster wrote:
>> Your subject matches the one git-revert creates (good), but you don't
>
> I was wondering whether rewrite it as "Restore 32-bit I/O accesses",
> keeping the original subject is better?

I recommend to start with git-revert, resolve the conflicts if any, and
append appropriate detail (rationale!) to git-revert's commit message.
That way, people immediately see it's a revert, and of what.

[...]