Re: [Qemu-block] [QEMU PATCH] MAINTAINERS: Downgrade status of block sections without "M:" to "Odd Fixes"

2019-05-05 Thread Niels de Vos 
On Mon, May 06, 2019 at 08:18:54AM +0200, Thomas Huth wrote:
> Fixes might still get picked up via the qemu-block mailing list,
> so the status is not "Orphan" yet.
> Also add the gluster mailing list as suggested by Niels here:
> 
>  https://patchwork.kernel.org/patch/10613297/#22409943
> 
> Signed-off-by: Thomas Huth 

Thanks, this counts for the Gluster part:
Reviewed-by: Niels de Vos 


> ---
>  MAINTAINERS | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 66ddbda9c9..899a4cd572 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2404,12 +2404,13 @@ F: block/ssh.c
>  
>  CURL
>  L: qemu-block@nongnu.org
> -S: Supported
> +S: Odd Fixes
>  F: block/curl.c
>  
>  GLUSTER
>  L: qemu-block@nongnu.org
> -S: Supported
> +L: integrat...@gluster.org
> +S: Odd Fixes
>  F: block/gluster.c
>  
>  Null Block Driver
> -- 
> 2.21.0
>

[Qemu-block] [QEMU PATCH] MAINTAINERS: Downgrade status of block sections without "M:" to "Odd Fixes"

2019-05-05 Thread Thomas Huth 
Fixes might still get picked up via the qemu-block mailing list,
so the status is not "Orphan" yet.
Also add the gluster mailing list as suggested by Niels here:

 https://patchwork.kernel.org/patch/10613297/#22409943

Signed-off-by: Thomas Huth 
---
 MAINTAINERS | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 66ddbda9c9..899a4cd572 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2404,12 +2404,13 @@ F: block/ssh.c
 
 CURL
 L: qemu-block@nongnu.org
-S: Supported
+S: Odd Fixes
 F: block/curl.c
 
 GLUSTER
 L: qemu-block@nongnu.org
-S: Supported
+L: integrat...@gluster.org
+S: Odd Fixes
 F: block/gluster.c
 
 Null Block Driver
-- 
2.21.0

Re: [Qemu-block] [Qemu-devel] [PATCH 0/2] buffer and delay backup COW write operation

2019-05-05 Thread Liang Li 
On Tue, Apr 30, 2019 at 10:35:32AM +, Vladimir Sementsov-Ogievskiy wrote:
> 28.04.2019 13:01, Liang Li wrote:
> > If the backup target is a slow device like ceph rbd, the backup
> > process will affect guest BLK write IO performance seriously,
> > it's cause by the drawback of COW mechanism, if guest overwrite the
> > backup BLK area, the IO can only be processed after the data has
> > been written to backup target.
> > The impact can be relieved by buffering data read from backup
> > source and writing to backup target later, so the guest BLK write
> > IO can be processed in time.
> > Data area with no overwrite will be process like before without
> > buffering, in most case, we don't need a very large buffer.
> > 
> > An fio test was done when the backup was going on, the test resut
> > show a obvious performance improvement by buffering.
> 
> Hi Liang!
> 
> Good thing. Something like this I've briefly mentioned in my KVM Forum 2018
> report as "RAM Cache", and I'd really prefer this functionality to be a 
> separate
> filter, instead of complication of backup code. Further more, write notifiers
> will go away from backup code, after my backup-top series merged.
> 
> v5: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg06211.html
> and separated preparing refactoring v7: 
> https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg04813.html
> 
> RAM Cache should be a filter driver, with an in-memory buffer(s) for data 
> written to it
> and with ability to flush data to underlying backing file.
> 
> Also, here is another approach for the problem, which helps if guest writing 
> activity
> is really high and long and buffer will be filled and performance will 
> decrease anyway:
> 
> 1. Create local temporary image, and COWs will go to it. (previously 
> considered on list, that we should call
> these backup operations issued by guest writes CBW = copy-before-write, as 
> copy-on-write
> is generally another thing, and using this term in backup is confusing).
> 
> 2. We also set original disk as a backing for temporary image, and start 
> another backup from
> temporary to real target.
> 
> This scheme is almost possible now, you need to start backup(sync=none) from 
> source to temp,
> to do [1]. Some patches are still needed to allow such scheme. I didn't send 
> them, as I want
> my other backup patches go first anyway. But I can. On the other hand if 
> approach with in-memory
> buffer works for you it may be better.
> 
> Also, I'm not sure for now, should we really do this thing through two backup 
> jobs, or we just
> need one separate backup-top filter and one backup job without filter, or we 
> need an additional
> parameter for backup job to set cache-block-node.
> 

Hi Vladimir,

   Thanks for your valuable information. I didn't notice that you are already 
working on
this,  so my patch will conflict with your work. We have thought about the way 
[2] and
give it up because it would affect local storage performance.
   I have read your slice in KVM Forum 2018 and the related patches, your 
solution can
help to solve the issues in backup. I am not sure if the "RAM cache" is a qcow2 
file in
RAM? if so, your implementation will free the RAM space occupied by BLK data 
once it's
written to the far target in time? or we may need a large cache to make things 
work.
   Two backup jobs seems complex and not user friendly, is it possible to make 
my patch
cowork with CBW?

Liang

Re: [Qemu-block] [PATCH v4 01/10] block/pflash_cfi02: Add test for supported commands

2019-05-05 Thread Thomas Huth 
On 26/04/2019 18.26, Stephen Checkoway wrote:
> Test the AMD command set for parallel flash chips. This test uses an
> ARM musicpal board with a pflash drive to test the following list of
> currently-supported commands.
> - Autoselect
> - CFI
> - Sector erase
> - Chip erase
> - Program
> - Unlock bypass
> - Reset
> 
> Signed-off-by: Stephen Checkoway 
> ---
>  tests/Makefile.include|   2 +
>  tests/pflash-cfi02-test.c | 225 ++
>  2 files changed, 227 insertions(+)
>  create mode 100644 tests/pflash-cfi02-test.c

Acked-by: Thomas Huth

Re: [Qemu-block] [PATCH] pflash: Only read non-zero parts of backend image

2019-05-05 Thread Xiang Zheng 


On 2019/5/5 23:37, Peter Maydell wrote:
> On Sun, 5 May 2019 at 08:02, Xiang Zheng  wrote:
>>
>> Currently we fill the memory space with two 64MB NOR images when
>> using persistent UEFI variables on virt board. Actually we only use
>> a very small(non-zero) part of the memory while the rest significant
>> large(zero) part of memory is wasted.
>>
>> So this patch checks the block status and only writes the non-zero part
>> into memory. This requires pflash devices to use sparse files for
>> backends.
> 
> Do you mean "pflash devices will no longer work if the file
> that is backing them is not sparse", or just "if the file that
> is backing them is not sparse then you won't get the benefit
> of using less memory" ?
> 

I mean the latter, if the file is not sparse, nothing would change.
I will improve this commit message in the next version.

-- 

Thanks,
Xiang

Re: [Qemu-block] [PATCH 5/5] hw/block/pflash_cfi02: Add the DeviceReset() handler

2019-05-05 Thread Wei Yang 
On Sun, May 05, 2019 at 10:06:02PM +0200, Philippe Mathieu-Daudé wrote:
>The pflash device is a child of TYPE_DEVICE, so it can implement
>the DeviceReset handler. Actually it has to implement it, else
>on machine reset it might stay in an incoherent state, as it has
>been reported in the buglink listed below.
>
>Add the DeviceReset handler and remove its call from the realize()
>function.
>
>Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>Reported-by: Laszlo Ersek 
>Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Wei Yang 


-- 
Wei Yang
Help you, Help me

Re: [Qemu-block] [PATCH 4/5] hw/block/pflash_cfi02: Extract the pflash_reset() code

2019-05-05 Thread Wei Yang 
On Sun, May 05, 2019 at 10:06:01PM +0200, Philippe Mathieu-Daudé wrote:
>The reset() code is used in various places, refactor it.
>
>Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Wei Yang 

>---
> hw/block/pflash_cfi02.c | 25 +++--
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
>diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>index f2c6201f813..f321b74433c 100644
>--- a/hw/block/pflash_cfi02.c
>+++ b/hw/block/pflash_cfi02.c
>@@ -120,6 +120,17 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
>rom_mode)
> pfl->rom_mode = rom_mode;
> }
> 
>+static void pflash_reset(PFlashCFI02 *pfl)
>+{
>+trace_pflash_reset();
>+timer_del(&pfl->timer);
>+pfl->bypass = 0;
>+pfl->wcycle = 0;
>+pfl->cmd = 0;
>+pfl->status = 0;
>+pflash_register_memory(pfl, 1);
>+}
>+
> static void pflash_timer (void *opaque)
> {
> PFlashCFI02 *pfl = opaque;
>@@ -129,11 +140,10 @@ static void pflash_timer (void *opaque)
> pfl->status ^= 0x80;
> if (pfl->bypass) {
> pfl->wcycle = 2;
>+pfl->cmd = 0;
> } else {
>-pflash_register_memory(pfl, 1);
>-pfl->wcycle = 0;
>+pflash_reset(pfl);
> }
>-pfl->cmd = 0;
> }
> 
> static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
>@@ -481,10 +491,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
> 
> /* Reset flash */
>  reset_flash:
>-trace_pflash_reset();
>-pfl->bypass = 0;
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>+pflash_reset(pfl);
> return;
> 
>  do_bypass:
>@@ -588,9 +595,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
>**errp)
> sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
> 
> timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>-pfl->status = 0;
>+pflash_reset(pfl);
> /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
> /* Standard "QRY" string */
> pfl->cfi_table[0x10] = 'Q';
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

Re: [Qemu-block] [PATCH 3/5] hw/block/pflash_cfi01: Add the DeviceReset() handler

2019-05-05 Thread Wei Yang 
On Sun, May 05, 2019 at 10:06:00PM +0200, Philippe Mathieu-Daudé wrote:
>The pflash device is a child of TYPE_DEVICE, so it can implement
>the DeviceReset handler. Actually it has to implement it, else
>on machine reset it might stay in an incoherent state, as it has
>been reported in the buglink listed below.
>
>Add the DeviceReset handler and remove its call from the realize()
>function.
>
>Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>Reported-by: Laszlo Ersek 
>Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Wei Yang 


-- 
Wei Yang
Help you, Help me

Re: [Qemu-block] [PATCH 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code

2019-05-05 Thread Wei Yang 
On Sun, May 05, 2019 at 10:05:59PM +0200, Philippe Mathieu-Daudé wrote:
>The reset() code is used in various places, refactor it.
>
>Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Wei Yang 

>---
> hw/block/pflash_cfi01.c | 21 -
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>index 6dc04f156a7..073cd14978f 100644
>--- a/hw/block/pflash_cfi01.c
>+++ b/hw/block/pflash_cfi01.c
>@@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
> }
> };
> 
>+static void pflash_reset(PFlashCFI01 *pfl)
>+{
>+trace_pflash_reset();
>+pfl->wcycle = 0;
>+pfl->cmd = 0;
>+pfl->status = 0;
>+memory_region_rom_device_set_romd(&pfl->mem, true);
>+}
>+
> /* Perform a CFI query based on the bank width of the flash.
>  * If this code is called we know we have a device_width set for
>  * this flash.
>@@ -275,8 +284,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr 
>offset,
> default:
> /* This should never happen : reset state & treat it as a read */
> DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>+pflash_reset(pfl);
> /* fall through to read code */
> case 0x00:
> /* Flash area read */
>@@ -639,10 +647,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>   "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
> 
>  reset_flash:
>-trace_pflash_reset();
>-memory_region_rom_device_set_romd(&pfl->mem, true);
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>+pflash_reset(pfl);
> }
> 
> 
>@@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
>**errp)
> pfl->max_device_width = pfl->device_width;
> }
> 
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>-pfl->status = 0;
>+pflash_reset(pfl);
> /* Hardcoded CFI table */
> /* Standard "QRY" string */
> pfl->cfi_table[0x10] = 'Q';
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

Re: [Qemu-block] [PATCH 1/5] hw/block/pflash_cfi01: Removed an unused timer

2019-05-05 Thread Wei Yang 
On Sun, May 05, 2019 at 10:05:58PM +0200, Philippe Mathieu-Daudé wrote:
>The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
>timing modelled. One year later, the CFI02 model was introduced
>(commit 05ee37ebf630) based on the CFI01 model. As noted in the
>header, "It does not support timings". 12 years later, we never
>had to model the device timings. Time to remove the unused timer,
>we can still add it back if required.
>
>Suggested-by: Laszlo Ersek 
>Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Wei Yang 

>---
>Yes, I plan to model those timings later. Actually I have a series
>working, but I'd rather first
>1/ refactor common code between the both CFI implementations,
>2/ discuss on list whether or not use timings for the Virt flash.
>---
> hw/block/pflash_cfi01.c | 15 ---
> 1 file changed, 15 deletions(-)
>
>diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>index 16dfae14b80..6dc04f156a7 100644
>--- a/hw/block/pflash_cfi01.c
>+++ b/hw/block/pflash_cfi01.c
>@@ -42,7 +42,6 @@
> #include "hw/block/flash.h"
> #include "sysemu/block-backend.h"
> #include "qapi/error.h"
>-#include "qemu/timer.h"
> #include "qemu/bitops.h"
> #include "qemu/host-utils.h"
> #include "qemu/log.h"
>@@ -86,7 +85,6 @@ struct PFlashCFI01 {
> uint8_t cfi_table[0x52];
> uint64_t counter;
> unsigned int writeblock_size;
>-QEMUTimer *timer;
> MemoryRegion mem;
> char *name;
> void *storage;
>@@ -110,18 +108,6 @@ static const VMStateDescription vmstate_pflash = {
> }
> };
> 
>-static void pflash_timer (void *opaque)
>-{
>-PFlashCFI01 *pfl = opaque;
>-
>-trace_pflash_timer_expired(pfl->cmd);
>-/* Reset flash */
>-pfl->status ^= 0x80;
>-memory_region_rom_device_set_romd(&pfl->mem, true);
>-pfl->wcycle = 0;
>-pfl->cmd = 0;
>-}
>-
> /* Perform a CFI query based on the bank width of the flash.
>  * If this code is called we know we have a device_width set for
>  * this flash.
>@@ -771,7 +757,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
>**errp)
> pfl->max_device_width = pfl->device_width;
> }
> 
>-pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> pfl->wcycle = 0;
> pfl->cmd = 0;
> pfl->status = 0;
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

[Qemu-block] [PATCH 11/13] hw/block/pflash_cfi02: Unify the MemoryRegionOps

2019-05-05 Thread Philippe Mathieu-Daudé 
The pflash_read()/pflash_write() can check the device endianess
via the pfl->be variable, so remove the 'int be' argument.

Since the big/little MemoryRegionOps are now identical, it is
pointless to declare them both. Unify them.

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-3-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Extracted from bigger patch to ease review]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 62 +++--
 1 file changed, 16 insertions(+), 46 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index adfa39f9b5e..59daade2ee6 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -186,11 +186,11 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 return ret;
 }
 
-static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
-int width, int be)
+static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
+PFlashCFI02 *pfl = opaque;
 hwaddr boff;
-uint32_t ret;
+uint64_t ret;
 
 ret = -1;
 trace_pflash_read(offset, pfl->cmd, width, pfl->wcycle);
@@ -238,14 +238,14 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 default:
 ret = pflash_data_read(pfl, offset, width);
 }
-DPRINTF("%s: ID " TARGET_FMT_plx " %" PRIx32 "\n", __func__, boff, 
ret);
+DPRINTF("%s: ID " TARGET_FMT_plx " %" PRIx64 "\n", __func__, boff, 
ret);
 break;
 case 0xA0:
 case 0x10:
 case 0x30:
 /* Status register read */
 ret = pfl->status;
-DPRINTF("%s: status %" PRIx32 "\n", __func__, ret);
+DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
 /* Toggle bit 6 */
 toggle_dq6(pfl);
 break;
@@ -263,8 +263,7 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
 }
 
 /* update flash content on disk */
-static void pflash_update(PFlashCFI02 *pfl, int offset,
-  int size)
+static void pflash_update(PFlashCFI02 *pfl, int offset, int size)
 {
 int offset_end;
 if (pfl->blk) {
@@ -277,9 +276,10 @@ static void pflash_update(PFlashCFI02 *pfl, int offset,
 }
 }
 
-static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
- uint32_t value, int width, int be)
+static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned int width)
 {
+PFlashCFI02 *pfl = opaque;
 hwaddr boff;
 uint8_t *p;
 uint8_t cmd;
@@ -295,7 +295,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 trace_pflash_write(offset, value, width, pfl->wcycle);
 offset &= pfl->chip_len - 1;
 
-DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx32 " %d\n", __func__,
+DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n", __func__,
 offset, value, width);
 boff = offset & (pfl->sector_len - 1);
 if (pfl->width == 2)
@@ -492,39 +492,9 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 pfl->cmd = 0;
 }
 
-static uint64_t pflash_be_readfn(void *opaque, hwaddr addr, unsigned size)
-{
-return pflash_read(opaque, addr, size, 1);
-}
-
-static void pflash_be_writefn(void *opaque, hwaddr addr,
-  uint64_t value, unsigned size)
-{
-pflash_write(opaque, addr, value, size, 1);
-}
-
-static uint64_t pflash_le_readfn(void *opaque, hwaddr addr, unsigned size)
-{
-return pflash_read(opaque, addr, size, 0);
-}
-
-static void pflash_le_writefn(void *opaque, hwaddr addr,
-  uint64_t value, unsigned size)
-{
-pflash_write(opaque, addr, value, size, 0);
-}
-
-static const MemoryRegionOps pflash_cfi02_ops_be = {
-.read = pflash_be_readfn,
-.write = pflash_be_writefn,
-.valid.min_access_size = 1,
-.valid.max_access_size = 4,
-.endianness = DEVICE_NATIVE_ENDIAN,
-};
-
-static const MemoryRegionOps pflash_cfi02_ops_le = {
-.read = pflash_le_readfn,
-.write = pflash_le_writefn,
+static const MemoryRegionOps pflash_cfi02_ops = {
+.read = pflash_read,
+.write = pflash_write,
 .valid.min_access_size = 1,
 .valid.max_access_size = 4,
 .endianness = DEVICE_NATIVE_ENDIAN,
@@ -552,9 +522,9 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 
 chip_len = pfl->sector_len * pfl->nb_blocs;
 
-memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
-  &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
-  pfl, pfl->name, chip_len, &local_err);
+memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
+  &pflash_cfi02_ops, pfl, pfl->name,
+  chip_len, &local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 retur

[Qemu-block] [PATCH 06/13] hw/block/pflash_cfi02: Add helpers to manipulate the status bits

2019-05-05 Thread Philippe Mathieu-Daudé 
Pull out all of the code to modify the status into simple helper
functions. Status handling becomes more complex once multiple
chips are interleaved to produce a single device.

No change in functionality is intended with this commit.

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-3-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Extracted from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 39 ++-
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index b27796d74d2..9673eee969f 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -100,6 +100,31 @@ struct PFlashCFI02 {
 void *storage;
 };
 
+/*
+ * Toggle status bit DQ7.
+ */
+static inline void toggle_dq7(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x80;
+}
+
+/*
+ * Set status bit DQ7 to bit 7 of value.
+ */
+static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+{
+pfl->status &= 0x7F;
+pfl->status |= value & 0x80;
+}
+
+/*
+ * Toggle status bit DQ6.
+ */
+static inline void toggle_dq6(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x40;
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -129,7 +154,7 @@ static void pflash_timer (void *opaque)
 
 trace_pflash_timer_expired(pfl->cmd);
 /* Reset flash */
-pfl->status ^= 0x80;
+toggle_dq7(pfl);
 if (pfl->bypass) {
 pfl->wcycle = 2;
 } else {
@@ -232,7 +257,7 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
 ret = pfl->status;
 DPRINTF("%s: status %" PRIx32 "\n", __func__, ret);
 /* Toggle bit 6 */
-pfl->status ^= 0x40;
+toggle_dq6(pfl);
 break;
 case 0x98:
 /* CFI query mode */
@@ -381,7 +406,11 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 break;
 }
 }
-pfl->status = 0x00 | ~(value & 0x80);
+/*
+ * While programming, status bit DQ7 should hold the opposite
+ * value from how it was programmed.
+ */
+set_dq7(pfl, ~value);
 /* Let's pretend write is immediate */
 if (pfl->bypass)
 goto do_bypass;
@@ -429,7 +458,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 memset(pfl->storage, 0xFF, pfl->chip_len);
 pflash_update(pfl, 0, pfl->chip_len);
 }
-pfl->status = 0x00;
+set_dq7(pfl, 0x00);
 /* Let's wait 5 seconds before chip erase is done */
 timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
   (NANOSECONDS_PER_SECOND * 5));
@@ -444,7 +473,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 memset(p + offset, 0xFF, pfl->sector_len);
 pflash_update(pfl, offset, pfl->sector_len);
 }
-pfl->status = 0x00;
+set_dq7(pfl, 0x00);
 /* Let's wait 1/2 second before sector erase is done */
 timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
   (NANOSECONDS_PER_SECOND / 2));
-- 
2.20.1

[Qemu-block] [PATCH 12/13] hw/block/pflash_cfi02: Fix command address comparison

2019-05-05 Thread Philippe Mathieu-Daudé 
From: Stephen Checkoway 

Most AMD commands only examine 11 bits of the address. This masks the
addresses used in the comparison to 11 bits. The exceptions are word or
sector addresses which use offset directly rather than the shifted
offset, boff.

Signed-off-by: Stephen Checkoway 
Acked-by: Thomas Huth 
Message-Id: <20190426162624.55977-4-stephen.checko...@oberlin.edu>
[PMD: Prepend 'hw/' in patch subject]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c   |  8 +++-
 tests/pflash-cfi02-test.c | 12 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 59daade2ee6..4c17dbf99f4 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -297,11 +297,13 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 
 DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n", __func__,
 offset, value, width);
-boff = offset & (pfl->sector_len - 1);
+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;
 switch (pfl->wcycle) {
 case 0:
 /* Set the device in I/O access mode if required */
@@ -520,6 +522,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
+/* Only 11 bits are used in the comparison. */
+pfl->unlock_addr0 &= 0x7FF;
+pfl->unlock_addr1 &= 0x7FF;
+
 chip_len = pfl->sector_len * pfl->nb_blocs;
 
 memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index 3c37465499a..1e5d9124b71 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -22,8 +22,8 @@
 
 #define FLASH_WIDTH 2
 #define CFI_ADDR (FLASH_WIDTH * 0x55)
-#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
-#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
+#define UNLOCK0_ADDR (FLASH_WIDTH * 0x555)
+#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AA)
 
 #define CFI_CMD 0x98
 #define UNLOCK0_CMD 0xAA
@@ -190,6 +190,14 @@ static void test_flash(const void *opaque)
 g_assert_cmpint(flash_read(6), ==, 0xCDEF);
 g_assert_cmpint(flash_read(8), ==, 0x);
 
+/* Test ignored high order bits of address. */
+flash_write(FLASH_WIDTH * 0x, UNLOCK0_CMD);
+flash_write(FLASH_WIDTH * 0x2AAA, UNLOCK1_CMD);
+flash_write(FLASH_WIDTH * 0x, AUTOSELECT_CMD);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0x), ==, 0x00BF);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0x0001), ==, 0x236D);
+reset();
+
 qtest_quit(global_qtest);
 }
 
-- 
2.20.1

[Qemu-block] [PATCH 13/13] hw/block/pflash_cfi02: Use the chip erase time specified in the CFI table

2019-05-05 Thread Philippe Mathieu-Daudé 
From: Stephen Checkoway 

When erasing the chip, use the typical time specified in the CFI table
rather than arbitrarily selecting 5 seconds.

Since the currently unconfigurable value set in the table is 12, this
means a chip erase takes 4096 ms so this isn't a big change in behavior.

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-11-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Prepend 'hw/' in patch subject]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4c17dbf99f4..49cd9ed0f91 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -429,9 +429,9 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pflash_update(pfl, 0, pfl->chip_len);
 }
 set_dq7(pfl, 0x00);
-/* Let's wait 5 seconds before chip erase is done */
+/* Wait the time specified at CFI address 0x22. */
 timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-  (NANOSECONDS_PER_SECOND * 5));
+  (1ULL << pfl->cfi_table[0x22]) * SCALE_MS);
 break;
 case 0x30:
 /* Sector erase */
-- 
2.20.1

[Qemu-block] [PATCH 07/13] hw/block/pflash_cfi02: Simplify a statement using fall through

2019-05-05 Thread Philippe Mathieu-Daudé 
Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-3-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Extracted from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 9673eee969f..1e006e7bf3a 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -241,10 +241,10 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x0E:
 case 0x0F:
 ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
-if (ret == (uint8_t)-1) {
-goto flash_read;
+if (ret != (uint8_t)-1) {
+break;
 }
-break;
+/* Fall through to data read. */
 default:
 goto flash_read;
 }
-- 
2.20.1

[Qemu-block] [PATCH 04/13] hw/block/pflash_cfi02: Fix debug format string

2019-05-05 Thread Philippe Mathieu-Daudé 
Always compile the debug code to prevent format string to bitrot.

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-3-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Extracted from bigger patch, use PRIx32]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f2c6201f813..f88e09cebaa 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -46,15 +46,13 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 
-//#define PFLASH_DEBUG
-#ifdef PFLASH_DEBUG
+#define PFLASH_DEBUG false
 #define DPRINTF(fmt, ...)  \
 do {   \
-fprintf(stderr, "PFLASH: " fmt , ## __VA_ARGS__);   \
+if (PFLASH_DEBUG) {\
+fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__);   \
+}  \
 } while (0)
-#else
-#define DPRINTF(fmt, ...) do { } while (0)
-#endif
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
@@ -220,14 +218,14 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 default:
 goto flash_read;
 }
-DPRINTF("%s: ID " TARGET_FMT_plx " %x\n", __func__, boff, ret);
+DPRINTF("%s: ID " TARGET_FMT_plx " %" PRIx32 "\n", __func__, boff, 
ret);
 break;
 case 0xA0:
 case 0x10:
 case 0x30:
 /* Status register read */
 ret = pfl->status;
-DPRINTF("%s: status %x\n", __func__, ret);
+DPRINTF("%s: status %" PRIx32 "\n", __func__, ret);
 /* Toggle bit 6 */
 pfl->status ^= 0x40;
 break;
@@ -277,7 +275,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 trace_pflash_write(offset, value, width, pfl->wcycle);
 offset &= pfl->chip_len - 1;
 
-DPRINTF("%s: offset " TARGET_FMT_plx " %08x %d\n", __func__,
+DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx32 " %d\n", __func__,
 offset, value, width);
 boff = offset & (pfl->sector_len - 1);
 if (pfl->width == 2)
-- 
2.20.1

[Qemu-block] [PATCH 08/13] hw/block/pflash_cfi02: Use the ldst API in pflash_write()

2019-05-05 Thread Philippe Mathieu-Daudé 
The load/store API eases code review.

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-3-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Extracted from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 38 --
 1 file changed, 8 insertions(+), 30 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 1e006e7bf3a..69e0086324e 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -373,38 +373,16 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 goto check_unlock0;
 case 0xA0:
 trace_pflash_data_write(offset, value, width, 0);
-p = pfl->storage;
 if (!pfl->ro) {
-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 {
-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;
+p = (uint8_t *)pfl->storage + offset;
+if (pfl->be) {
+uint64_t current = ldn_be_p(p, width);
+stn_be_p(p, width, current & value);
+} else {
+uint64_t current = ldn_le_p(p, width);
+stn_le_p(p, width, current & value);
 }
+pflash_update(pfl, offset, width);
 }
 /*
  * While programming, status bit DQ7 should hold the opposite
-- 
2.20.1

[Qemu-block] [PATCH 03/13] tests/pflash-cfi02: Use IEC binary prefixes for size constants

2019-05-05 Thread Philippe Mathieu-Daudé 
Using IEC binary prefixes in order to make the code more readable.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/pflash-cfi02-test.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index ff775618c02..3c37465499a 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "libqtest.h"
 
 /*
@@ -16,7 +17,7 @@
  * all. In particular, we're limited to a 16-bit wide flash device.
  */
 
-#define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
+#define MP_FLASH_SIZE_MAX (32 * MiB)
 #define BASE_ADDR (0x1ULL - MP_FLASH_SIZE_MAX)
 
 #define FLASH_WIDTH 2
@@ -205,7 +206,7 @@ int main(int argc, char **argv)
 char *image_path;
 int fd = g_file_open_tmp("pflash-cfi02-XX.raw", &image_path, &error);
 g_assert_no_error(error);
-if (ftruncate(fd, 8 * 1024 * 1024) < 0) {
+if (ftruncate(fd, 8 * MiB) < 0) {
 g_printerr("Failed to truncate file %s to 8 MB: %s\n", image_path,
strerror(errno));
 close(fd);
-- 
2.20.1

[Qemu-block] [PATCH 10/13] hw/block/pflash_cfi02: Extract the pflash_data_read() function

2019-05-05 Thread Philippe Mathieu-Daudé 
Extract the code block in a new function, remove a goto statement.

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-3-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Extracted from bigger patch, remove the XXX tracing comment]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 44 ++---
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 2777167af11..adfa39f9b5e 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -164,12 +164,33 @@ static void pflash_timer (void *opaque)
 pfl->cmd = 0;
 }
 
+/*
+ * Read data from flash.
+ */
+static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr offset,
+ unsigned int width)
+{
+uint8_t *p = (uint8_t *)pfl->storage + offset;
+uint64_t ret = pfl->be ? ldn_be_p(p, width) : ldn_le_p(p, width);
+switch (width) {
+case 1:
+trace_pflash_data_read8(offset, ret);
+break;
+case 2:
+trace_pflash_data_read16(offset, ret);
+break;
+case 4:
+trace_pflash_data_read32(offset, ret);
+break;
+}
+return ret;
+}
+
 static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
 int width, int be)
 {
 hwaddr boff;
 uint32_t ret;
-uint8_t *p;
 
 ret = -1;
 trace_pflash_read(offset, pfl->cmd, width, pfl->wcycle);
@@ -194,25 +215,8 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x80:
 /* We accept reads during second unlock sequence... */
 case 0x00:
-flash_read:
 /* Flash area read */
-p = (uint8_t *)pfl->storage + offset;
-if (pfl->be) {
-ret = ldn_be_p(p, width);
-} else {
-ret = ldn_le_p(p, width);
-}
-switch (width) {
-case 1:
-trace_pflash_data_read8(offset, ret);
-break;
-case 2:
-trace_pflash_data_read16(offset, ret);
-break;
-case 4:
-trace_pflash_data_read32(offset, ret);
-break;
-}
+ret = pflash_data_read(pfl, offset, width);
 break;
 case 0x90:
 /* flash ID read */
@@ -232,7 +236,7 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
 }
 /* Fall through to data read. */
 default:
-goto flash_read;
+ret = pflash_data_read(pfl, offset, width);
 }
 DPRINTF("%s: ID " TARGET_FMT_plx " %" PRIx32 "\n", __func__, boff, 
ret);
 break;
-- 
2.20.1

[Qemu-block] [PATCH 05/13] hw/block/pflash_cfi02: Add an enum to define the write cycles

2019-05-05 Thread Philippe Mathieu-Daudé 
No change in functionality is intended with this commit.

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-3-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Extracted from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f88e09cebaa..b27796d74d2 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -56,6 +56,11 @@ do {   \
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/* Special write cycles for CFI queries. */
+enum {
+WCYCLE_CFI  = 7,
+};
+
 struct PFlashCFI02 {
 /*< private >*/
 SysBusDevice parent_obj;
@@ -293,7 +298,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 if (boff == 0x55 && cmd == 0x98) {
 enter_CFI_mode:
 /* Enter CFI query mode */
-pfl->wcycle = 7;
+pfl->wcycle = WCYCLE_CFI;
 pfl->cmd = 0x98;
 return;
 }
@@ -465,7 +470,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 goto reset_flash;
 }
 break;
-case 7: /* Special value for CFI queries */
+case WCYCLE_CFI: /* Special value for CFI queries */
 DPRINTF("%s: invalid write in CFI query mode\n", __func__);
 goto reset_flash;
 default:
-- 
2.20.1

[Qemu-block] [PATCH 09/13] hw/block/pflash_cfi02: Use the ldst API in pflash_read()

2019-05-05 Thread Philippe Mathieu-Daudé 
The load/store API eases code review.

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-3-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: Extracted from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 26 ++
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 69e0086324e..2777167af11 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -196,34 +196,20 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x00:
 flash_read:
 /* Flash area read */
-p = pfl->storage;
+p = (uint8_t *)pfl->storage + offset;
+if (pfl->be) {
+ret = ldn_be_p(p, width);
+} else {
+ret = ldn_le_p(p, width);
+}
 switch (width) {
 case 1:
-ret = p[offset];
 trace_pflash_data_read8(offset, ret);
 break;
 case 2:
-if (be) {
-ret = p[offset] << 8;
-ret |= p[offset + 1];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-}
 trace_pflash_data_read16(offset, ret);
 break;
 case 4:
-if (be) {
-ret = p[offset] << 24;
-ret |= p[offset + 1] << 16;
-ret |= p[offset + 2] << 8;
-ret |= p[offset + 3];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-ret |= p[offset + 2] << 16;
-ret |= p[offset + 3] << 24;
-}
 trace_pflash_data_read32(offset, ret);
 break;
 }
-- 
2.20.1

[Qemu-block] [PATCH 00/13] hw/block/pflash_cfi02: Clean-up and fixes

2019-05-05 Thread Philippe Mathieu-Daudé 
Hi,

While reviewing Stephen Checkoway's v4 "Implement missing AMD
pflash functionality" [*] I found it hard (for me) to digest,
so I took step by step notes. This series is the result of
those notes.
Regarding Stephen's series, this series only contains the
generic code movement and trivial cleanup. The other patches
are rather dense and I need more time to study the specs.

Stephen: If you take out the patch #2 ("Use the GLib API"),
you can rebase your series on top of this.
I'd appreciate if you can adapt your tests to use the GLib
functions, else I plan to do it later.

Regards,

Phil.

[*] https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg04595.html

Philippe Mathieu-Daudé (10):
  tests/pflash-cfi02: Use the GLib API
  tests/pflash-cfi02: Use IEC binary prefixes for size constants
  hw/block/pflash_cfi02: Fix debug format string
  hw/block/pflash_cfi02: Add an enum to define the write cycles
  hw/block/pflash_cfi02: Add helpers to manipulate the status bits
  hw/block/pflash_cfi02: Simplify a statement using fall through
  hw/block/pflash_cfi02: Use the ldst API in pflash_write()
  hw/block/pflash_cfi02: Use the ldst API in pflash_read()
  hw/block/pflash_cfi02: Extract the pflash_data_read() function
  hw/block/pflash_cfi02: Unify the MemoryRegionOps

Stephen Checkoway (3):
  tests/pflash-cfi02: Add test for supported CFI commands
  hw/block/pflash_cfi02: Fix command address comparison
  hw/block/pflash_cfi02: Use the chip erase time specified in the CFI
table

 hw/block/pflash_cfi02.c   | 234 +-
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 232 +
 3 files changed, 339 insertions(+), 129 deletions(-)
 create mode 100644 tests/pflash-cfi02-test.c

-- 
2.20.1

[Qemu-block] [PATCH 01/13] tests/pflash-cfi02: Add test for supported CFI commands

2019-05-05 Thread Philippe Mathieu-Daudé 
From: Stephen Checkoway 

Test the AMD command set for parallel flash chips. This test uses an
ARM musicpal board with a pflash drive to test the following list of
currently-supported commands.
- Autoselect
- CFI
- Sector erase
- Chip erase
- Program
- Unlock bypass
- Reset

Signed-off-by: Stephen Checkoway 
Message-Id: <20190426162624.55977-2-stephen.checko...@oberlin.edu>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
[PMD: reworded the patch subject]
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 225 ++
 2 files changed, 227 insertions(+)
 create mode 100644 tests/pflash-cfi02-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 7c8b9c84b24..b84a308d9a1 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -263,6 +263,7 @@ check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/hexloader-test$(EXESUF)
+check-qtest-arm-$(CONFIG_PFLASH_CFI02) += tests/pflash-cfi02-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
@@ -773,6 +774,7 @@ tests/device-introspect-test$(EXESUF): 
tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
 tests/hexloader-test$(EXESUF): tests/hexloader-test.o
+tests/pflash-cfi02$(EXESUF): tests/pflash-cfi02-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
 tests/rtas-test$(EXESUF): tests/rtas-test.o $(libqos-spapr-obj-y)
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
new file mode 100644
index 000..40af1bb523e
--- /dev/null
+++ b/tests/pflash-cfi02-test.c
@@ -0,0 +1,225 @@
+/*
+ * QTest testcase for parallel flash with AMD command set
+ *
+ * Copyright (c) 2019 Stephen Checkoway
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+/*
+ * To test the pflash_cfi02 device, we run QEMU with the musicpal machine with
+ * a pflash drive. This enables us to test some flash configurations, but not
+ * all. In particular, we're limited to a 16-bit wide flash device.
+ */
+
+#define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
+#define BASE_ADDR (0x1ULL - MP_FLASH_SIZE_MAX)
+
+#define FLASH_WIDTH 2
+#define CFI_ADDR (FLASH_WIDTH * 0x55)
+#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
+#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
+
+#define CFI_CMD 0x98
+#define UNLOCK0_CMD 0xAA
+#define UNLOCK1_CMD 0x55
+#define AUTOSELECT_CMD 0x90
+#define RESET_CMD 0xF0
+#define PROGRAM_CMD 0xA0
+#define SECTOR_ERASE_CMD 0x30
+#define CHIP_ERASE_CMD 0x10
+#define UNLOCK_BYPASS_CMD 0x20
+#define UNLOCK_BYPASS_RESET_CMD 0x00
+
+static char image_path[] = "/tmp/qtest.XX";
+
+static inline void flash_write(uint64_t byte_addr, uint16_t data)
+{
+qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
+}
+
+static inline uint16_t flash_read(uint64_t byte_addr)
+{
+return qtest_readw(global_qtest, BASE_ADDR + byte_addr);
+}
+
+static void unlock(void)
+{
+flash_write(UNLOCK0_ADDR, UNLOCK0_CMD);
+flash_write(UNLOCK1_ADDR, UNLOCK1_CMD);
+}
+
+static void reset(void)
+{
+flash_write(0, RESET_CMD);
+}
+
+static void sector_erase(uint64_t byte_addr)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(byte_addr, SECTOR_ERASE_CMD);
+}
+
+static void wait_for_completion(uint64_t byte_addr)
+{
+/* If DQ6 is toggling, step the clock and ensure the toggle stops. */
+if ((flash_read(byte_addr) & 0x40) ^ (flash_read(byte_addr) & 0x40)) {
+/* Wait for erase or program to finish. */
+clock_step_next();
+/* Ensure that DQ6 has stopped toggling. */
+g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
+}
+}
+
+static void bypass_program(uint64_t byte_addr, uint16_t data)
+{
+flash_write(UNLOCK0_ADDR, PROGRAM_CMD);
+flash_write(byte_addr, data);
+/*
+ * Data isn't valid until DQ6 stops toggling. We don't model this as
+ * writes are immediate, but if this changes in the future, we can wait
+ * until the program is complete.
+ */
+wait_for_completion(byte_addr);
+}
+
+static void program(uint64_t byte_addr, uint16_t data)
+{
+unlock();
+bypass_program(byte_addr, data);
+}
+
+static void chip_erase(void)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(UNLOCK0_ADDR, SECTOR_ERASE_CMD);
+}
+
+static void test_flash(void)
+{
+global_qtest = qtest_initf("-M musicpal,accel=qtest "
+   "-drive 
if=pflash,file=%s,format=raw,copy-on-read",
+

[Qemu-block] [PATCH 02/13] tests/pflash-cfi02: Use the GLib API

2019-05-05 Thread Philippe Mathieu-Daudé 
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/pflash-cfi02-test.c | 28 +---
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index 40af1bb523e..ff775618c02 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -35,8 +35,6 @@
 #define UNLOCK_BYPASS_CMD 0x20
 #define UNLOCK_BYPASS_RESET_CMD 0x00
 
-static char image_path[] = "/tmp/qtest.XX";
-
 static inline void flash_write(uint64_t byte_addr, uint16_t data)
 {
 qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
@@ -103,8 +101,9 @@ static void chip_erase(void)
 flash_write(UNLOCK0_ADDR, SECTOR_ERASE_CMD);
 }
 
-static void test_flash(void)
+static void test_flash(const void *opaque)
 {
+const char *image_path = opaque;
 global_qtest = qtest_initf("-M musicpal,accel=qtest "
"-drive 
if=pflash,file=%s,format=raw,copy-on-read",
image_path);
@@ -195,31 +194,30 @@ static void test_flash(void)
 
 static void cleanup(void *opaque)
 {
+char *image_path = opaque;
 unlink(image_path);
+g_free(image_path);
 }
 
 int main(int argc, char **argv)
 {
-int fd = mkstemp(image_path);
-if (fd == -1) {
-g_printerr("Failed to create temporary file %s: %s\n", image_path,
-   strerror(errno));
-exit(EXIT_FAILURE);
-}
+GError *error = NULL;
+char *image_path;
+int fd = g_file_open_tmp("pflash-cfi02-XX.raw", &image_path, &error);
+g_assert_no_error(error);
 if (ftruncate(fd, 8 * 1024 * 1024) < 0) {
-int error_code = errno;
+g_printerr("Failed to truncate file %s to 8 MB: %s\n", image_path,
+   strerror(errno));
 close(fd);
 unlink(image_path);
-g_printerr("Failed to truncate file %s to 8 MB: %s\n", image_path,
-   strerror(error_code));
 exit(EXIT_FAILURE);
 }
 close(fd);
 
-qtest_add_abrt_handler(cleanup, NULL);
+qtest_add_abrt_handler(cleanup, image_path);
 g_test_init(&argc, &argv, NULL);
-qtest_add_func("pflash-cfi02", test_flash);
+qtest_add_data_func("pflash-cfi02", image_path, test_flash);
 int result = g_test_run();
-cleanup(NULL);
+cleanup(image_path);
 return result;
 }
-- 
2.20.1

[Qemu-block] [PATCH 5/5] hw/block/pflash_cfi02: Add the DeviceReset() handler

2019-05-05 Thread Philippe Mathieu-Daudé 
The pflash device is a child of TYPE_DEVICE, so it can implement
the DeviceReset handler. Actually it has to implement it, else
on machine reset it might stay in an incoherent state, as it has
been reported in the buglink listed below.

Add the DeviceReset handler and remove its call from the realize()
function.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
Reported-by: Laszlo Ersek 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f321b74433c..5af367d1563 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -674,6 +674,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 pfl->cfi_table[0x3c] = 0x00;
 }
 
+static void pflash_cfi02_reset(DeviceState *dev)
+{
+pflash_reset(PFLASH_CFI02(dev));
+}
+
 static Property pflash_cfi02_properties[] = {
 DEFINE_PROP_DRIVE("drive", PFlashCFI02, blk),
 DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, nb_blocs, 0),
@@ -701,6 +706,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, 
void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
+dc->reset = pflash_cfi02_reset;
 dc->realize = pflash_cfi02_realize;
 dc->unrealize = pflash_cfi02_unrealize;
 dc->props = pflash_cfi02_properties;
-- 
2.20.1

[Qemu-block] [PATCH 4/5] hw/block/pflash_cfi02: Extract the pflash_reset() code

2019-05-05 Thread Philippe Mathieu-Daudé 
The reset() code is used in various places, refactor it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi02.c | 25 +++--
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f2c6201f813..f321b74433c 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -120,6 +120,17 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
rom_mode)
 pfl->rom_mode = rom_mode;
 }
 
+static void pflash_reset(PFlashCFI02 *pfl)
+{
+trace_pflash_reset();
+timer_del(&pfl->timer);
+pfl->bypass = 0;
+pfl->wcycle = 0;
+pfl->cmd = 0;
+pfl->status = 0;
+pflash_register_memory(pfl, 1);
+}
+
 static void pflash_timer (void *opaque)
 {
 PFlashCFI02 *pfl = opaque;
@@ -129,11 +140,10 @@ static void pflash_timer (void *opaque)
 pfl->status ^= 0x80;
 if (pfl->bypass) {
 pfl->wcycle = 2;
+pfl->cmd = 0;
 } else {
-pflash_register_memory(pfl, 1);
-pfl->wcycle = 0;
+pflash_reset(pfl);
 }
-pfl->cmd = 0;
 }
 
 static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
@@ -481,10 +491,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 
 /* Reset flash */
  reset_flash:
-trace_pflash_reset();
-pfl->bypass = 0;
-pfl->wcycle = 0;
-pfl->cmd = 0;
+pflash_reset(pfl);
 return;
 
  do_bypass:
@@ -588,9 +595,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
 
 timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
-pfl->wcycle = 0;
-pfl->cmd = 0;
-pfl->status = 0;
+pflash_reset(pfl);
 /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
 /* Standard "QRY" string */
 pfl->cfi_table[0x10] = 'Q';
-- 
2.20.1

[Qemu-block] [PATCH 1/5] hw/block/pflash_cfi01: Removed an unused timer

2019-05-05 Thread Philippe Mathieu-Daudé 
The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
timing modelled. One year later, the CFI02 model was introduced
(commit 05ee37ebf630) based on the CFI01 model. As noted in the
header, "It does not support timings". 12 years later, we never
had to model the device timings. Time to remove the unused timer,
we can still add it back if required.

Suggested-by: Laszlo Ersek 
Signed-off-by: Philippe Mathieu-Daudé 
---
Yes, I plan to model those timings later. Actually I have a series
working, but I'd rather first
1/ refactor common code between the both CFI implementations,
2/ discuss on list whether or not use timings for the Virt flash.
---
 hw/block/pflash_cfi01.c | 15 ---
 1 file changed, 15 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 16dfae14b80..6dc04f156a7 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -42,7 +42,6 @@
 #include "hw/block/flash.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
-#include "qemu/timer.h"
 #include "qemu/bitops.h"
 #include "qemu/host-utils.h"
 #include "qemu/log.h"
@@ -86,7 +85,6 @@ struct PFlashCFI01 {
 uint8_t cfi_table[0x52];
 uint64_t counter;
 unsigned int writeblock_size;
-QEMUTimer *timer;
 MemoryRegion mem;
 char *name;
 void *storage;
@@ -110,18 +108,6 @@ static const VMStateDescription vmstate_pflash = {
 }
 };
 
-static void pflash_timer (void *opaque)
-{
-PFlashCFI01 *pfl = opaque;
-
-trace_pflash_timer_expired(pfl->cmd);
-/* Reset flash */
-pfl->status ^= 0x80;
-memory_region_rom_device_set_romd(&pfl->mem, true);
-pfl->wcycle = 0;
-pfl->cmd = 0;
-}
-
 /* Perform a CFI query based on the bank width of the flash.
  * If this code is called we know we have a device_width set for
  * this flash.
@@ -771,7 +757,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
**errp)
 pfl->max_device_width = pfl->device_width;
 }
 
-pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
 pfl->wcycle = 0;
 pfl->cmd = 0;
 pfl->status = 0;
-- 
2.20.1

[Qemu-block] [PATCH 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code

2019-05-05 Thread Philippe Mathieu-Daudé 
The reset() code is used in various places, refactor it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi01.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 6dc04f156a7..073cd14978f 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
 }
 };
 
+static void pflash_reset(PFlashCFI01 *pfl)
+{
+trace_pflash_reset();
+pfl->wcycle = 0;
+pfl->cmd = 0;
+pfl->status = 0;
+memory_region_rom_device_set_romd(&pfl->mem, true);
+}
+
 /* Perform a CFI query based on the bank width of the flash.
  * If this code is called we know we have a device_width set for
  * this flash.
@@ -275,8 +284,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
 default:
 /* This should never happen : reset state & treat it as a read */
 DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
-pfl->wcycle = 0;
-pfl->cmd = 0;
+pflash_reset(pfl);
 /* fall through to read code */
 case 0x00:
 /* Flash area read */
@@ -639,10 +647,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
   "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
 
  reset_flash:
-trace_pflash_reset();
-memory_region_rom_device_set_romd(&pfl->mem, true);
-pfl->wcycle = 0;
-pfl->cmd = 0;
+pflash_reset(pfl);
 }
 
 
@@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
**errp)
 pfl->max_device_width = pfl->device_width;
 }
 
-pfl->wcycle = 0;
-pfl->cmd = 0;
-pfl->status = 0;
+pflash_reset(pfl);
 /* Hardcoded CFI table */
 /* Standard "QRY" string */
 pfl->cfi_table[0x10] = 'Q';
-- 
2.20.1

[Qemu-block] [PATCH 3/5] hw/block/pflash_cfi01: Add the DeviceReset() handler

2019-05-05 Thread Philippe Mathieu-Daudé 
The pflash device is a child of TYPE_DEVICE, so it can implement
the DeviceReset handler. Actually it has to implement it, else
on machine reset it might stay in an incoherent state, as it has
been reported in the buglink listed below.

Add the DeviceReset handler and remove its call from the realize()
function.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
Reported-by: Laszlo Ersek 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi01.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 073cd14978f..639b05bc4d5 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -762,7 +762,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
**errp)
 pfl->max_device_width = pfl->device_width;
 }
 
-pflash_reset(pfl);
 /* Hardcoded CFI table */
 /* Standard "QRY" string */
 pfl->cfi_table[0x10] = 'Q';
@@ -850,6 +849,11 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
**errp)
 pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
 }
 
+static void pflash_cfi01_reset(DeviceState *dev)
+{
+pflash_reset(PFLASH_CFI01(dev));
+}
+
 static Property pflash_cfi01_properties[] = {
 DEFINE_PROP_DRIVE("drive", PFlashCFI01, blk),
 /* num-blocks is the number of blocks actually visible to the guest,
@@ -894,6 +898,7 @@ static void pflash_cfi01_class_init(ObjectClass *klass, 
void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
+dc->reset = pflash_cfi01_reset;
 dc->realize = pflash_cfi01_realize;
 dc->props = pflash_cfi01_properties;
 dc->vmsd = &vmstate_pflash;
-- 
2.20.1

[Qemu-block] [PATCH 0/5] hw/block/pflash: Add DeviceReset() handlers

2019-05-05 Thread Philippe Mathieu-Daudé 
The pflash device lacks a reset() function.
When a machine is resetted, the flash might be in an
inconsistent state, leading to unexpected behavior:
https://bugzilla.redhat.com/show_bug.cgi?id=1678713

Resolve this issue by adding a DeviceReset() handler.

Both CFI01/CFI02 devices are fixed by this series.

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  hw/block/pflash_cfi01: Removed an unused timer
  hw/block/pflash_cfi01: Extract the pflash_reset() code
  hw/block/pflash_cfi01: Add the DeviceReset() handler
  hw/block/pflash_cfi02: Extract the pflash_reset() code
  hw/block/pflash_cfi02: Add the DeviceReset() handler

 hw/block/pflash_cfi01.c | 31 ---
 hw/block/pflash_cfi02.c | 31 +--
 2 files changed, 33 insertions(+), 29 deletions(-)

-- 
2.20.1

Re: [Qemu-block] [PATCH v4 10/10] block/pflash_cfi02: Use the chip erase time specified in the CFI table

2019-05-05 Thread Philippe Mathieu-Daudé 
On 4/26/19 6:26 PM, Stephen Checkoway wrote:
> When erasing the chip, use the typical time specified in the CFI table
> rather than arbitrarily selecting 5 seconds.
> 
> Since the currently unconfigurable value set in the table is 12, this
> means a chip erase takes 4096 ms so this isn't a big change in behavior.
> 
> Signed-off-by: Stephen Checkoway 
> ---
>  hw/block/pflash_cfi02.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index d9087cafff..76c8af4365 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -633,9 +633,9 @@ static void pflash_write(void *opaque, hwaddr offset, 
> uint64_t value,
>  pflash_update(pfl, 0, pfl->total_len);
>  }
>  set_dq7(pfl, 0x00);
> -/* Let's wait 5 seconds before chip erase is done */
> +/* Wait the time specified at CFI address 0x22. */
>  timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -  (NANOSECONDS_PER_SECOND * 5));
> +  (1ULL << pfl->cfi_table[0x22]) * SCALE_MS);
>  break;
>  case 0x30:
>  /* Sector erase */
> 

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé

Re: [Qemu-block] [PATCH 8/9] tcg/i386: add support for IBT

2019-05-05 Thread Richard Henderson 
On 5/4/19 5:05 AM, Paolo Bonzini wrote:
> Add endbr annotations before indirect branch targets.  This lets QEMU enable
> IBT even for TCG-enabled builds.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  Makefile.target   |  2 ++
>  configure |  9 +
>  include/qemu/cpuid.h  |  5 +
>  tcg/i386/tcg-target.inc.c | 19 +++
>  4 files changed, 35 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [PATCH 5/9] coroutine: add host specific coroutine backend for 64-bit s390

2019-05-05 Thread Richard Henderson 
On 5/4/19 5:05 AM, Paolo Bonzini wrote:
> +  "bras %%r3, 1f\n"/* source PC will be after the BR */ \
> +  "1: aghi %%r3, 12\n" /* 4 */  \
> +  "stg %%r3, %[SCRATCH](%%r1)\n"   /* 6 save switch-back PC */  \
> +  "br %%r4\n"  /* 2 jump to destination PC */   \

Better as

larl%r3, 2f
stg %r3, SCRATCH(%r1)
br  %r4
2:


r~

Re: [Qemu-block] [PATCH 4/9] coroutine: add host specific coroutine backend for 64-bit ARM

2019-05-05 Thread Richard Henderson 
On 5/4/19 5:05 AM, Paolo Bonzini wrote:
> The speedup is similar to x86, 120 ns vs 180 ns on an APM Mustang.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  configure|  2 +-
>  scripts/qemugdb/coroutine_asm.py |  6 -
>  util/Makefile.objs   |  2 ++
>  util/coroutine-asm.c | 45 
>  4 files changed, 53 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 

> +"ldr x30, [x1, %[SCRATCH]]\n"/* load destination PC */   \
> +"ldr x1, [x1, %[SP]]\n"  /* load destination SP */   \
> +"mov sp, x1\n"   \
> +"br x30\n"   \
> +"2: \n"  \

For future reference, "bti j" (aka hint #36) goes here,
for the aarch64 branch target identification extension.


r~

Re: [Qemu-block] [PATCH 3/9] coroutine: add host specific coroutine backend for 64-bit x86

2019-05-05 Thread Richard Henderson 
On 5/4/19 5:05 AM, Paolo Bonzini wrote:
> This backend is faster (100ns vs 150ns per switch on my laptop), but
> especially it will be possible to add CET support to it.  Most of the
> code is actually not architecture specific.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  configure|  10 ++
>  scripts/qemugdb/coroutine.py |   5 +-
>  scripts/qemugdb/coroutine_asm.py |  20 +++
>  util/Makefile.objs   |   1 +
>  util/coroutine-asm.c | 230 +++
>  5 files changed, 264 insertions(+), 2 deletions(-)
>  create mode 100644 scripts/qemugdb/coroutine_asm.py
>  create mode 100644 util/coroutine-asm.c

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [RFC PATCH] tests/qemu-iotests: re-format output to for make check-block

2019-05-05 Thread Thomas Huth 
On 03/05/2019 16.39, Alex Bennée wrote:
> This attempts to clean-up the output to better match the output of the
> rest of the QEMU check system. This includes:
> 
>   - formatting as "  TESTiotest: nnn"
>   - calculating time diff at the end
>   - only dumping config on failure
> 
> Signed-off-by: Alex Bennée 
> ---
>  tests/qemu-iotests/check | 71 +++-
>  1 file changed, 34 insertions(+), 37 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 922c5d1d3d..2ffc14113e 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -633,12 +633,6 @@ _wallclock()
>  date "+%H %M %S" | awk '{ print $1*3600 + $2*60 + $3 }'
>  }
>  
> -_timestamp()
> -{
> -now=$(date "+%T")
> -printf %s " [$now]"
> -}
> -
>  _wrapup()
>  {
>  if $showme
> @@ -709,19 +703,6 @@ trap "_wrapup; exit \$status" 0 1 2 3 15
>  FULL_IMGFMT_DETAILS=$(_full_imgfmt_details)
>  FULL_HOST_DETAILS=$(_full_platform_details)
>  
> -cat < -QEMU  -- "$QEMU_PROG" $QEMU_OPTIONS
> -QEMU_IMG  -- "$QEMU_IMG_PROG" $QEMU_IMG_OPTIONS
> -QEMU_IO   -- "$QEMU_IO_PROG" $QEMU_IO_OPTIONS
> -QEMU_NBD  -- "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS
> -IMGFMT-- $FULL_IMGFMT_DETAILS
> -IMGPROTO  -- $IMGPROTO
> -PLATFORM  -- $FULL_HOST_DETAILS
> -TEST_DIR  -- $TEST_DIR
> -SOCKET_SCM_HELPER -- $SOCKET_SCM_HELPER
> -
> -EOF

Maybe turn it into a function instead, so that it could also always be
printed when the script is run with the "-v" parameter?

>  seq="check"
>  
>  [ -n "$TESTS_REMAINING_LOG" ] && echo $list > $TESTS_REMAINING_LOG
> @@ -729,7 +710,9 @@ seq="check"
>  for seq in $list
>  do
>  err=false
> -printf %s "$seq"
> +reason=""
> +times=""
> +
>  if [ -n "$TESTS_REMAINING_LOG" ] ; then
>  sed -e "s/$seq//" -e 's/  / /' -e 's/^ *//' $TESTS_REMAINING_LOG > 
> $TESTS_REMAINING_LOG.tmp
>  mv $TESTS_REMAINING_LOG.tmp $TESTS_REMAINING_LOG
> @@ -738,7 +721,7 @@ do
>  
>  if $showme
>  then
> -echo
> +echo "  TESTiotest: $seq (not actually run)"

I wonder whether some other scripts depend on the output of "check -n"
... in that case, it make sense to only print the numbers, without the
additional strings here.

>  continue
>  elif [ -f expunged ] && $expunge && egrep "^$seq([ ]|\$)" 
> expunged >/dev/null
>  then
> @@ -753,17 +736,11 @@ do
>  # really going to try and run this one
>  #
>  rm -f $seq.out.bad
> -lasttime=$(sed -n -e "/^$seq /s/.* //p" <$TIMESTAMP_FILE)
> -if [ "X$lasttime" != X ]; then
> -printf %s " ${lasttime}s ..."
> -else
> -printf ""# prettier output with timestamps.
> -fi
>  rm -f core $seq.notrun
>  rm -f $seq.casenotrun
>  
>  start=$(_wallclock)
> -$timestamp && printf %s "[$(date "+%T")]"
> +$timestamp && times="[$(date "+%T")]"
>  
>  if [ "$(head -n 1 "$source_iotests/$seq")" == "#!/usr/bin/env 
> python" ]; then
>  run_command="$PYTHON $seq"
> @@ -781,26 +758,26 @@ do
>  $run_command >$tmp.out 2>&1)
>  fi
>  sts=$?
> -$timestamp && _timestamp
> +$timestamp && times="$times -> [$(date "+%T")]"
>  stop=$(_wallclock)
>  
>  if [ -f core ]
>  then
> -printf " [dumped core]"
>  mv core $seq.core
> +reason="dumped core $seq.core"
>  err=true
>  fi
>  
>  if [ -f $seq.notrun ]
>  then
> -$timestamp || printf " [not run] "
> -$timestamp && echo " [not run]" && printf %s "$seq -- "
> +$timestamp || reason="[not run]"
> +$timestamp && reason="[not run] $seq -- "
>  cat $seq.notrun
>  notrun="$notrun $seq"
>  else
>  if [ $sts -ne 0 ]
>  then
> -printf %s " [failed, exit status $sts]"
> +reason=$(printf %s "[failed, exit status $sts]")
>  err=true
>  fi
>  
> @@ -821,22 +798,27 @@ do
>  
>  if [ ! -f "$reference" ]
>  then
> -echo " - no qualified output"
> +reason=" - no qualified output"
>  err=true
>  else
>  if diff -w "$reference" $tmp.out >/dev/null 2>&1
>  then
> -echo ""
>  if $err
>  then
>  :
>  else
> -echo "$seq $(expr $stop - $start)" >>$tmp.time
> +lasttime=$(sed -n -e "/^$seq /s/.* //p" 
> <$TIMESTAMP_FILE)
> +thistime=$(expr $stop - $start)
> +echo "$seq $thistime" >>$tmp.time
> +
> +if [ "

Re: [Qemu-block] [RFC PATCH] tests/qemu-iotests: re-format output to for make check-block

2019-05-05 Thread Thomas Huth 
On 03/05/2019 18.15, Alex Bennée wrote:
> 
> Thomas Huth  writes:
> 
>> On 03/05/2019 16.39, Alex Bennée wrote:
>>> This attempts to clean-up the output to better match the output of the
>>> rest of the QEMU check system. This includes:
>>>
>>>   - formatting as "  TESTiotest: nnn"
>>>   - calculating time diff at the end
>>>   - only dumping config on failure
>>>
>>> Signed-off-by: Alex Bennée 
>>> ---
>>>  tests/qemu-iotests/check | 71 +++-
>>>  1 file changed, 34 insertions(+), 37 deletions(-)
>>
>> Thanks for tackling this! The output now looks nicer indeed if you run
>> "make check-qtest check-block -j8". However, if you add a "V=1" at the
>> end of the command line, the outputs look quite different again...
>>
>> That's why I thought that having a TAP mode for the check script could
>> be a good idea, too. Then we could pipe the output through the
>> tap-driver.pl script, too, so we get uniform output for all tests...?
> 
> That would probably be a cleaner approach. What would be even better is
> somehow expanding the list of tests at make time so you could run your
> tests in parallel.

I agree that this might be the ultimate solution ... but I'm not sure
whether the iotests are really ready for being run in parallel yet, so
it will likely take quite some while 'till we are at that point. With
that in mind (and thus also not sure yet whether my TAP idea is really
the right approach), your patch is certainly a good interim solution
which we should try to get merged, too, when my "make check" series gets
accepted?

> I did wonder how useful the timing stuff was to developers.

Yes, me too ... maybe the block layer folks can comment on that one...?

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH 0/9] Assembly coroutine backend and x86 CET support

2019-05-05 Thread Alex Bennée 


Paolo Bonzini  writes:

> *** BLURB HERE ***

I assume there was going to be a bit more background here?

--
Alex Bennée

Re: [Qemu-block] [PATCH] pflash: Only read non-zero parts of backend image

2019-05-05 Thread Peter Maydell 
On Sun, 5 May 2019 at 08:02, Xiang Zheng  wrote:
>
> Currently we fill the memory space with two 64MB NOR images when
> using persistent UEFI variables on virt board. Actually we only use
> a very small(non-zero) part of the memory while the rest significant
> large(zero) part of memory is wasted.
>
> So this patch checks the block status and only writes the non-zero part
> into memory. This requires pflash devices to use sparse files for
> backends.

Do you mean "pflash devices will no longer work if the file
that is backing them is not sparse", or just "if the file that
is backing them is not sparse then you won't get the benefit
of using less memory" ?

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH] qcow2: Fix error handling in the compression code

2019-05-05 Thread no-reply 
Patchew URL: https://patchew.org/QEMU/20190430100802.15368-1-be...@igalia.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190430100802.15368-1-be...@igalia.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[Qemu-block] [PATCH] pflash: Only read non-zero parts of backend image

2019-05-05 Thread Xiang Zheng 
Currently we fill the memory space with two 64MB NOR images when
using persistent UEFI variables on virt board. Actually we only use
a very small(non-zero) part of the memory while the rest significant
large(zero) part of memory is wasted.

So this patch checks the block status and only writes the non-zero part
into memory. This requires pflash devices to use sparse files for
backends.

Suggested-by: Kevin Wolf 
Signed-off-by: Xiang Zheng 
---
 hw/block/block.c | 40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index bf56c76..3cb9d4c 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -15,6 +15,44 @@
 #include "qapi/qapi-types-block.h"
 
 /*
+ * Read the non-zeroes parts of @blk into @buf
+ * Reading all of the @blk is expensive if the zeroes parts of @blk
+ * is large enough. Therefore check the block status and only write
+ * the non-zeroes block into @buf.
+ *
+ * Return 0 on success, non-zero on error.
+ */
+static int blk_pread_nonzeroes(BlockBackend *blk, void *buf)
+{
+int ret;
+int64_t target_size, bytes, offset = 0;
+BlockDriverState *bs = blk_bs(blk);
+
+target_size = bdrv_getlength(bs);
+if (target_size < 0) {
+return target_size;
+}
+
+for (;;) {
+bytes = MIN(target_size - offset, BDRV_REQUEST_MAX_SECTORS);
+if (bytes <= 0) {
+return 0;
+}
+ret = bdrv_block_status(bs, offset, bytes, &bytes, NULL, NULL);
+if (ret < 0) {
+return ret;
+}
+if (!(ret & BDRV_BLOCK_ZERO)) {
+ret = bdrv_pread(bs->file, offset, (uint8_t *) buf + offset, 
bytes);
+if (ret < 0) {
+return ret;
+}
+}
+offset += bytes;
+}
+}
+
+/*
  * Read the entire contents of @blk into @buf.
  * @blk's contents must be @size bytes, and @size must be at most
  * BDRV_REQUEST_MAX_BYTES.
@@ -53,7 +91,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void 
*buf, hwaddr size,
  * block device and read only on demand.
  */
 assert(size <= BDRV_REQUEST_MAX_BYTES);
-ret = blk_pread(blk, 0, buf, size);
+ret = blk_pread_nonzeroes(blk, buf);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "can't read block backend");
 return false;
-- 
1.8.3.1