* [Qemu-devel] [PATCH 0/3] m25p80: various fixes
@ 2016-06-28 8:39 Paolo Bonzini
2016-06-28 8:39 ` [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Paolo Bonzini @ 2016-06-28 8:39 UTC (permalink / raw)
To: qemu-devel; +Cc: clg
The first causes a failure in the new qtest. The second fixes a possible
out-of-bounds access, and the third fixes again half of the same
out-of-bounds access.
Paolo Bonzini (3):
m25p80: do not put iovec on the stack
m25p80: avoid out of bounds accesses
m25p80: change cur_addr to 32 bit integer
hw/block/m25p80.c | 54 +++++++++++++++++++++++++++++-------------------------
1 file changed, 29 insertions(+), 25 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack
2016-06-28 8:39 [Qemu-devel] [PATCH 0/3] m25p80: various fixes Paolo Bonzini
@ 2016-06-28 8:39 ` Paolo Bonzini
2016-06-28 8:53 ` Cédric Le Goater
2016-06-28 14:33 ` Eric Blake
2016-06-28 8:39 ` [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses Paolo Bonzini
2016-06-28 8:39 ` [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer Paolo Bonzini
2 siblings, 2 replies; 12+ messages in thread
From: Paolo Bonzini @ 2016-06-28 8:39 UTC (permalink / raw)
To: qemu-devel; +Cc: clg
When doing a read-modify-write cycle, QEMU uses the iovec after returning
from blk_aio_pwritev. m25p80 puts the iovec on the stack of blk_aio_pwritev's
caller, which causes trouble in this case. This has been a problem
since commit 243e6f6 ("m25p80: Switch to byte-based block access",
2016-05-12) started doing writes at a smaller granularity than 512 bytes.
In principle however it could have broken before when using -drive
if=mtd,cache=none on a disk with 4K native sectors.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/block/m25p80.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 09b4767..dd6714d 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -446,6 +446,11 @@ static inline Manufacturer get_man(Flash *s)
static void blk_sync_complete(void *opaque, int ret)
{
+ QEMUIOVector *iov = opaque;
+
+ qemu_iovec_destroy(iov);
+ g_free(iov);
+
/* do nothing. Masters do not directly interact with the backing store,
* only the working copy so no mutexing required.
*/
@@ -453,31 +458,31 @@ static void blk_sync_complete(void *opaque, int ret)
static void flash_sync_page(Flash *s, int page)
{
- QEMUIOVector iov;
+ QEMUIOVector *iov = g_new(QEMUIOVector, 1);
if (!s->blk || blk_is_read_only(s->blk)) {
return;
}
- qemu_iovec_init(&iov, 1);
- qemu_iovec_add(&iov, s->storage + page * s->pi->page_size,
+ qemu_iovec_init(iov, 1);
+ qemu_iovec_add(iov, s->storage + page * s->pi->page_size,
s->pi->page_size);
- blk_aio_pwritev(s->blk, page * s->pi->page_size, &iov, 0,
- blk_sync_complete, NULL);
+ blk_aio_pwritev(s->blk, page * s->pi->page_size, iov, 0,
+ blk_sync_complete, iov);
}
static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
{
- QEMUIOVector iov;
+ QEMUIOVector *iov = g_new(QEMUIOVector, 1);
if (!s->blk || blk_is_read_only(s->blk)) {
return;
}
assert(!(len % BDRV_SECTOR_SIZE));
- qemu_iovec_init(&iov, 1);
- qemu_iovec_add(&iov, s->storage + off, len);
- blk_aio_pwritev(s->blk, off, &iov, 0, blk_sync_complete, NULL);
+ qemu_iovec_init(iov, 1);
+ qemu_iovec_add(iov, s->storage + off, len);
+ blk_aio_pwritev(s->blk, off, iov, 0, blk_sync_complete, iov);
}
static void flash_erase(Flash *s, int offset, FlashCMD cmd)
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses
2016-06-28 8:39 [Qemu-devel] [PATCH 0/3] m25p80: various fixes Paolo Bonzini
2016-06-28 8:39 ` [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack Paolo Bonzini
@ 2016-06-28 8:39 ` Paolo Bonzini
2016-06-28 9:05 ` Cédric Le Goater
2016-06-28 8:39 ` [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer Paolo Bonzini
2 siblings, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2016-06-28 8:39 UTC (permalink / raw)
To: qemu-devel; +Cc: clg
s->cur_addr can be made to point outside s->storage, either by
writing a value >= 128 to s->ear (because s->ear * MAX_3BYTES_SIZE
is a signed integer and sign-extends into the 64-bit cur_addr),
or just by writing an address beyond the size of the flash being
emulated. Avoid the sign extension to make the code cleaner, and
on top of that mask s->cur_addr to s->size.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/block/m25p80.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index dd6714d..76a9bcf 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -586,18 +586,16 @@ static inline int get_addr_length(Flash *s)
static void complete_collecting_data(Flash *s)
{
- int i;
-
- s->cur_addr = 0;
+ int i, n;
- for (i = 0; i < get_addr_length(s); ++i) {
+ n = get_addr_length(s);
+ s->cur_addr = (n == 3 ? s->ear : 0);
+ for (i = 0; i < n; ++i) {
s->cur_addr <<= 8;
s->cur_addr |= s->data[i];
}
- if (get_addr_length(s) == 3) {
- s->cur_addr += s->ear * MAX_3BYTES_SIZE;
- }
+ s->cur_addr &= s->size - 1;
s->state = STATE_IDLE;
@@ -1099,14 +1097,14 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8 "\n",
s->cur_addr, (uint8_t)tx);
flash_write8(s, s->cur_addr, (uint8_t)tx);
- s->cur_addr++;
+ s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
break;
case STATE_READ:
r = s->storage[s->cur_addr];
DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
(uint8_t)r);
- s->cur_addr = (s->cur_addr + 1) % s->size;
+ s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
break;
case STATE_COLLECTING_DATA:
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer
2016-06-28 8:39 [Qemu-devel] [PATCH 0/3] m25p80: various fixes Paolo Bonzini
2016-06-28 8:39 ` [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack Paolo Bonzini
2016-06-28 8:39 ` [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses Paolo Bonzini
@ 2016-06-28 8:39 ` Paolo Bonzini
2016-06-28 9:18 ` Cédric Le Goater
2 siblings, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2016-06-28 8:39 UTC (permalink / raw)
To: qemu-devel; +Cc: clg
The maximum amount of storage that can be addressed by the m25p80 command
set is 4 GiB. However, cur_addr is currently a 64-bit integer. To avoid
further problems related to sign extension of signed 32-bit integer
expressions, change cur_addr to a 32 bit integer. Preserve migration
format by adding a dummy 4-byte field in place of the (big-endian)
high four bytes in the formerly 64-bit cur_addr field.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/block/m25p80.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 76a9bcf..7668b22 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -389,7 +389,7 @@ typedef struct Flash {
uint32_t pos;
uint8_t needed_bytes;
uint8_t cmd_in_progress;
- uint64_t cur_addr;
+ uint32_t cur_addr;
uint32_t nonvolatile_cfg;
/* Configuration register for Macronix */
uint32_t volatile_cfg;
@@ -535,9 +535,9 @@ static inline void flash_sync_dirty(Flash *s, int64_t newpage)
}
static inline
-void flash_write8(Flash *s, uint64_t addr, uint8_t data)
+void flash_write8(Flash *s, uint32_t addr, uint8_t data)
{
- int64_t page = addr / s->pi->page_size;
+ uint32_t page = addr / s->pi->page_size;
uint8_t prev = s->storage[s->cur_addr];
if (!s->write_enable) {
@@ -545,7 +545,7 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
}
if ((prev ^ data) & data) {
- DB_PRINT_L(1, "programming zero to one! addr=%" PRIx64 " %" PRIx8
+ DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 " %" PRIx8
" -> %" PRIx8 "\n", addr, prev, data);
}
@@ -1094,7 +1094,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
switch (s->state) {
case STATE_PAGE_PROGRAM:
- DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8 "\n",
+ DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8 "\n",
s->cur_addr, (uint8_t)tx);
flash_write8(s, s->cur_addr, (uint8_t)tx);
s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
@@ -1102,7 +1102,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
case STATE_READ:
r = s->storage[s->cur_addr];
- DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
+ DB_PRINT_L(1, "READ 0x%" PRIx32 "=%" PRIx8 "\n", s->cur_addr,
(uint8_t)r);
s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
break;
@@ -1199,7 +1199,8 @@ static const VMStateDescription vmstate_m25p80 = {
VMSTATE_UINT32(pos, Flash),
VMSTATE_UINT8(needed_bytes, Flash),
VMSTATE_UINT8(cmd_in_progress, Flash),
- VMSTATE_UINT64(cur_addr, Flash),
+ VMSTATE_UNUSED(4),
+ VMSTATE_UINT32(cur_addr, Flash),
VMSTATE_BOOL(write_enable, Flash),
VMSTATE_BOOL_V(reset_enable, Flash, 2),
VMSTATE_UINT8_V(ear, Flash, 2),
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack
2016-06-28 8:39 ` [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack Paolo Bonzini
@ 2016-06-28 8:53 ` Cédric Le Goater
2016-06-28 10:02 ` Cédric Le Goater
2016-06-28 14:33 ` Eric Blake
1 sibling, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2016-06-28 8:53 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> When doing a read-modify-write cycle, QEMU uses the iovec after returning
> from blk_aio_pwritev. m25p80 puts the iovec on the stack of blk_aio_pwritev's
> caller, which causes trouble in this case. This has been a problem
> since commit 243e6f6 ("m25p80: Switch to byte-based block access",
> 2016-05-12) started doing writes at a smaller granularity than 512 bytes.
> In principle however it could have broken before when using -drive
> if=mtd,cache=none on a disk with 4K native sectors.
Ah ! Thanks. That was a problem I was seeing.
I was thinking we could just do synchronous writes :
https://github.com/legoater/qemu/commit/aef3fe4db3be632077c581541fe30b4e36b5a6f7
and enable snapshotting like :
https://github.com/legoater/qemu/commit/b90ccab7873fd3538b47396ec7c3ae35c8e13270
Thanks,
C.
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/block/m25p80.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 09b4767..dd6714d 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -446,6 +446,11 @@ static inline Manufacturer get_man(Flash *s)
>
> static void blk_sync_complete(void *opaque, int ret)
> {
> + QEMUIOVector *iov = opaque;
> +
> + qemu_iovec_destroy(iov);
> + g_free(iov);
> +
> /* do nothing. Masters do not directly interact with the backing store,
> * only the working copy so no mutexing required.
> */
> @@ -453,31 +458,31 @@ static void blk_sync_complete(void *opaque, int ret)
>
> static void flash_sync_page(Flash *s, int page)
> {
> - QEMUIOVector iov;
> + QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>
> if (!s->blk || blk_is_read_only(s->blk)) {
> return;
> }
>
> - qemu_iovec_init(&iov, 1);
> - qemu_iovec_add(&iov, s->storage + page * s->pi->page_size,
> + qemu_iovec_init(iov, 1);
> + qemu_iovec_add(iov, s->storage + page * s->pi->page_size,
> s->pi->page_size);
> - blk_aio_pwritev(s->blk, page * s->pi->page_size, &iov, 0,
> - blk_sync_complete, NULL);
> + blk_aio_pwritev(s->blk, page * s->pi->page_size, iov, 0,
> + blk_sync_complete, iov);
> }
>
> static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
> {
> - QEMUIOVector iov;
> + QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>
> if (!s->blk || blk_is_read_only(s->blk)) {
> return;
> }
>
> assert(!(len % BDRV_SECTOR_SIZE));
> - qemu_iovec_init(&iov, 1);
> - qemu_iovec_add(&iov, s->storage + off, len);
> - blk_aio_pwritev(s->blk, off, &iov, 0, blk_sync_complete, NULL);
> + qemu_iovec_init(iov, 1);
> + qemu_iovec_add(iov, s->storage + off, len);
> + blk_aio_pwritev(s->blk, off, iov, 0, blk_sync_complete, iov);
> }
>
> static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses
2016-06-28 8:39 ` [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses Paolo Bonzini
@ 2016-06-28 9:05 ` Cédric Le Goater
2016-06-28 11:42 ` Paolo Bonzini
0 siblings, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2016-06-28 9:05 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> s->cur_addr can be made to point outside s->storage, either by
> writing a value >= 128 to s->ear (because s->ear * MAX_3BYTES_SIZE
> is a signed integer and sign-extends into the 64-bit cur_addr),
> or just by writing an address beyond the size of the flash being
> emulated. Avoid the sign extension to make the code cleaner, and
> on top of that mask s->cur_addr to s->size.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Yes. We can trash memory very easily with the m25p80 object so we
clearly want this fix.
Maybe, we could add a guest error when going beyond storage and
the case doing flash_write8() needs some care also. Anyhow, that
can be covered in another patch.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/block/m25p80.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index dd6714d..76a9bcf 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -586,18 +586,16 @@ static inline int get_addr_length(Flash *s)
>
> static void complete_collecting_data(Flash *s)
> {
> - int i;
> -
> - s->cur_addr = 0;
> + int i, n;
>
> - for (i = 0; i < get_addr_length(s); ++i) {
> + n = get_addr_length(s);
> + s->cur_addr = (n == 3 ? s->ear : 0);
> + for (i = 0; i < n; ++i) {
> s->cur_addr <<= 8;
> s->cur_addr |= s->data[i];
> }
>
> - if (get_addr_length(s) == 3) {
> - s->cur_addr += s->ear * MAX_3BYTES_SIZE;
> - }
> + s->cur_addr &= s->size - 1;
>
> s->state = STATE_IDLE;
>
> @@ -1099,14 +1097,14 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
> DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8 "\n",
> s->cur_addr, (uint8_t)tx);
> flash_write8(s, s->cur_addr, (uint8_t)tx);
> - s->cur_addr++;
> + s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> break;
>
> case STATE_READ:
> r = s->storage[s->cur_addr];
> DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
> (uint8_t)r);
> - s->cur_addr = (s->cur_addr + 1) % s->size;
> + s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> break;
>
> case STATE_COLLECTING_DATA:
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer
2016-06-28 8:39 ` [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer Paolo Bonzini
@ 2016-06-28 9:18 ` Cédric Le Goater
2016-06-28 11:41 ` Paolo Bonzini
0 siblings, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2016-06-28 9:18 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> The maximum amount of storage that can be addressed by the m25p80 command
> set is 4 GiB. However, cur_addr is currently a 64-bit integer. To avoid
> further problems related to sign extension of signed 32-bit integer
> expressions, change cur_addr to a 32 bit integer. Preserve migration
> format by adding a dummy 4-byte field in place of the (big-endian)
> high four bytes in the formerly 64-bit cur_addr field.
I do not think that migration ever worked before. did it ?
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/block/m25p80.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 76a9bcf..7668b22 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -389,7 +389,7 @@ typedef struct Flash {
> uint32_t pos;
> uint8_t needed_bytes;
> uint8_t cmd_in_progress;
> - uint64_t cur_addr;
> + uint32_t cur_addr;
> uint32_t nonvolatile_cfg;
> /* Configuration register for Macronix */
> uint32_t volatile_cfg;
> @@ -535,9 +535,9 @@ static inline void flash_sync_dirty(Flash *s, int64_t newpage)
> }
>
> static inline
> -void flash_write8(Flash *s, uint64_t addr, uint8_t data)
> +void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> {
> - int64_t page = addr / s->pi->page_size;
> + uint32_t page = addr / s->pi->page_size;
> uint8_t prev = s->storage[s->cur_addr];
This routine needs a cleanup. It takes an 'addr' parameter (it is called
with s->cur_addr) and uses s->cur_addr at the same time.
C.
> if (!s->write_enable) {
> @@ -545,7 +545,7 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
> }
>
> if ((prev ^ data) & data) {
> - DB_PRINT_L(1, "programming zero to one! addr=%" PRIx64 " %" PRIx8
> + DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 " %" PRIx8
> " -> %" PRIx8 "\n", addr, prev, data);
> }
>
> @@ -1094,7 +1094,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
> switch (s->state) {
>
> case STATE_PAGE_PROGRAM:
> - DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8 "\n",
> + DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8 "\n",
> s->cur_addr, (uint8_t)tx);
> flash_write8(s, s->cur_addr, (uint8_t)tx);
> s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> @@ -1102,7 +1102,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>
> case STATE_READ:
> r = s->storage[s->cur_addr];
> - DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
> + DB_PRINT_L(1, "READ 0x%" PRIx32 "=%" PRIx8 "\n", s->cur_addr,
> (uint8_t)r);
> s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> break;
> @@ -1199,7 +1199,8 @@ static const VMStateDescription vmstate_m25p80 = {
> VMSTATE_UINT32(pos, Flash),
> VMSTATE_UINT8(needed_bytes, Flash),
> VMSTATE_UINT8(cmd_in_progress, Flash),
> - VMSTATE_UINT64(cur_addr, Flash),
> + VMSTATE_UNUSED(4),
> + VMSTATE_UINT32(cur_addr, Flash),
> VMSTATE_BOOL(write_enable, Flash),
> VMSTATE_BOOL_V(reset_enable, Flash, 2),
> VMSTATE_UINT8_V(ear, Flash, 2),
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack
2016-06-28 8:53 ` Cédric Le Goater
@ 2016-06-28 10:02 ` Cédric Le Goater
0 siblings, 0 replies; 12+ messages in thread
From: Cédric Le Goater @ 2016-06-28 10:02 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 06/28/2016 10:53 AM, Cédric Le Goater wrote:
> On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
>> When doing a read-modify-write cycle, QEMU uses the iovec after returning
>> from blk_aio_pwritev. m25p80 puts the iovec on the stack of blk_aio_pwritev's
>> caller, which causes trouble in this case. This has been a problem
>> since commit 243e6f6 ("m25p80: Switch to byte-based block access",
>> 2016-05-12) started doing writes at a smaller granularity than 512 bytes.
>> In principle however it could have broken before when using -drive
>> if=mtd,cache=none on a disk with 4K native sectors.
>
> Ah ! Thanks. That was a problem I was seeing.
>
>
> I was thinking we could just do synchronous writes :
>
> https://github.com/legoater/qemu/commit/aef3fe4db3be632077c581541fe30b4e36b5a6f7
>
> and enable snapshotting like :
>
> https://github.com/legoater/qemu/commit/b90ccab7873fd3538b47396ec7c3ae35c8e13270
>
Anyhow, I tested this patch on a P8/ppc64le system on which the
assertion was very systematic :
bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed.
It is gone.
Reviewed-and-tested-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> hw/block/m25p80.c | 23 ++++++++++++++---------
>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 09b4767..dd6714d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -446,6 +446,11 @@ static inline Manufacturer get_man(Flash *s)
>>
>> static void blk_sync_complete(void *opaque, int ret)
>> {
>> + QEMUIOVector *iov = opaque;
>> +
>> + qemu_iovec_destroy(iov);
>> + g_free(iov);
>> +
>> /* do nothing. Masters do not directly interact with the backing store,
>> * only the working copy so no mutexing required.
>> */
>> @@ -453,31 +458,31 @@ static void blk_sync_complete(void *opaque, int ret)
>>
>> static void flash_sync_page(Flash *s, int page)
>> {
>> - QEMUIOVector iov;
>> + QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>>
>> if (!s->blk || blk_is_read_only(s->blk)) {
>> return;
>> }
>>
>> - qemu_iovec_init(&iov, 1);
>> - qemu_iovec_add(&iov, s->storage + page * s->pi->page_size,
>> + qemu_iovec_init(iov, 1);
>> + qemu_iovec_add(iov, s->storage + page * s->pi->page_size,
>> s->pi->page_size);
>> - blk_aio_pwritev(s->blk, page * s->pi->page_size, &iov, 0,
>> - blk_sync_complete, NULL);
>> + blk_aio_pwritev(s->blk, page * s->pi->page_size, iov, 0,
>> + blk_sync_complete, iov);
>> }
>>
>> static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
>> {
>> - QEMUIOVector iov;
>> + QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>>
>> if (!s->blk || blk_is_read_only(s->blk)) {
>> return;
>> }
>>
>> assert(!(len % BDRV_SECTOR_SIZE));
>> - qemu_iovec_init(&iov, 1);
>> - qemu_iovec_add(&iov, s->storage + off, len);
>> - blk_aio_pwritev(s->blk, off, &iov, 0, blk_sync_complete, NULL);
>> + qemu_iovec_init(iov, 1);
>> + qemu_iovec_add(iov, s->storage + off, len);
>> + blk_aio_pwritev(s->blk, off, iov, 0, blk_sync_complete, iov);
>> }
>>
>> static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer
2016-06-28 9:18 ` Cédric Le Goater
@ 2016-06-28 11:41 ` Paolo Bonzini
0 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2016-06-28 11:41 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-devel
> On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> > The maximum amount of storage that can be addressed by the m25p80 command
> > set is 4 GiB. However, cur_addr is currently a 64-bit integer. To avoid
> > further problems related to sign extension of signed 32-bit integer
> > expressions, change cur_addr to a 32 bit integer. Preserve migration
> > format by adding a dummy 4-byte field in place of the (big-endian)
> > high four bytes in the formerly 64-bit cur_addr field.
>
> I do not think that migration ever worked before. did it ?
Who knows. :) But it is pretty easy to not break it further...
Paolo
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> > ---
> > hw/block/m25p80.c | 15 ++++++++-------
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 76a9bcf..7668b22 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -389,7 +389,7 @@ typedef struct Flash {
> > uint32_t pos;
> > uint8_t needed_bytes;
> > uint8_t cmd_in_progress;
> > - uint64_t cur_addr;
> > + uint32_t cur_addr;
> > uint32_t nonvolatile_cfg;
> > /* Configuration register for Macronix */
> > uint32_t volatile_cfg;
> > @@ -535,9 +535,9 @@ static inline void flash_sync_dirty(Flash *s, int64_t
> > newpage)
> > }
> >
> > static inline
> > -void flash_write8(Flash *s, uint64_t addr, uint8_t data)
> > +void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> > {
> > - int64_t page = addr / s->pi->page_size;
> > + uint32_t page = addr / s->pi->page_size;
> > uint8_t prev = s->storage[s->cur_addr];
>
> This routine needs a cleanup. It takes an 'addr' parameter (it is called
> with s->cur_addr) and uses s->cur_addr at the same time.
>
> C.
>
> > if (!s->write_enable) {
> > @@ -545,7 +545,7 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t
> > data)
> > }
> >
> > if ((prev ^ data) & data) {
> > - DB_PRINT_L(1, "programming zero to one! addr=%" PRIx64 " %" PRIx8
> > + DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 " %" PRIx8
> > " -> %" PRIx8 "\n", addr, prev, data);
> > }
> >
> > @@ -1094,7 +1094,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss,
> > uint32_t tx)
> > switch (s->state) {
> >
> > case STATE_PAGE_PROGRAM:
> > - DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8
> > "\n",
> > + DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8
> > "\n",
> > s->cur_addr, (uint8_t)tx);
> > flash_write8(s, s->cur_addr, (uint8_t)tx);
> > s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> > @@ -1102,7 +1102,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss,
> > uint32_t tx)
> >
> > case STATE_READ:
> > r = s->storage[s->cur_addr];
> > - DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
> > + DB_PRINT_L(1, "READ 0x%" PRIx32 "=%" PRIx8 "\n", s->cur_addr,
> > (uint8_t)r);
> > s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> > break;
> > @@ -1199,7 +1199,8 @@ static const VMStateDescription vmstate_m25p80 = {
> > VMSTATE_UINT32(pos, Flash),
> > VMSTATE_UINT8(needed_bytes, Flash),
> > VMSTATE_UINT8(cmd_in_progress, Flash),
> > - VMSTATE_UINT64(cur_addr, Flash),
> > + VMSTATE_UNUSED(4),
> > + VMSTATE_UINT32(cur_addr, Flash),
> > VMSTATE_BOOL(write_enable, Flash),
> > VMSTATE_BOOL_V(reset_enable, Flash, 2),
> > VMSTATE_UINT8_V(ear, Flash, 2),
> >
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses
2016-06-28 9:05 ` Cédric Le Goater
@ 2016-06-28 11:42 ` Paolo Bonzini
2016-06-28 12:00 ` [Qemu-devel] " Krzeminski, Marcin (Nokia - PL/Wroclaw)
0 siblings, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2016-06-28 11:42 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-devel
----- Original Message -----
> From: "Cédric Le Goater" <clg@kaod.org>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> Sent: Tuesday, June 28, 2016 11:05:24 AM
> Subject: Re: [PATCH 2/3] m25p80: avoid out of bounds accesses
>
> On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> > s->cur_addr can be made to point outside s->storage, either by
> > writing a value >= 128 to s->ear (because s->ear * MAX_3BYTES_SIZE
> > is a signed integer and sign-extends into the 64-bit cur_addr),
> > or just by writing an address beyond the size of the flash being
> > emulated. Avoid the sign extension to make the code cleaner, and
> > on top of that mask s->cur_addr to s->size.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Yes. We can trash memory very easily with the m25p80 object so we
> clearly want this fix.
>
> Maybe, we could add a guest error when going beyond storage and
> the case doing flash_write8() needs some care also.
I don't know, it's quite possible that real hardware does an AND
simply because some address lines are not connected anywhere.
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] m25p80: avoid out of bounds accesses
2016-06-28 11:42 ` Paolo Bonzini
@ 2016-06-28 12:00 ` Krzeminski, Marcin (Nokia - PL/Wroclaw)
0 siblings, 0 replies; 12+ messages in thread
From: Krzeminski, Marcin (Nokia - PL/Wroclaw) @ 2016-06-28 12:00 UTC (permalink / raw)
To: Paolo Bonzini, Cédric Le Goater; +Cc: qemu-devel
> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Paolo
> Bonzini
> Sent: Tuesday, June 28, 2016 1:42 PM
> To: Cédric Le Goater <clg@kaod.org>
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds
> accesses
>
>
>
> ----- Original Message -----
> > From: "Cédric Le Goater" <clg@kaod.org>
> > To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> > Sent: Tuesday, June 28, 2016 11:05:24 AM
> > Subject: Re: [PATCH 2/3] m25p80: avoid out of bounds accesses
> >
> > On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> > > s->cur_addr can be made to point outside s->storage, either by
> > > writing a value >= 128 to s->ear (because s->ear * MAX_3BYTES_SIZE
> > > is a signed integer and sign-extends into the 64-bit cur_addr), or
> > > just by writing an address beyond the size of the flash being
> > > emulated. Avoid the sign extension to make the code cleaner, and on
> > > top of that mask s->cur_addr to s->size.
> > >
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >
> > Yes. We can trash memory very easily with the m25p80 object so we
> > clearly want this fix.
> >
> > Maybe, we could add a guest error when going beyond storage and the
> > case doing flash_write8() needs some care also.
>
> I don't know, it's quite possible that real hardware does an AND simply
> because some address lines are not connected anywhere.
>
> Paolo
Real HW wraps when address reach end of the memory (it could be
configurable for some devices).
Reviewed by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
Regards,
Marcin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack
2016-06-28 8:39 ` [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack Paolo Bonzini
2016-06-28 8:53 ` Cédric Le Goater
@ 2016-06-28 14:33 ` Eric Blake
1 sibling, 0 replies; 12+ messages in thread
From: Eric Blake @ 2016-06-28 14:33 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: clg
[-- Attachment #1: Type: text/plain, Size: 920 bytes --]
On 06/28/2016 02:39 AM, Paolo Bonzini wrote:
> When doing a read-modify-write cycle, QEMU uses the iovec after returning
> from blk_aio_pwritev. m25p80 puts the iovec on the stack of blk_aio_pwritev's
> caller, which causes trouble in this case. This has been a problem
> since commit 243e6f6 ("m25p80: Switch to byte-based block access",
> 2016-05-12) started doing writes at a smaller granularity than 512 bytes.
> In principle however it could have broken before when using -drive
> if=mtd,cache=none on a disk with 4K native sectors.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/block/m25p80.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
and thanks for tracking this one down
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-06-28 14:34 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-28 8:39 [Qemu-devel] [PATCH 0/3] m25p80: various fixes Paolo Bonzini
2016-06-28 8:39 ` [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack Paolo Bonzini
2016-06-28 8:53 ` Cédric Le Goater
2016-06-28 10:02 ` Cédric Le Goater
2016-06-28 14:33 ` Eric Blake
2016-06-28 8:39 ` [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses Paolo Bonzini
2016-06-28 9:05 ` Cédric Le Goater
2016-06-28 11:42 ` Paolo Bonzini
2016-06-28 12:00 ` [Qemu-devel] " Krzeminski, Marcin (Nokia - PL/Wroclaw)
2016-06-28 8:39 ` [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer Paolo Bonzini
2016-06-28 9:18 ` Cédric Le Goater
2016-06-28 11:41 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.