Re: [PATCH v2 5/6] hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

[Bin Meng]

Hi Philippe,

On Fri, Feb 19, 2021 at 2:03 AM Philippe Mathieu-Daudé  wrote:
>
> On 2/18/21 6:09 PM, Philippe Mathieu-Daudé wrote:
> > On 2/16/21 4:46 AM, Bin Meng wrote:
> >> The codes to limit the maximum block size is only necessary when
> >> SDHC_BLKSIZE register is writable.
>
> Per "SD Command Generation":
>
>   The Host Driver should not read the SDMA System Address, Block Size
>   and Block Count registers during a data transaction unless the
>   transfer is stopped because the value is changing and not stable.
>   To prevent destruction of registers using data transfer when issuing
>   command, the 32-bit Block Count, Block Size, 16-bit Block Count and
>   Transfer Mode registers shall be write protected by the Host
>   Controller while Command Inhibit (DAT) is set to 1 in the Present
>   State register.
>
> Shouldn't we check for !(s->prnsts & SDHC_DATA_INHIBIT) instead?

Yes, for accurate emulation I think we should.

Current implementation uses !(s->prnsts & (SDHC_DOING_READ |
SDHC_DOING_WRITE)) which eventually is correct, because:

SDHC_DATA_INHIBIT bit is set if either SDHC_DAT_LINE_ACTIVE or
SDHC_DOING_READ is set (SD Host Controller Spec v7.00 chapter 2.2.9
Present State Register)

SDHC_DAT_LINE_ACTIVE bit is set after the end bit of read or write
command, and after end bit of read or write command will generate
SDHC_DOING_READ or SDHC_DOING_WRITE (SD Host Controller Spec v7.00
chapter 2.2.9 Present State Register)

Regards,
Bin

Re: [PATCH v2 6/6] hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

[Bin Meng]

Hi Philippe,

On Fri, Feb 19, 2021 at 2:06 AM Philippe Mathieu-Daudé  wrote:
>
> Hi Bin,
>
> On 2/16/21 4:46 AM, Bin Meng wrote:
> > If the block size is programmed to a different value from the
> > previous one, reset the data pointer of s->fifo_buffer[] so that
> > s->fifo_buffer[] can be filled in using the new block size in
> > the next transfer.
> >
> > With this fix, the following reproducer:
> >
> > outl 0xcf8 0x80001010
> > outl 0xcfc 0xe000
> > outl 0xcf8 0x80001001
> > outl 0xcfc 0x0600
> > write 0xe02c 0x1 0x05
> > write 0xe005 0x1 0x02
> > write 0xe007 0x1 0x01
> > write 0xe028 0x1 0x10
> > write 0x0 0x1 0x23
> > write 0x2 0x1 0x08
> > write 0xe00c 0x1 0x01
> > write 0xe00e 0x1 0x20
> > write 0xe00f 0x1 0x00
> > write 0xe00c 0x1 0x32
> > write 0xe004 0x2 0x0200
> > write 0xe028 0x1 0x00
> > write 0xe003 0x1 0x40
> >
> > cannot be reproduced with the following QEMU command line:
> >
> > $ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
> >   -nodefaults -device sdhci-pci,sd-spec-version=3 \
> >   -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
> >   -device sd-card,drive=mydrive -qtest stdio
> >
> > Cc: qemu-sta...@nongnu.org
> > Fixes: CVE-2020-17380
> > Fixes: CVE-2020-25085
> > Fixes: CVE-2021-3409
> > Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
> > Reported-by: Alexander Bulekov 
> > Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
> > Reported-by: Muhammad Ramdhan
> > Reported-by: Sergej Schumilo (Ruhr-University Bochum)
> > Reported-by: Simon Wrner (Ruhr-University Bochum)
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
> > Signed-off-by: Bin Meng 
> >
> > ---
> >
> > Changes in v2:
> > - new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a 
> > different block size is programmed
> >
> >  hw/sd/sdhci.c | 12 
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index d0c8e29..5b86781 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -1140,6 +1140,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t 
> > val, unsigned size)
> >  break;
> >  case SDHC_BLKSIZE:
> >  if (!TRANSFERRING_DATA(s->prnsts)) {
> > +uint16_t blksize = s->blksize;
> > +
> >  MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
> >  MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
> >
> > @@ -1151,6 +1153,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t 
> > val, unsigned size)
> >
> >  s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
> >  }
> > +
> > +/*
> > + * If the block size is programmed to a different value from
> > + * the previous one, reset the data pointer of s->fifo_buffer[]
> > + * so that s->fifo_buffer[] can be filled in using the new 
> > block
> > + * size in the next transfer.
> > + */
> > +if (blksize != s->blksize) {
> > +s->data_count = 0;
>
> I doubt the hardware works that way.

Me too, because s->data_count is not exposed by the hardware as a
register or descriptor, so it's purely our internal implementation. A
hardware might implement like that, but we really don't know unless
some hardware guys who designed a SDHC could jump out and comment :)

> Shouldn't we reset the FIFO each time BLKSIZE is accessed, regardless of its 
> previous value?

If we do that, we will end up rewriting the logic of the data transfer
functions. I looked at the current implementation and I think there
are some spec violations about handling page boundaries, and that part
is related to sd->data-count. But like I said in the cover letter
these should be addressed in future patches.

>
> > +}
> >  }
> >
> >  break;
> >

Regards,
Bin

Re: [PATCH v2 6/8] hw/sd: sd: Actually perform the erase operation

[Bin Meng]

On Sat, Feb 20, 2021 at 6:28 AM Philippe Mathieu-Daudé  wrote:
>
> On 2/16/21 4:02 PM, Bin Meng wrote:
> > From: Bin Meng 
> >
> > At present the sd_erase() does not erase the requested range of card
> > data to 0xFFs. Let's make the erase operation actually happen.
> >
> > Signed-off-by: Bin Meng 
> >
> > ---
> >
> > Changes in v2:
> > - honor the write protection bits for SDSC cards
> >
> >  hw/sd/sd.c | 22 ++
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index f1f98bdec3..b386f16fcb 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -766,6 +766,9 @@ static void sd_erase(SDState *sd)
> >  uint64_t erase_start = sd->erase_start;
> >  uint64_t erase_end = sd->erase_end;
> >  bool sdsc = true;
> > +uint64_t wpnum;
> > +uint64_t erase_addr;
> > +int erase_len = 1 << HWBLOCK_SHIFT;
> >
> >  trace_sdcard_erase(sd->erase_start, sd->erase_end);
> >  if (sd->erase_start == INVALID_ADDRESS
> > @@ -794,17 +797,20 @@ static void sd_erase(SDState *sd)
> >  sd->erase_end = INVALID_ADDRESS;
> >  sd->csd[14] |= 0x40;
> >
> > -/* Only SDSC cards support write protect groups */
> > -if (sdsc) {
> > -erase_start = sd_addr_to_wpnum(erase_start);
> > -erase_end = sd_addr_to_wpnum(erase_end);
> > -
> > -for (i = erase_start; i <= erase_end; i++) {
> > -assert(i < sd->wpgrps_size);
> > -if (test_bit(i, sd->wp_groups)) {
> > +memset(sd->data, 0xff, erase_len);
> > +erase_addr = erase_start;
> > +for (i = 0; i <= (erase_end - erase_start) / erase_len; i++) {
> > +if (sdsc) {
> > +/* Only SDSC cards support write protect groups */
> > +wpnum = sd_addr_to_wpnum(erase_addr);
> > +assert(wpnum < sd->wpgrps_size);
> > +if (test_bit(wpnum, sd->wp_groups)) {
> >  sd->card_status |= WP_ERASE_SKIP;
> > +continue;
>
> So if a group is protected, you skip it but don't increase erase_addr.
> If G#4 is protected and G#5 isn't, when you check G#5 you end erasing
> G#4.
>

Oops, good catch!

I will send v2.

> >  }
> >  }
> > +BLK_WRITE_BLOCK(erase_addr, erase_len);
> > +erase_addr += erase_len;
> >  }
> >  }

Regards,
Bin

[PULL 18/18] MAINTAINERS: Add Bin Meng as co-maintainer for SD/MMC cards

[Philippe Mathieu-Daudé]

There is new interest in the SD/MMC device emulation, so it
would be good to have more than only one maintainer / reviewer
for it.

Bin Meng proved by his contributions a deep understanding of the
SD cards internals, so let's add him to the corresponding section
in the MAINTAINERS file.

Signed-off-by: Philippe Mathieu-Daudé 
Acked-by: Bin Meng 
Message-Id: <20210216132841.1121653-1-f4...@amsat.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 66354e6e495..5eeba79c5a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1754,6 +1754,7 @@ F: hw/ssi/xilinx_*
 
 SD (Secure Card)
 M: Philippe Mathieu-Daudé 
+M: Bin Meng 
 L: qemu-block@nongnu.org
 S: Odd Fixes
 F: include/hw/sd/sd*
-- 
2.26.2

[PULL 13/18] hw/sd: sd: Move the sd_block_{read, write} and macros ahead

[Philippe Mathieu-Daudé]

From: Bin Meng 

These APIs and macros may be referenced by functions that are
currently before them. Move them ahead a little bit.

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210216150225.27996-5-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 42 +-
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 47ac0c51a8e..4c6e7c2a33e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -739,6 +739,27 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq 
insert)
 qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
 }
 
+static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
+{
+trace_sdcard_read_block(addr, len);
+if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
+fprintf(stderr, "sd_blk_read: read error on host side\n");
+}
+}
+
+static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
+{
+trace_sdcard_write_block(addr, len);
+if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
+fprintf(stderr, "sd_blk_write: write error on host side\n");
+}
+}
+
+#define BLK_READ_BLOCK(a, len)  sd_blk_read(sd, a, len)
+#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
+#define APP_READ_BLOCK(a, len)  memset(sd->data, 0xec, len)
+#define APP_WRITE_BLOCK(a, len)
+
 static void sd_erase(SDState *sd)
 {
 int i;
@@ -1754,27 +1775,6 @@ send_response:
 return rsplen;
 }
 
-static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
-{
-trace_sdcard_read_block(addr, len);
-if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
-fprintf(stderr, "sd_blk_read: read error on host side\n");
-}
-}
-
-static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
-{
-trace_sdcard_write_block(addr, len);
-if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
-fprintf(stderr, "sd_blk_write: write error on host side\n");
-}
-}
-
-#define BLK_READ_BLOCK(a, len) sd_blk_read(sd, a, len)
-#define BLK_WRITE_BLOCK(a, len)sd_blk_write(sd, a, len)
-#define APP_READ_BLOCK(a, len) memset(sd->data, 0xec, len)
-#define APP_WRITE_BLOCK(a, len)
-
 void sd_write_byte(SDState *sd, uint8_t value)
 {
 int i;
-- 
2.26.2

[PULL 14/18] hw/sd: sd: Skip write protect groups check in sd_erase() for high capacity cards

[Philippe Mathieu-Daudé]

From: Bin Meng 

High capacity cards don't support write protection hence we should
not perform the write protect groups check in sd_erase() for them.

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210216150225.27996-6-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4c6e7c2a33e..883c04de028 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -765,6 +765,7 @@ static void sd_erase(SDState *sd)
 int i;
 uint64_t erase_start = sd->erase_start;
 uint64_t erase_end = sd->erase_end;
+bool sdsc = true;
 
 trace_sdcard_erase(sd->erase_start, sd->erase_end);
 if (sd->erase_start == INVALID_ADDRESS
@@ -779,6 +780,7 @@ static void sd_erase(SDState *sd)
 /* High capacity memory card: erase units are 512 byte blocks */
 erase_start *= 512;
 erase_end *= 512;
+sdsc = false;
 }
 
 if (erase_start > sd->size || erase_end > sd->size) {
@@ -788,16 +790,20 @@ static void sd_erase(SDState *sd)
 return;
 }
 
-erase_start = sd_addr_to_wpnum(erase_start);
-erase_end = sd_addr_to_wpnum(erase_end);
 sd->erase_start = INVALID_ADDRESS;
 sd->erase_end = INVALID_ADDRESS;
 sd->csd[14] |= 0x40;
 
-for (i = erase_start; i <= erase_end; i++) {
-assert(i < sd->wpgrps_size);
-if (test_bit(i, sd->wp_groups)) {
-sd->card_status |= WP_ERASE_SKIP;
+/* Only SDSC cards support write protect groups */
+if (sdsc) {
+erase_start = sd_addr_to_wpnum(erase_start);
+erase_end = sd_addr_to_wpnum(erase_end);
+
+for (i = erase_start; i <= erase_end; i++) {
+assert(i < sd->wpgrps_size);
+if (test_bit(i, sd->wp_groups)) {
+sd->card_status |= WP_ERASE_SKIP;
+}
 }
 }
 }
-- 
2.26.2

[PULL 16/18] hw/sd: sd: Bypass the RCA check for CMD13 in SPI mode

[Philippe Mathieu-Daudé]

From: Bin Meng 

Unlike SD mode, when SD card is working in SPI mode, the argument
of CMD13 is stuff bits. Hence we should bypass the RCA check.

See "Physical Layer Specification Version 8.00", chapter 7.3.1.3
Detailed Command Description (SPI mode):

  "The card shall ignore stuff bits and reserved bits in an argument"

and Table 7-3 Commands and Arguments (SPI mode):

  "CMD13 Argument [31:0] stuff bits"

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210216150225.27996-9-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 3a515a5365f..8b397effbcc 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1163,8 +1163,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 case 13:   /* CMD13:  SEND_STATUS */
 switch (sd->mode) {
 case sd_data_transfer_mode:
-if (sd->rca != rca)
+if (!sd->spi && sd->rca != rca) {
 return sd_r0;
+}
 
 return sd_r1;
 
-- 
2.26.2

[PULL 17/18] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()

[Philippe Mathieu-Daudé]

From: Bin Meng 

s->prnsts is updated in both branches of the if () else () statement.
Move the common bits outside so that it is cleaner.

Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 
Reviewed-by: Alexander Bulekov 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <1613447214-81951-5-git-send-email-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sdhci.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8ffa53999d8..9acf4467a32 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -596,9 +596,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 page_aligned = true;
 }
 
+s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
 if (s->trnmod & SDHC_TRNS_READ) {
-s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
-SDHC_DAT_LINE_ACTIVE;
+s->prnsts |= SDHC_DOING_READ;
 while (s->blkcnt) {
 if (s->data_count == 0) {
 sdbus_read_data(>sdbus, s->fifo_buffer, block_size);
@@ -625,8 +625,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 }
 }
 } else {
-s->prnsts |= SDHC_DOING_WRITE | SDHC_DATA_INHIBIT |
-SDHC_DAT_LINE_ACTIVE;
+s->prnsts |= SDHC_DOING_WRITE;
 while (s->blkcnt) {
 begin = s->data_count;
 if (((boundary_count + begin) < block_size) && page_aligned) {
-- 
2.26.2

[PULL 11/18] hw/sd: sd: Only SDSC cards support CMD28/29/30

[Philippe Mathieu-Daudé]

From: Bin Meng 

Per the "Physical Layer Specification Version 8.00", table 4-26
(SD mode) and table 7-3 (SPI mode) command descriptions, the
following commands:

- CMD28 (SET_WRITE_PROT)
- CMD29 (CLR_WRITE_PROT)
- CMD30 (SEND_WRITE_PROT)

are only supported by SDSC cards.

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210216150225.27996-3-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 7adcb4edfaa..dd1ce0bdae4 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1284,6 +1284,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 
 /* Write protection (Class 6) */
 case 28:   /* CMD28:  SET_WRITE_PROT */
+if (sd->size > SDSC_MAX_CAPACITY) {
+return sd_illegal;
+}
+
 switch (sd->state) {
 case sd_transfer_state:
 if (addr >= sd->size) {
@@ -1303,6 +1307,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 break;
 
 case 29:   /* CMD29:  CLR_WRITE_PROT */
+if (sd->size > SDSC_MAX_CAPACITY) {
+return sd_illegal;
+}
+
 switch (sd->state) {
 case sd_transfer_state:
 if (addr >= sd->size) {
@@ -1322,6 +1330,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 break;
 
 case 30:   /* CMD30:  SEND_WRITE_PROT */
+if (sd->size > SDSC_MAX_CAPACITY) {
+return sd_illegal;
+}
+
 switch (sd->state) {
 case sd_transfer_state:
 sd->state = sd_sendingdata_state;
-- 
2.26.2

[PULL 12/18] hw/sd: sd: Fix CMD30 response type

[Philippe Mathieu-Daudé]

From: Bin Meng 

Per the "Physical Layer Specification Version 8.00", table 4-26
(SD mode) and table 7-3 (SPI mode) command descriptions, CMD30
response type is R1, not R1b.

Fixes: a1bb27b1e98a ("SD card emulation initial implementation")
Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210216150225.27996-4-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index dd1ce0bdae4..47ac0c51a8e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1340,7 +1340,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 *(uint32_t *) sd->data = sd_wpbits(sd, req.arg);
 sd->data_start = addr;
 sd->data_offset = 0;
-return sd_r1b;
+return sd_r1;
 
 default:
 break;
-- 
2.26.2

[PULL 10/18] hw/sd: sd: Fix address check in sd_erase()

[Philippe Mathieu-Daudé]

From: Bin Meng 

For high capacity memory cards, the erase start address and end
address are multiplied by 512, but the address check is still
based on the original block number in sd->erase_{start, end}.

Fixes: 1bd6fd8ed593 ("hw/sd/sdcard: Do not attempt to erase out of range 
addresses")
Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210216150225.27996-2-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 172e83f99d9..7adcb4edfaa 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -760,7 +760,7 @@ static void sd_erase(SDState *sd)
 erase_end *= 512;
 }
 
-if (sd->erase_start > sd->size || sd->erase_end > sd->size) {
+if (erase_start > sd->size || erase_end > sd->size) {
 sd->card_status |= OUT_OF_RANGE;
 sd->erase_start = INVALID_ADDRESS;
 sd->erase_end = INVALID_ADDRESS;
-- 
2.26.2

[PULL 09/18] hw/sd: ssi-sd: Handle the rest commands with R1b response type

[Philippe Mathieu-Daudé]

From: Bin Meng 

Besides CMD12, the following command's reponse type is R1b:

- SET_WRITE_PROT (CMD28)
- CLR_WRITE_PROT (CMD29)
- ERASE (CMD38)

Reuse the same s->stopping to indicate a R1b reponse is needed.

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210128063035.15674-10-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/ssi-sd.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 907d681d19e..97ee58e20cf 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -194,6 +194,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, 
uint32_t val)
 /* CMD13 returns a 2-byte statuse work. Other commands
only return the first byte.  */
 s->arglen = (s->cmd == 13) ? 2 : 1;
+
+/* handle R1b */
+if (s->cmd == 28 || s->cmd == 29 || s->cmd == 38) {
+s->stopping = 1;
+}
+
 cardstatus = ldl_be_p(longresp);
 status = 0;
 if (((cardstatus >> 9) & 0xf) < 4)
-- 
2.26.2

[PULL 05/18] hw/sd: ssi-sd: Support single block write

[Philippe Mathieu-Daudé]

From: Bin Meng 

Add 2 more states for the block write operation. The SPI host needs
to send a data start token to start the transfer, and the data block
written to the card will be acknowledged by a data response token.

Signed-off-by: Bin Meng 
[PMD: Change VMState version id 6 -> 7]
Signed-off-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Acked-by: Alistair Francis 
Message-Id: <20210128063035.15674-6-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/ssi-sd.c | 44 
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 6d20a240c69..1205ad8b52c 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -43,6 +43,8 @@ typedef enum {
 SSI_SD_DATA_START,
 SSI_SD_DATA_READ,
 SSI_SD_DATA_CRC16,
+SSI_SD_DATA_WRITE,
+SSI_SD_SKIP_CRC16,
 } ssi_sd_mode;
 
 struct ssi_sd_state {
@@ -53,6 +55,7 @@ struct ssi_sd_state {
 uint8_t response[5];
 uint16_t crc16;
 int32_t read_bytes;
+int32_t write_bytes;
 int32_t arglen;
 int32_t response_pos;
 int32_t stopping;
@@ -85,6 +88,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(ssi_sd_state, SSI_SD)
 /* dummy value - don't care */
 #define SSI_DUMMY   0xff
 
+/* data accepted */
+#define DATA_RESPONSE_ACCEPTED  0x05
+
 static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
 {
 ssi_sd_state *s = SSI_SD(dev);
@@ -113,10 +119,17 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, 
uint32_t val)
 
 switch (s->mode) {
 case SSI_SD_CMD:
-if (val == SSI_DUMMY) {
+switch (val) {
+case SSI_DUMMY:
 DPRINTF("NULL command\n");
 return SSI_DUMMY;
+break;
+case SSI_TOKEN_SINGLE:
+DPRINTF("Start write block\n");
+s->mode = SSI_SD_DATA_WRITE;
+return SSI_DUMMY;
 }
+
 s->cmd = val & 0x3f;
 s->mode = SSI_SD_CMDARG;
 s->arglen = 0;
@@ -250,6 +263,27 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, 
uint32_t val)
 s->response_pos = 0;
 }
 return val;
+case SSI_SD_DATA_WRITE:
+sdbus_write_byte(>sdbus, val);
+s->write_bytes++;
+if (!sdbus_receive_ready(>sdbus) || s->write_bytes == 512) {
+DPRINTF("Data write end\n");
+s->mode = SSI_SD_SKIP_CRC16;
+s->response_pos = 0;
+}
+return val;
+case SSI_SD_SKIP_CRC16:
+/* we don't verify the crc16 */
+s->response_pos++;
+if (s->response_pos == 2) {
+DPRINTF("CRC16 receive end\n");
+s->mode = SSI_SD_RESPONSE;
+s->write_bytes = 0;
+s->arglen = 1;
+s->response[0] = DATA_RESPONSE_ACCEPTED;
+s->response_pos = 0;
+}
+return SSI_DUMMY;
 }
 /* Should never happen.  */
 return SSI_DUMMY;
@@ -259,7 +293,7 @@ static int ssi_sd_post_load(void *opaque, int version_id)
 {
 ssi_sd_state *s = (ssi_sd_state *)opaque;
 
-if (s->mode > SSI_SD_DATA_CRC16) {
+if (s->mode > SSI_SD_SKIP_CRC16) {
 return -EINVAL;
 }
 if (s->mode == SSI_SD_CMDARG &&
@@ -277,8 +311,8 @@ static int ssi_sd_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_ssi_sd = {
 .name = "ssi_sd",
-.version_id = 6,
-.minimum_version_id = 6,
+.version_id = 7,
+.minimum_version_id = 7,
 .post_load = ssi_sd_post_load,
 .fields = (VMStateField []) {
 VMSTATE_UINT32(mode, ssi_sd_state),
@@ -287,6 +321,7 @@ static const VMStateDescription vmstate_ssi_sd = {
 VMSTATE_UINT8_ARRAY(response, ssi_sd_state, 5),
 VMSTATE_UINT16(crc16, ssi_sd_state),
 VMSTATE_INT32(read_bytes, ssi_sd_state),
+VMSTATE_INT32(write_bytes, ssi_sd_state),
 VMSTATE_INT32(arglen, ssi_sd_state),
 VMSTATE_INT32(response_pos, ssi_sd_state),
 VMSTATE_INT32(stopping, ssi_sd_state),
@@ -340,6 +375,7 @@ static void ssi_sd_reset(DeviceState *dev)
 memset(s->response, 0, sizeof(s->response));
 s->crc16 = 0;
 s->read_bytes = 0;
+s->write_bytes = 0;
 s->arglen = 0;
 s->response_pos = 0;
 s->stopping = 0;
-- 
2.26.2

[PULL 15/18] hw/sd: sd: Skip write protect groups check in CMD24/25 for high capacity cards

[Philippe Mathieu-Daudé]

From: Bin Meng 

High capacity cards don't support write protection hence we should
not perform the write protect groups check in CMD24/25 for them.

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210216150225.27996-8-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 883c04de028..3a515a5365f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1268,8 +1268,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 sd->data_offset = 0;
 sd->blk_written = 0;
 
-if (sd_wp_addr(sd, sd->data_start)) {
-sd->card_status |= WP_VIOLATION;
+if (sd->size <= SDSC_MAX_CAPACITY) {
+if (sd_wp_addr(sd, sd->data_start)) {
+sd->card_status |= WP_VIOLATION;
+}
 }
 if (sd->csd[14] & 0x30) {
 sd->card_status |= WP_VIOLATION;
@@ -1821,9 +1823,11 @@ void sd_write_byte(SDState *sd, uint8_t value)
 sd->card_status |= ADDRESS_ERROR;
 break;
 }
-if (sd_wp_addr(sd, sd->data_start)) {
-sd->card_status |= WP_VIOLATION;
-break;
+if (sd->size <= SDSC_MAX_CAPACITY) {
+if (sd_wp_addr(sd, sd->data_start)) {
+sd->card_status |= WP_VIOLATION;
+break;
+}
 }
 }
 sd->data[sd->data_offset++] = value;
-- 
2.26.2

[PULL 07/18] hw/sd: ssi-sd: Fix SEND_IF_COND (CMD8) response

[Philippe Mathieu-Daudé]

From: Bin Meng 

The SEND_IF_COND command (CMD8) response is of format R7, but
current code returns R1 for CMD8. Fix it.

Fixes: 775616c3ae8c ("Partial SD card SPI mode support")
Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210128063035.15674-8-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/ssi-sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 200e885225a..84c873b3fd4 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -176,9 +176,9 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, 
uint32_t val)
 s->arglen = 1;
 s->response[0] = 4;
 DPRINTF("SD command failed\n");
-} else if (s->cmd == 58) {
-/* CMD58 returns R3 response (OCR)  */
-DPRINTF("Returned OCR\n");
+} else if (s->cmd == 8 || s->cmd == 58) {
+/* CMD8/CMD58 returns R3/R7 response */
+DPRINTF("Returned R3/R7\n");
 s->arglen = 5;
 s->response[0] = 1;
 memcpy(>response[1], longresp, 4);
-- 
2.26.2

[PULL 08/18] hw/sd: ssi-sd: Fix STOP_TRANSMISSION (CMD12) response

[Philippe Mathieu-Daudé]

From: Bin Meng 

CMD12's response type is R1b, which is basically a R1 plus optional
addition of the busy signal token that can be any number of bytes.
A zero value indicates card is busy and a non-zero value indicates
the card is ready for the next command.

Current implementation sends the busy signal token without sending
the R1 first. This does not break the U-Boot/Linux mmc_spi driver,
but it does not make the VxWorks driver happy.

Move the testing logic of s->stopping in the SSI_SD_RESPONSE state
a bit later, after the first byte of the card reponse is sent out,
to conform with the spec. After the busy signal token is sent, the
state should be transferred to SSI_SD_CMD.

Fixes: 775616c3ae8c ("Partial SD card SPI mode support")
Signed-off-by: Bin Meng 
Message-Id: <20210128063035.15674-9-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/ssi-sd.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 84c873b3fd4..907d681d19e 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -243,14 +243,15 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, 
uint32_t val)
 s->mode = SSI_SD_RESPONSE;
 return SSI_DUMMY;
 case SSI_SD_RESPONSE:
-if (s->stopping) {
-s->stopping = 0;
-return SSI_DUMMY;
-}
 if (s->response_pos < s->arglen) {
 DPRINTF("Response 0x%02x\n", s->response[s->response_pos]);
 return s->response[s->response_pos++];
 }
+if (s->stopping) {
+s->stopping = 0;
+s->mode = SSI_SD_CMD;
+return SSI_DUMMY;
+}
 if (sdbus_data_ready(>sdbus)) {
 DPRINTF("Data read\n");
 s->mode = SSI_SD_DATA_START;
-- 
2.26.2

[PULL 03/18] hw/sd: sd: Allow single/multiple block write for SPI mode

[Philippe Mathieu-Daudé]

From: Bin Meng 

At present the single/multiple block write in SPI mode is blocked
by sd_normal_command(). Remove the limitation.

Signed-off-by: Bin Meng 
Tested-by: Philippe Mathieu-Daudé 
Acked-by: Alistair Francis 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20210128063035.15674-4-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a85a821abbe..5de9e0a6c20 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1230,9 +1230,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 case 25:   /* CMD25:  WRITE_MULTIPLE_BLOCK */
 switch (sd->state) {
 case sd_transfer_state:
-/* Writing in SPI mode not implemented.  */
-if (sd->spi)
-break;
 
 if (addr + sd->blk_len > sd->size) {
 sd->card_status |= ADDRESS_ERROR;
-- 
2.26.2

[PULL 04/18] hw/sd: Introduce receive_ready() callback

[Philippe Mathieu-Daudé]

From: Bin Meng 

At present there is a data_ready() callback for the SD data read
path. Let's add a receive_ready() for the SD data write path.

Signed-off-by: Bin Meng 
Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Philippe Mathieu-Daudé 
Acked-by: Alistair Francis 
Message-Id: <20210128063035.15674-5-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/sd/sd.h |  2 ++
 hw/sd/core.c   | 13 +
 hw/sd/sd.c |  6 ++
 3 files changed, 21 insertions(+)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 05ef9b73e56..47360ba4ee9 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -116,6 +116,7 @@ struct SDCardClass {
  * Return: byte value read
  */
 uint8_t (*read_byte)(SDState *sd);
+bool (*receive_ready)(SDState *sd);
 bool (*data_ready)(SDState *sd);
 void (*set_voltage)(SDState *sd, uint16_t millivolts);
 uint8_t (*get_dat_lines)(SDState *sd);
@@ -187,6 +188,7 @@ void sdbus_write_data(SDBus *sdbus, const void *buf, size_t 
length);
  * Read multiple bytes of data on the data lines of a SD bus.
  */
 void sdbus_read_data(SDBus *sdbus, void *buf, size_t length);
+bool sdbus_receive_ready(SDBus *sd);
 bool sdbus_data_ready(SDBus *sd);
 bool sdbus_get_inserted(SDBus *sd);
 bool sdbus_get_readonly(SDBus *sd);
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 08c93b59034..30ee62c5106 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -160,6 +160,19 @@ void sdbus_read_data(SDBus *sdbus, void *buf, size_t 
length)
 }
 }
 
+bool sdbus_receive_ready(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDCardClass *sc = SD_CARD_GET_CLASS(card);
+
+return sc->receive_ready(card);
+}
+
+return false;
+}
+
 bool sdbus_data_ready(SDBus *sdbus)
 {
 SDState *card = get_card(sdbus);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5de9e0a6c20..172e83f99d9 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2037,6 +2037,11 @@ uint8_t sd_read_byte(SDState *sd)
 return ret;
 }
 
+static bool sd_receive_ready(SDState *sd)
+{
+return sd->state == sd_receivingdata_state;
+}
+
 static bool sd_data_ready(SDState *sd)
 {
 return sd->state == sd_sendingdata_state;
@@ -2147,6 +2152,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
 sc->do_command = sd_do_command;
 sc->write_byte = sd_write_byte;
 sc->read_byte = sd_read_byte;
+sc->receive_ready = sd_receive_ready;
 sc->data_ready = sd_data_ready;
 sc->enable = sd_enable;
 sc->get_inserted = sd_get_inserted;
-- 
2.26.2

[PULL 02/18] hw/sd: sd: Remove duplicated codes in single/multiple block read/write

[Philippe Mathieu-Daudé]

From: Bin Meng 

The single block read (CMD17) codes are the same as the multiple
block read (CMD18). Merge them into one. The same applies to single
block write (CMD24) and multiple block write (CMD25).

Signed-off-by: Bin Meng 
Tested-by: Philippe Mathieu-Daudé 
Acked-by: Alistair Francis 
Message-Id: <20210128063035.15674-3-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/sd.c | 47 ---
 1 file changed, 47 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 8517dbce8ba..a85a821abbe 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1181,24 +1181,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 break;
 
 case 17:   /* CMD17:  READ_SINGLE_BLOCK */
-switch (sd->state) {
-case sd_transfer_state:
-
-if (addr + sd->blk_len > sd->size) {
-sd->card_status |= ADDRESS_ERROR;
-return sd_r1;
-}
-
-sd->state = sd_sendingdata_state;
-sd->data_start = addr;
-sd->data_offset = 0;
-return sd_r1;
-
-default:
-break;
-}
-break;
-
 case 18:   /* CMD18:  READ_MULTIPLE_BLOCK */
 switch (sd->state) {
 case sd_transfer_state:
@@ -1245,35 +1227,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 
 /* Block write commands (Class 4) */
 case 24:   /* CMD24:  WRITE_SINGLE_BLOCK */
-switch (sd->state) {
-case sd_transfer_state:
-/* Writing in SPI mode not implemented.  */
-if (sd->spi)
-break;
-
-if (addr + sd->blk_len > sd->size) {
-sd->card_status |= ADDRESS_ERROR;
-return sd_r1;
-}
-
-sd->state = sd_receivingdata_state;
-sd->data_start = addr;
-sd->data_offset = 0;
-sd->blk_written = 0;
-
-if (sd_wp_addr(sd, sd->data_start)) {
-sd->card_status |= WP_VIOLATION;
-}
-if (sd->csd[14] & 0x30) {
-sd->card_status |= WP_VIOLATION;
-}
-return sd_r1;
-
-default:
-break;
-}
-break;
-
 case 25:   /* CMD25:  WRITE_MULTIPLE_BLOCK */
 switch (sd->state) {
 case sd_transfer_state:
-- 
2.26.2

[PULL 01/18] hw/sd: ssi-sd: Support multiple block read

[Philippe Mathieu-Daudé]

From: Bin Meng 

In the case of a multiple block read operation every transferred
block has its suffix of CRC16. Update the state machine logic to
handle multiple block read.

Signed-off-by: Bin Meng 
[PMD: Change VMState version id 5 -> 6]
Signed-off-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Acked-by: Alistair Francis 
Message-Id: <20210128063035.15674-2-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/ssi-sd.c | 42 +-
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index be1bb101645..6d20a240c69 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -52,6 +52,7 @@ struct ssi_sd_state {
 uint8_t cmdarg[4];
 uint8_t response[5];
 uint16_t crc16;
+int32_t read_bytes;
 int32_t arglen;
 int32_t response_pos;
 int32_t stopping;
@@ -88,11 +89,26 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, 
uint32_t val)
 {
 ssi_sd_state *s = SSI_SD(dev);
 
-/* Special case: allow CMD12 (STOP TRANSMISSION) while reading data.  */
-if (s->mode == SSI_SD_DATA_READ && val == 0x4c) {
-s->mode = SSI_SD_CMD;
-/* There must be at least one byte delay before the card responds.  */
-s->stopping = 1;
+/*
+ * Special case: allow CMD12 (STOP TRANSMISSION) while reading data.
+ *
+ * See "Physical Layer Specification Version 8.00" chapter 7.5.2.2,
+ * to avoid conflict between CMD12 response and next data block,
+ * timing of CMD12 should be controlled as follows:
+ *
+ * - CMD12 issued at the timing that end bit of CMD12 and end bit of
+ *   data block is overlapped
+ * - CMD12 issued after one clock cycle after host receives a token
+ *   (either Start Block token or Data Error token)
+ *
+ * We need to catch CMD12 in all of the data read states.
+ */
+if (s->mode >= SSI_SD_PREP_DATA && s->mode <= SSI_SD_DATA_CRC16) {
+if (val == 0x4c) {
+s->mode = SSI_SD_CMD;
+/* There must be at least one byte delay before the card responds 
*/
+s->stopping = 1;
+}
 }
 
 switch (s->mode) {
@@ -212,8 +228,9 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, 
uint32_t val)
 return SSI_TOKEN_SINGLE;
 case SSI_SD_DATA_READ:
 val = sdbus_read_byte(>sdbus);
+s->read_bytes++;
 s->crc16 = crc_ccitt_false(s->crc16, (uint8_t *), 1);
-if (!sdbus_data_ready(>sdbus)) {
+if (!sdbus_data_ready(>sdbus) || s->read_bytes == 512) {
 DPRINTF("Data read end\n");
 s->mode = SSI_SD_DATA_CRC16;
 }
@@ -224,7 +241,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, 
uint32_t val)
 s->response_pos++;
 if (s->response_pos == 2) {
 DPRINTF("CRC16 read end\n");
-s->mode = SSI_SD_CMD;
+if (s->read_bytes == 512 && s->cmd != 17) {
+s->mode = SSI_SD_PREP_DATA;
+} else {
+s->mode = SSI_SD_CMD;
+}
+s->read_bytes = 0;
 s->response_pos = 0;
 }
 return val;
@@ -255,8 +277,8 @@ static int ssi_sd_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_ssi_sd = {
 .name = "ssi_sd",
-.version_id = 5,
-.minimum_version_id = 5,
+.version_id = 6,
+.minimum_version_id = 6,
 .post_load = ssi_sd_post_load,
 .fields = (VMStateField []) {
 VMSTATE_UINT32(mode, ssi_sd_state),
@@ -264,6 +286,7 @@ static const VMStateDescription vmstate_ssi_sd = {
 VMSTATE_UINT8_ARRAY(cmdarg, ssi_sd_state, 4),
 VMSTATE_UINT8_ARRAY(response, ssi_sd_state, 5),
 VMSTATE_UINT16(crc16, ssi_sd_state),
+VMSTATE_INT32(read_bytes, ssi_sd_state),
 VMSTATE_INT32(arglen, ssi_sd_state),
 VMSTATE_INT32(response_pos, ssi_sd_state),
 VMSTATE_INT32(stopping, ssi_sd_state),
@@ -316,6 +339,7 @@ static void ssi_sd_reset(DeviceState *dev)
 memset(s->cmdarg, 0, sizeof(s->cmdarg));
 memset(s->response, 0, sizeof(s->response));
 s->crc16 = 0;
+s->read_bytes = 0;
 s->arglen = 0;
 s->response_pos = 0;
 s->stopping = 0;
-- 
2.26.2

[PULL 00/18] SD/MMC patches for 2021-02-20

[Philippe Mathieu-Daudé]

The following changes since commit e90ef02389dc8b57eaea22b290244609d720a8bf:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2021-02-18' into 
staging (2021-02-19 17:22:42 +)

are available in the Git repository at:

  https://gitlab.com/philmd/qemu.git tags/sdmmc-20210220

for you to fetch changes up to 3e0a7693be30d6a6eda8a56f3862ac2e502a9e81:

  MAINTAINERS: Add Bin Meng as co-maintainer for SD/MMC cards (2021-02-20 
01:08:59 +0100)


SD/MMC patches

- Various improvements for SD cards in SPI mode (Bin Meng)
- Add Bin Meng as SD/MMC cards co-maintainer


Bin Meng (17):
  hw/sd: ssi-sd: Support multiple block read
  hw/sd: sd: Remove duplicated codes in single/multiple block read/write
  hw/sd: sd: Allow single/multiple block write for SPI mode
  hw/sd: Introduce receive_ready() callback
  hw/sd: ssi-sd: Support single block write
  hw/sd: ssi-sd: Support multiple block write
  hw/sd: ssi-sd: Fix SEND_IF_COND (CMD8) response
  hw/sd: ssi-sd: Fix STOP_TRANSMISSION (CMD12) response
  hw/sd: ssi-sd: Handle the rest commands with R1b response type
  hw/sd: sd: Fix address check in sd_erase()
  hw/sd: sd: Only SDSC cards support CMD28/29/30
  hw/sd: sd: Fix CMD30 response type
  hw/sd: sd: Move the sd_block_{read, write} and macros ahead
  hw/sd: sd: Skip write protect groups check in sd_erase() for high
capacity cards
  hw/sd: sd: Skip write protect groups check in CMD24/25 for high
capacity cards
  hw/sd: sd: Bypass the RCA check for CMD13 in SPI mode
  hw/sd: sdhci: Simplify updating s->prnsts in
sdhci_sdma_transfer_multi_blocks()

Philippe Mathieu-Daudé (1):
  MAINTAINERS: Add Bin Meng as co-maintainer for SD/MMC cards

 include/hw/sd/sd.h |   2 +
 hw/sd/core.c   |  13 
 hw/sd/sd.c | 149 +++--
 hw/sd/sdhci.c  |   7 +--
 hw/sd/ssi-sd.c | 136 +++--
 MAINTAINERS|   1 +
 6 files changed, 199 insertions(+), 109 deletions(-)

-- 
2.26.2

Re: [PATCH] MAINTAINERS: Add Bin Meng as co-maintainer for SD/MMC cards

[Philippe Mathieu-Daudé]

On 2/16/21 2:28 PM, Philippe Mathieu-Daudé wrote:
> There is new interest in the SD/MMC device emulation, so it
> would be good to have more than only one maintainer / reviewer
> for it.
> 
> Bin Meng proved by his contributions a deep understanding of the
> SD cards internals, so let's add him to the corresponding section
> in the MAINTAINERS file.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)

Thanks, applied to sdmmc-next tree.

Re: [PATCH v2 4/6] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()

[Philippe Mathieu-Daudé]

On 2/16/21 4:46 AM, Bin Meng wrote:
> s->prnsts is updated in both branches of the if () else () statement.
> Move the common bits outside so that it is cleaner.
> 
> Signed-off-by: Bin Meng 
> ---
> 
> (no changes since v1)
> 
>  hw/sd/sdhci.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)

As there are some questions in this series and it makes sense to
merge all patches at once to help downstream distributions track
the CVE fixes, I'm queuing this single patch to sdmmc-next tree.

Thanks,

Phil.

Re: [PATCH v2 00/12] block/export: vhost-user-blk server tests and input validation

[Peter Maydell]

On Mon, 15 Feb 2021 at 10:41, Kevin Wolf  wrote:
>
> Am 07.12.2020 um 18:20 hat Stefan Hajnoczi geschrieben:
> > v2:
> >  * Add abrt handler that terminates qemu-storage-daemon to
> >vhost-user-blk-test. No more orphaned processes on test failure. [Peter]
> >  * Fix sector number calculation in vhost-user-blk-server.c
> >  * Introduce VIRTIO_BLK_SECTOR_BITS/SIZE to make code clearer [Max]
> >  * Fix vhost-user-blk-server.c blk_size double byteswap
> >  * Fix vhost-user-blk blkcfg->num_queues endianness [Peter]
> >  * Squashed cleanups into Coiby vhost-user-blk-test commit so the code is
> >easier to review
> >
> > The vhost-user-blk server test was already in Michael Tsirkin's recent vhost
> > pull request, but was dropped because it exposed vhost-user regressions
> > (b7c1bd9d7848 and the Based-on tag below). Now that the vhost-user 
> > regressions
> > are fixed we can re-introduce the test case.
> >
> > This series adds missing input validation that led to a Coverity report. The
> > virtio-blk read, write, discard, and write zeroes commands need to check
> > sector/byte ranges and other inputs. This solves the issue Peter Maydell 
> > raised
> > in "[PATCH for-5.2] block/export/vhost-user-blk-server.c: Avoid potential
> > integer overflow".
> >
> > Merging just the input validation patches would be possible too, but I 
> > prefer
> > to merge the corresponding tests so the code is exercised by the CI.
>
> Is this series still open? I don't see it in master.

The Coverity issue is still unfixed, at any rate...

-- PMM

Re: [PATCH v2 0/8] hw/sd: sd: Erase operation and other fixes

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng 
> 
> This includes several fixes related to erase operation of a SD card.

> Bin Meng (8):
>   hw/sd: sd: Fix address check in sd_erase()
>   hw/sd: sd: Only SDSC cards support CMD28/29/30
>   hw/sd: sd: Fix CMD30 response type
>   hw/sd: sd: Move the sd_block_{read,write} and macros ahead
>   hw/sd: sd: Skip write protect groups check in sd_erase() for high
> capacity cards
>   hw/sd: sd: Actually perform the erase operation
>   hw/sd: sd: Skip write protect groups check in CMD24/25 for high
> capacity cards
>   hw/sd: sd: Bypass the RCA check for CMD13 in SPI mode
> 
>  hw/sd/sd.c | 99 +++---
>  1 file changed, 64 insertions(+), 35 deletions(-)

Thanks, patches 1-5 and 7-8 applied to sdmmc-next tree.

Re: [PATCH v2 5/8] hw/sd: sd: Skip write protect groups check in sd_erase() for high capacity cards

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng 
> 
> High capacity cards don't support write protection hence we should
> not preform the write protect groups check in sd_erase() for them.
> 
> Signed-off-by: Bin Meng 
> 
> ---
> 
> Changes in v2:
> - new patch: sd: Skip write protect groups check in sd_erase() for high 
> capacity card
> 
>  hw/sd/sd.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 6/8] hw/sd: sd: Actually perform the erase operation

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng 
> 
> At present the sd_erase() does not erase the requested range of card
> data to 0xFFs. Let's make the erase operation actually happen.
> 
> Signed-off-by: Bin Meng 
> 
> ---
> 
> Changes in v2:
> - honor the write protection bits for SDSC cards
> 
>  hw/sd/sd.c | 22 ++
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index f1f98bdec3..b386f16fcb 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -766,6 +766,9 @@ static void sd_erase(SDState *sd)
>  uint64_t erase_start = sd->erase_start;
>  uint64_t erase_end = sd->erase_end;
>  bool sdsc = true;
> +uint64_t wpnum;
> +uint64_t erase_addr;
> +int erase_len = 1 << HWBLOCK_SHIFT;
>  
>  trace_sdcard_erase(sd->erase_start, sd->erase_end);
>  if (sd->erase_start == INVALID_ADDRESS
> @@ -794,17 +797,20 @@ static void sd_erase(SDState *sd)
>  sd->erase_end = INVALID_ADDRESS;
>  sd->csd[14] |= 0x40;
>  
> -/* Only SDSC cards support write protect groups */
> -if (sdsc) {
> -erase_start = sd_addr_to_wpnum(erase_start);
> -erase_end = sd_addr_to_wpnum(erase_end);
> -
> -for (i = erase_start; i <= erase_end; i++) {
> -assert(i < sd->wpgrps_size);
> -if (test_bit(i, sd->wp_groups)) {
> +memset(sd->data, 0xff, erase_len);
> +erase_addr = erase_start;
> +for (i = 0; i <= (erase_end - erase_start) / erase_len; i++) {
> +if (sdsc) {
> +/* Only SDSC cards support write protect groups */
> +wpnum = sd_addr_to_wpnum(erase_addr);
> +assert(wpnum < sd->wpgrps_size);
> +if (test_bit(wpnum, sd->wp_groups)) {
>  sd->card_status |= WP_ERASE_SKIP;
> +continue;

So if a group is protected, you skip it but don't increase erase_addr.
If G#4 is protected and G#5 isn't, when you check G#5 you end erasing
G#4.

>  }
>  }
> +BLK_WRITE_BLOCK(erase_addr, erase_len);
> +erase_addr += erase_len;
>  }
>  }
>  
> 

Re: [PATCH v2 7/8] hw/sd: sd: Skip write protect groups check in CMD24/25 for high capacity cards

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng 
> 
> High capacity cards don't support write protection hence we should
> not preform the write protect groups check in CMD24/25 for them.
> 
> Signed-off-by: Bin Meng 
> 
> ---
> 
> Changes in v2:
> - new patch: sd: Skip write protect groups check in CMD24/25 for high 
> capacity cards
> 
>  hw/sd/sd.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 3/8] hw/sd: sd: Fix CMD30 response type

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng 
> 
> Per the "Physical Layer Specification Version 8.00", table 4-26
> (SD mode) and table 7-3 (SPI mode) command descriptions, CMD30
> response type is R1, not R1b.
> 
> Fixes: a1bb27b1e98a ("SD card emulation initial implementation")
> Signed-off-by: Bin Meng 
> 
> ---
> 
> Changes in v2:
> - new patch: sd: Fix CMD30 response type
> 
>  hw/sd/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] nbd: server: Report holes for raw images

[Eric Blake]

On 2/19/21 10:42 AM, Eric Blake wrote:

>> To me, data=false looks compatible with NBD_STATE_HOLE. From user point
>> of view, getting same results from qemu-nbd and qemu-img is more
>> important than being more correct about allocation status.
> 
> More to the point, here is our inconsistency:
> 
> In nbd/server.c, we turn !BDRV_BLOCK_ALLOCATED into NBD_STATE_HOLE
> 
> In block/nbd.c, we turn !NBD_STATE_HOLE into BDRV_BLOCK_DATA
> 
> The fact that we are not doing a round-trip conversion means that one of
> the two places is wrong.  And your argument that the server side is
> wrong makes sense to me.

In fact, when I went back and researched when this was introduced (see
commit e7b1948d51 in 2018), we may have been aware of the inconsistency
between client and server, but didn't make up our minds at the time:
https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg03465.html
"? Hm, don't remember, what we decided about DATA/HOLE flags mapping.."

> 
> I'll wait a few days for any other reviewer commentary before taking
> this through my NBD tree.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] nbd: server: Report holes for raw images

[Eric Blake]

On 2/19/21 10:07 AM, Nir Soffer wrote:
> When querying image extents for raw image, qemu-nbd reports holes as
> zero:
> 
> $ qemu-nbd -t -r -f raw empty-6g.raw
> 
> $ qemu-img map --output json nbd://localhost
> [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": true, 
> "offset": 0}]
> 
> $ qemu-img map --output json empty-6g.raw
> [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": false, 
> "offset": 0}]
> 
> Turns out that qemu-img map reports a hole based on BDRV_BLOCK_DATA, but
> nbd server reports a hole based on BDRV_BLOCK_ALLOCATED.
> 
> The NBD protocol says:
> 
> NBD_STATE_HOLE (bit 0): if set, the block represents a hole (and
> future writes to that area may cause fragmentation or encounter an
> NBD_ENOSPC error); if clear, the block is allocated or the server
> could not otherwise determine its status.

Notoriously, lseek(SEEK_HOLE) cannot actually tell us about which
portions of a file are allocated (it can only reliably tell us which
portions of a file read as zero); you have to use the FIEMAP ioctl or
similar to learn about allocation status of a raw file, which is too
expensive to be useful.  The fact that SEEK_HOLE does not tell us about
NBD_STATE_HOLE, but instead is solely about NBD_STATE_ZERO, is therefore
a misnomer that doesn't make it any easier to reason about.

But this patch is also an indication that our meaning of
BDRV_BLOCK_ALLOCATED is confused; there has been a mismatch over time on
whether it represents whether something is allocated by occupying space
on disk (which as pointed out above is too expensive to determine for
files, but easy to determine for qcow2) or represents allocated at this
depth within a backing chain (makes no sense for protocol layers, but
total sense for format layers like qcow2).  At one point, Vladimir
audited the code base to try and reserve BDRV_BLOCK_ALLOCATED solely for
the latter meaning (which layer of a backing chain supplies the data,
regardless of whether that extent occupies reserved storage space).

So on that grounds alone, I am inclined to agree with you that using
BDRV_BLOCK_ALLOCATED to feed NBD_STATE_HOLE is wrong: whether something
comes from the current layer of the backing image is unrelated to
whether that extent is known to occupy reserved storage in the file
system.  Failure to sest NBD_STATE_HOLE is not fatal (the NBD protocol
specifically chose NBD_STATE_HOLE and NBD_STATE_ZERO to have default
values of 0 in the common case, where setting them to 1 allows some
optimizations, but where skipping the chance to set them to 1 because
determining the answer is too expensive, such as FIEMAP, is fine as well).

> 
> qemu-img manual says:
> 
> whether the sectors contain actual data or not (boolean field data;
> if false, the sectors are either unallocated or stored as
> optimized all-zero clusters);
> 
> To me, data=false looks compatible with NBD_STATE_HOLE. From user point
> of view, getting same results from qemu-nbd and qemu-img is more
> important than being more correct about allocation status.

More to the point, here is our inconsistency:

In nbd/server.c, we turn !BDRV_BLOCK_ALLOCATED into NBD_STATE_HOLE

In block/nbd.c, we turn !NBD_STATE_HOLE into BDRV_BLOCK_DATA

The fact that we are not doing a round-trip conversion means that one of
the two places is wrong.  And your argument that the server side is
wrong makes sense to me.

> 
> Changing nbd server to report holes using BDRV_BLOCK_DATA makes qemu-nbd
> results compatible with qemu-img map:
> 
> $ qemu-img map --output json nbd://localhost
> [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": false, 
> "offset": 0}]
> 
> Signed-off-by: Nir Soffer 
> ---
>  nbd/server.c   | 4 ++--
>  tests/qemu-iotests/241.out | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

Conflicts with my patch to make NBD_CMD_READ/NBD_CMD_BLOCK_STATUS obey
block alignment boundaries:
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg06010.html

Reviewed-by: Eric Blake 

I'll wait a few days for any other reviewer commentary before taking
this through my NBD tree.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH] nbd: server: Report holes for raw images

[Nir Soffer]

When querying image extents for raw image, qemu-nbd reports holes as
zero:

$ qemu-nbd -t -r -f raw empty-6g.raw

$ qemu-img map --output json nbd://localhost
[{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": true, 
"offset": 0}]

$ qemu-img map --output json empty-6g.raw
[{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": false, 
"offset": 0}]

Turns out that qemu-img map reports a hole based on BDRV_BLOCK_DATA, but
nbd server reports a hole based on BDRV_BLOCK_ALLOCATED.

The NBD protocol says:

NBD_STATE_HOLE (bit 0): if set, the block represents a hole (and
future writes to that area may cause fragmentation or encounter an
NBD_ENOSPC error); if clear, the block is allocated or the server
could not otherwise determine its status.

qemu-img manual says:

whether the sectors contain actual data or not (boolean field data;
if false, the sectors are either unallocated or stored as
optimized all-zero clusters);

To me, data=false looks compatible with NBD_STATE_HOLE. From user point
of view, getting same results from qemu-nbd and qemu-img is more
important than being more correct about allocation status.

Changing nbd server to report holes using BDRV_BLOCK_DATA makes qemu-nbd
results compatible with qemu-img map:

$ qemu-img map --output json nbd://localhost
[{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": false, 
"offset": 0}]

Signed-off-by: Nir Soffer 
---
 nbd/server.c   | 4 ++--
 tests/qemu-iotests/241.out | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 7229f487d2..86a44a9b41 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2087,8 +2087,8 @@ static int blockstatus_to_extents(BlockDriverState *bs, 
uint64_t offset,
 return ret;
 }
 
-flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
-(ret & BDRV_BLOCK_ZERO  ? NBD_STATE_ZERO : 0);
+flags = (ret & BDRV_BLOCK_DATA ? 0 : NBD_STATE_HOLE) |
+(ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
 
 if (nbd_extent_array_add(ea, num, flags) < 0) {
 return 0;
diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
index 75f9f465e5..3f8c173cc8 100644
--- a/tests/qemu-iotests/241.out
+++ b/tests/qemu-iotests/241.out
@@ -5,7 +5,7 @@ QA output created by 241
   size:  1024
   min block: 1
 [{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, 
"offset": OFFSET},
-{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, 
"offset": OFFSET}]
+{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": false, 
"offset": OFFSET}]
 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0)
 
 === Exporting unaligned raw image, forced server sector alignment ===
@@ -23,6 +23,6 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' 
and probing guessed
   size:  1024
   min block: 1
 [{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, 
"offset": OFFSET},
-{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, 
"offset": OFFSET}]
+{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": false, 
"offset": OFFSET}]
 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0)
 *** done
-- 
2.26.2

Re: [PATCH 3/3] iotests/283: Check that finalize drops backup-top

[Max Reitz]

On 19.02.21 16:33, Max Reitz wrote:

Without any of HEAD^ or HEAD^^ applied, qemu will most likely crash on
the qemu-io invocation, for a variety of immediate reasons.  The
underlying problem is generally a use-after-free access into
backup-top's BlockCopyState.

With only HEAD^ applied, qemu-io will run into an EIO (which is not
capture by the output, but you can see that the qemu-io invocation will
be accepted (i.e., qemu-io will run) in contrast to the reference
output, where the node name cannot be found), and qemu will then crash
in query-named-block-nodes: bdrv_get_allocated_file_size() detects
backup-top to be a filter and passes the request through to its child.
However, after bdrv_backup_top_drop(), that child is NULL, so the
recursive call crashes.

With HEAD^^ applied, this test should pass.

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/283 | 55 ++
  tests/qemu-iotests/283.out | 15 +++
  2 files changed, 70 insertions(+)

diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
index 79643e375b..509dcbbcf4 100755
--- a/tests/qemu-iotests/283
+++ b/tests/qemu-iotests/283
@@ -97,3 +97,58 @@ vm.qmp_log('blockdev-add', **{
  vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
  
  vm.shutdown()

+
+
+"""
+Check that the backup-top node is gone after job-finalize.
+
+During finalization, the node becomes inactive and can no longer
+function.  If it is still present, new parents might be attached, and
+there would be no meaningful way to handle their I/O requests.
+"""


Oh no, 297/pylint complains that this “string statement has no effect”. 
 Guess it should be a normal comment under the following print() then...


Max


+print('\n=== backup-top should be gone after job-finalize ===\n')

[PATCH 2/3] backup-top: Refuse I/O in inactive state

[Max Reitz]

When the backup-top node transitions from active to inactive in
bdrv_backup_top_drop(), the BlockCopyState is freed and the filtered
child is removed, so the node effectively becomes unusable.

However, noone told its I/O functions this, so they will happily
continue accessing bs->backing and s->bcs.  Prevent that by aborting
early when s->active is false.

(After the preceding patch, the node should be gone after
bdrv_backup_top_drop(), so this should largely be a theoretical problem.
But still, better to be safe than sorry, and also I think it just makes
sense to check s->active in the I/O functions.)

Signed-off-by: Max Reitz 
---
 block/backup-top.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/block/backup-top.c b/block/backup-top.c
index d1253e1aa6..589e8b651d 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -45,6 +45,12 @@ static coroutine_fn int backup_top_co_preadv(
 BlockDriverState *bs, uint64_t offset, uint64_t bytes,
 QEMUIOVector *qiov, int flags)
 {
+BDRVBackupTopState *s = bs->opaque;
+
+if (!s->active) {
+return -EIO;
+}
+
 return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
 }
 
@@ -54,6 +60,10 @@ static coroutine_fn int backup_top_cbw(BlockDriverState *bs, 
uint64_t offset,
 BDRVBackupTopState *s = bs->opaque;
 uint64_t off, end;
 
+if (!s->active) {
+return -EIO;
+}
+
 if (flags & BDRV_REQ_WRITE_UNCHANGED) {
 return 0;
 }
-- 
2.29.2

[PATCH 1/3] backup: Remove nodes from job in .clean()

[Max Reitz]

The block job holds a reference to the backup-top node (because it is
passed as the main job BDS to block_job_create()).  Therefore,
bdrv_backup_top_drop() cannot delete the backup-top node (replacing it
by its child does not affect the job parent, because that has
.stay_at_node set).  That is a problem, because all of its I/O functions
assume the BlockCopyState (s->bcs) to be valid and that it has a
filtered child; but after bdrv_backup_top_drop(), neither of those
things are true.

It does not make sense to add new parents to backup-top after
backup_clean(), so we should detach it from the job before
bdrv_backup_top_drop().  Because there is no function to do that for a
single node, just detach all of the job's nodes -- the job does not do
anything past backup_clean() anyway.

Signed-off-by: Max Reitz 
---
 block/backup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/backup.c b/block/backup.c
index 94e6dcd72e..6cf2f974aa 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -103,6 +103,7 @@ static void backup_abort(Job *job)
 static void backup_clean(Job *job)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
+block_job_remove_all_bdrv(>common);
 bdrv_backup_top_drop(s->backup_top);
 }
 
-- 
2.29.2

[PATCH 3/3] iotests/283: Check that finalize drops backup-top

[Max Reitz]

Without any of HEAD^ or HEAD^^ applied, qemu will most likely crash on
the qemu-io invocation, for a variety of immediate reasons.  The
underlying problem is generally a use-after-free access into
backup-top's BlockCopyState.

With only HEAD^ applied, qemu-io will run into an EIO (which is not
capture by the output, but you can see that the qemu-io invocation will
be accepted (i.e., qemu-io will run) in contrast to the reference
output, where the node name cannot be found), and qemu will then crash
in query-named-block-nodes: bdrv_get_allocated_file_size() detects
backup-top to be a filter and passes the request through to its child.
However, after bdrv_backup_top_drop(), that child is NULL, so the
recursive call crashes.

With HEAD^^ applied, this test should pass.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/283 | 55 ++
 tests/qemu-iotests/283.out | 15 +++
 2 files changed, 70 insertions(+)

diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
index 79643e375b..509dcbbcf4 100755
--- a/tests/qemu-iotests/283
+++ b/tests/qemu-iotests/283
@@ -97,3 +97,58 @@ vm.qmp_log('blockdev-add', **{
 vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
 
 vm.shutdown()
+
+
+"""
+Check that the backup-top node is gone after job-finalize.
+
+During finalization, the node becomes inactive and can no longer
+function.  If it is still present, new parents might be attached, and
+there would be no meaningful way to handle their I/O requests.
+"""
+
+print('\n=== backup-top should be gone after job-finalize ===\n')
+
+vm = iotests.VM()
+vm.launch()
+
+vm.qmp_log('blockdev-add', **{
+'node-name': 'source',
+'driver': 'null-co',
+})
+
+vm.qmp_log('blockdev-add', **{
+'node-name': 'target',
+'driver': 'null-co',
+})
+
+vm.qmp_log('blockdev-backup',
+   job_id='backup',
+   device='source',
+   target='target',
+   sync='full',
+   filter_node_name='backup-filter',
+   auto_finalize=False,
+   auto_dismiss=False)
+
+vm.event_wait('BLOCK_JOB_PENDING', 5.0)
+
+# The backup-top filter should still be present prior to finalization
+assert vm.node_info('backup-filter') is not None
+
+vm.qmp_log('job-finalize', id='backup')
+vm.event_wait('BLOCK_JOB_COMPLETED', 5.0)
+
+# The filter should be gone now.  Check that by trying to access it
+# with qemu-io (which will most likely crash qemu if it is still
+# there.).
+vm.qmp_log('human-monitor-command',
+   command_line='qemu-io backup-filter "write 0 1M"')
+
+# (Also, do an explicit check.)
+assert vm.node_info('backup-filter') is None
+
+vm.qmp_log('job-dismiss', id='backup')
+vm.event_wait('JOB_STATUS_CHANGE', 5.0, {'data': {'status': 'null'}})
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index d8cff22cc1..7e9cd9a7d4 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -6,3 +6,18 @@
 {"return": {}}
 {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": 
"full", "target": "target"}}
 {"error": {"class": "GenericError", "desc": "Cannot set permissions for 
backup-top filter: Conflicts with use by other as 'image', which uses 'write' 
on base"}}
+
+=== backup-top should be gone after job-finalize ===
+
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": 
"source"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": 
"target"}}
+{"return": {}}
+{"execute": "blockdev-backup", "arguments": {"auto-dismiss": false, 
"auto-finalize": false, "device": "source", "filter-node-name": 
"backup-filter", "job-id": "backup", "sync": "full", "target": "target"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "backup"}}
+{"return": {}}
+{"execute": "human-monitor-command", "arguments": {"command-line": "qemu-io 
backup-filter \"write 0 1M\""}}
+{"return": "Error: Cannot find device= nor node_name=backup-filter\r\n"}
+{"execute": "job-dismiss", "arguments": {"id": "backup"}}
+{"return": {}}
-- 
2.29.2

[PATCH 0/3] backup-top: Don't crash on post-finalize accesses

[Max Reitz]

Hi,

After job-finalize, the backup-top node generally stays around.  That’s
quite a problem, because its BlockCopyState is already freed then, and
it has no filtered child.  We really want the node to be gone.

The only reference that realistically can keep it alive is that of the
backup job (though block_job_add_bdrv() called by block_job_create()).
Dropping that reference before bdrv_backup_top_drop() should[1] ensure
bdrv_backup_top_drop() will delete the node.

[1]: bdrv_backup_top_drop() replaces the backup-top node by its filtered
 child, which detaches all parents from backup-top but the ones with
 .stay_at_node set.  The only parent that does this is a block job.
 I don’t think nodes can be in use by multiple block jobs at once,
 so the only parent with .stay_at_node set can be backup-top’s own
 backup job.


Patch 2 is there kind of as a failsafe, and kind of because it just made
sense to me, even if it won’t do anything.


Max Reitz (3):
  backup: Remove nodes from job in .clean()
  backup-top: Refuse I/O in inactive state
  iotests/283: Check that finalize drops backup-top

 block/backup-top.c | 10 +++
 block/backup.c |  1 +
 tests/qemu-iotests/283 | 55 ++
 tests/qemu-iotests/283.out | 15 +++
 4 files changed, 81 insertions(+)

-- 
2.29.2

Re: [PATCH 5/5] iotests: add parallels-read-bitmap test

[Vladimir Sementsov-Ogievskiy]

19.02.2021 14:56, Denis V. Lunev wrote:

On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:

Test support for reading bitmap from parallels image format.
parallels-with-bitmap.bz2 is generated on Virtuozzo by
parallels-with-bitmap.sh

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  .../sample_images/parallels-with-bitmap.bz2   | Bin 0 -> 203 bytes
  .../sample_images/parallels-with-bitmap.sh|  33 ++
  .../qemu-iotests/tests/parallels-read-bitmap  |  57 ++
  .../tests/parallels-read-bitmap.out   |   6 ++
  4 files changed, 96 insertions(+)
  create mode 100644 tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
  create mode 100755 tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
  create mode 100755 tests/qemu-iotests/tests/parallels-read-bitmap
  create mode 100644 tests/qemu-iotests/tests/parallels-read-bitmap.out

diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 
b/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
new file mode 100644
index 
..54892fd4d01bf743d395bd4f3d896494146ab5a9
GIT binary patch
literal 203
zcmV;+05tzXT4*^jL0KkKS@=;0bpT+Hf7|^?KmNPH-R
Fx`3oHQ9u9y

literal 0
HcmV?d1

diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh 
b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
new file mode 100755
index 00..e4a1d71277
--- /dev/null
+++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
@@ -0,0 +1,33 @@

do we need Copyright notice here?


Will add


I am unsure that this script is to be
acceptable in QEMU repo. Anyway, it looks fine :)


Yes, it requires some proprietary tools, not available for most of developers..
On the other hand, I should document somehow, how did I create the image
(at least for future me). I can do it in a commit message, but I think better
is just add a script, it will not hurt.





+#!/bin/bash
+
+CT=parallels-with-bitmap-ct
+DIR=$PWD/parallels-with-bitmap-dir
+IMG=$DIR/root.hds
+XML=$DIR/DiskDescriptor.xml
+TARGET=parallels-with-bitmap.bz2
+
+rm -rf $DIR
+
+prlctl create $CT --vmtype ct
+prlctl set $CT --device-add hdd --image $DIR --recreate --size 2G
+
+# cleanup the image
+qemu-img create -f parallels $IMG 64G
+
+# create bitmap
+prlctl backup $CT
+
+prlctl set $CT --device-del hdd1
+prlctl destroy $CT
+
+dev=$(ploop mount $XML | sed -n 's/^Adding delta 
dev=\(\/dev\/ploop[0-9]\+\).*/\1/p')
+dd if=/dev/zero of=$dev bs=64K seek=5 count=2 oflag=direct
+dd if=/dev/zero of=$dev bs=64K seek=30 count=1 oflag=direct
+dd if=/dev/zero of=$dev bs=64K seek=10 count=3 oflag=direct
+ploop umount $XML  # bitmap name will be in the output
+
+bzip2 -z $IMG
+
+mv $IMG.bz2 $TARGET
+
+rm -rf $DIR
diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap 
b/tests/qemu-iotests/tests/parallels-read-bitmap
new file mode 100755
index 00..b50b79f509
--- /dev/null
+++ b/tests/qemu-iotests/tests/parallels-read-bitmap
@@ -0,0 +1,57 @@
+#!/usr/bin/env python3
+#
+# Test parallels load bitmap
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import json
+import iotests
+from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path
+
+iotests.script_initialize(supported_fmts=['parallels'])
+
+nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
+disk = iotests.file_path('disk')
+bitmap = 'e4f2eed0-37fe-4539-b50b-85d2e7fd235f'
+nbd_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}' \
+f',x-dirty-bitmap=qemu:dirty-bitmap:{bitmap}'
+
+
+iotests.unarchive_sample_image('parallels-with-bitmap', disk)
+
+iotests.unarchive_sample_image('parallels-with-bitmap', '/work/mega')

no-no-no, '/work/mega' is absolutely no way



Oops)




+
+
+with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}',
+f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk):
+out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts)
+chunks = json.loads(out)
+cluster = 64 * 1024
+
+log('dirty clusters (cluster size is 64K):')
+for c in chunks:
+assert c['start'] % cluster == 0
+assert c['length'] % cluster == 0
+if c['data']:
+continue
+
+a = c['start'] // cluster
+b = (c['start'] + c['length']) // cluster
+if b - a > 1:
+log(f'{a}-{b-1}')
+   

Re: [PATCH 3/5] parallels: support bitmap extension for read-only mode

[Vladimir Sementsov-Ogievskiy]

19.02.2021 14:47, Denis V. Lunev wrote:

On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/parallels.h |   6 +-
  block/parallels-ext.c | 286 ++
  block/parallels.c |  18 +++
  block/meson.build |   3 +-
  4 files changed, 311 insertions(+), 2 deletions(-)
  create mode 100644 block/parallels-ext.c

diff --git a/block/parallels.h b/block/parallels.h
index 5aa101cfc8..2dbb7668a3 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -48,7 +48,8 @@ typedef struct ParallelsHeader {
  uint64_t nb_sectors;
  uint32_t inuse;
  uint32_t data_off;
-char padding[12];
+uint32_t flags;
+uint64_t ext_off;
  } QEMU_PACKED ParallelsHeader;
  
  typedef enum ParallelsPreallocMode {

@@ -84,4 +85,7 @@ typedef struct BDRVParallelsState {
  Error *migration_blocker;
  } BDRVParallelsState;
  
+int parallels_read_format_extension(BlockDriverState *bs,

+int64_t ext_off, Error **errp);
+
  #endif
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
new file mode 100644
index 00..b825b55124
--- /dev/null
+++ b/block/parallels-ext.c
@@ -0,0 +1,286 @@
+/*
+ * Support for Parallels Format Extansion. It's a part of parallels format
+ * driver.

s/Extansion/Extension/
s/Support for Parallels/Support of Parallels/
s/It's a part of parallels format/It's a part of Parallels format/

+ *
+ * Copyright (c) 2021 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "block/block_int.h"
+#include "parallels.h"
+#include "crypto/hash.h"
+#include "qemu/uuid.h"
+
+#define PARALLELS_FORMAT_EXTENSION_MAGIC 0xAB234CEF23DCEA87ULL
+
+#define PARALLELS_END_OF_FEATURES_MAGIC 0x0ULL
+#define PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC 0x20385FAE252CB34AULL
+
+typedef struct ParallelsFormatExtensionHeader {
+uint64_t magic; /* PARALLELS_FORMAT_EXTENSION_MAGIC */
+uint8_t check_sum[16];
+} QEMU_PACKED ParallelsFormatExtensionHeader;
+
+typedef struct ParallelsFeatureHeader {
+uint64_t magic;
+uint64_t flags;
+uint32_t data_size;
+uint32_t _unused;
+} QEMU_PACKED ParallelsFeatureHeader;
+
+typedef struct ParallelsDirtyBitmapFeature {
+uint64_t size;
+uint8_t id[16];
+uint32_t granularity;
+uint32_t l1_size;
+/* L1 table follows */
+} QEMU_PACKED ParallelsDirtyBitmapFeature;
+
+/* Given L1 table read bitmap data from the image and populate @bitmap */
+static int parallels_load_bitmap_data(BlockDriverState *bs,
+  const uint64_t *l1_table,
+  uint32_t l1_size,
+  BdrvDirtyBitmap *bitmap,
+  Error **errp)
+{
+BDRVParallelsState *s = bs->opaque;
+int ret = 0;
+uint64_t offset, limit;
+int cluster_size = s->tracks << BDRV_SECTOR_BITS;


should we save cluster size to BDS or create helper for the purpose?
Such operation through the code is looking terrible. Originally it was
available in one place only and that was acceptable. Now it spreads
over and over.


Agree, will do.




+uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+uint8_t *buf = NULL;
+uint64_t i, tab_size =
+DIV_ROUND_UP(bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size),
+ cluster_size);
+
+if (tab_size != l1_size) {
+error_setg(errp, "Bitmap table size %" PRIu32 " does not correspond "
+   "to bitmap size and cluster size. Expected %" PRIu64,
+   l1_size, tab_size);
+return -EINVAL;
+}
+
+buf = qemu_blockalign(bs, cluster_size);
+limit = bdrv_dirty_bitmap_serialization_coverage(cluster_size, bitmap);
+for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
+

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Philippe Mathieu-Daudé]

On 2/19/21 12:07 PM, Max Reitz wrote:
> On 13.02.21 22:54, Fam Zheng wrote:
>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
>>> The null-co driver doesn't zeroize buffer in its default config,
>>> because it is designed for testing and tests want to run fast.
>>> However this confuses security researchers (access to uninit
>>> buffers).
>>
>> I'm a little surprised.
>>
>> Is changing default the only way to fix this? I'm not opposed to
>> changing the default but I'm not convinced this is the easiest way.
>> block/nvme.c also doesn't touch the memory, but defers to the device
>> DMA, why doesn't that confuse the security checker?

Generally speaking, there is a balance between security and performance.
We try to provide both, but when we can't, my understanding is security
is more important.

Customers expect a secure product. If they prefer performance and
at the price of security, it is also possible by enabling an option
that is not the default.

I'm not sure why you mention block/nvme here. I have the understanding
the null-co driver is only useful for testing. Are there production
cases where null-co is used?

BTW block/nvme is a particular driver where performance matters more
than security (but we have to make sure the users are aware of that).

>> Cannot we just somehow annotate it in a way that the checker can
>> understand (akin to how we provide coverity models) and be done?
> 
> The question is, why wouldn’t we change the default?  read-zeroes=true
> seems the better default to me.  I consider silencing valgrind warnings
> and the like a nice side effect.
> 
> Max
> 

Re: [PATCH] iotests: Drop deprecated 'props' from object-add

[Kevin Wolf]

Am 19.02.2021 um 13:45 hat Alberto Garcia geschrieben:
> On Fri 19 Feb 2021 01:21:49 PM CET, Kevin Wolf  wrote:
> >>  log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
> >> -   props={ 'x-bps-total': size }))
> >> +   x_bps_total=size))
> >
> > x-bps-total isn't a stable interface, I'd prefer to use limits.
> >
> > My patch from November [1] had this:
> 
> Do you want me to resend mine, or wait for yours, or what then? :)

It's your patch, you decide. :-)

I haven't compared the patches in detail yet, so if you think merging my
patch has the same result and is less work, I can do that. If not and
you send a v2, I'll take that. Or you see differences and post review
comments to my patch instead of a v2 of yours. I'm fine with any of
these options.

Kevin

Re: [PATCH] iotests: Drop deprecated 'props' from object-add

[Alberto Garcia]

On Fri 19 Feb 2021 01:21:49 PM CET, Kevin Wolf  wrote:
>>  log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
>> -   props={ 'x-bps-total': size }))
>> +   x_bps_total=size))
>
> x-bps-total isn't a stable interface, I'd prefer to use limits.
>
> My patch from November [1] had this:

Do you want me to resend mine, or wait for yours, or what then? :)

Berto

Re: [PATCH] iotests: Drop deprecated 'props' from object-add

[Alberto Garcia]

On Fri 19 Feb 2021 01:04:00 PM CET, Max Reitz  wrote:
> Two Python syntax nit picks below.

>>   ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',
>> - props={'x-bps-read': 4096})
>> + x_bps_read = 4096)
>
> To stay consistent, I think there shouldn’t be spaces around '=' here.

Right, I didn't notice that.

>> @@ -103,10 +103,9 @@ def test_concurrent_finish(write_to_stream_node):
>>   vm.qmp_log('object-add',
>>  qom_type='throttle-group',
>>  id='tg',
>> -   props={
>> -   'x-iops-write': 1,
>> -   'x-iops-write-max': 1
>> -   })
>> +   x_iops_write=1,
>> +   x_iops_write_max=1
>> +   )
>
> This indentation looks weird to me now.  Unfortunately, flake8 finds
> this is the only correct indentation, so I have no reason to complain.
>
> Perhaps putting it on the preceding line would be better?

I'm fine either way, I can resend the patch with Kevin's suggestions.

Berto

Re: [PATCH] iotests: Drop deprecated 'props' from object-add

[Max Reitz]

On 16.02.21 18:16, Alberto Garcia wrote:

Signed-off-by: Alberto Garcia 
---
  tests/qemu-iotests/087 |  8 ++--
  tests/qemu-iotests/184 | 18 ++
  tests/qemu-iotests/218 |  2 +-
  tests/qemu-iotests/235 |  2 +-
  tests/qemu-iotests/245 |  4 ++--
  tests/qemu-iotests/258 |  7 +++
  tests/qemu-iotests/258.out |  4 ++--
  tests/qemu-iotests/295 |  2 +-
  tests/qemu-iotests/296 |  2 +-
  9 files changed, 19 insertions(+), 30 deletions(-)


Reviewed-by: Max Reitz 

Two Python syntax nit picks below.

[...]


diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index ae7c4fb187..cbb38923cf 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -152,7 +152,7 @@ with iotests.VM() as vm, \
  vm.launch()
  
  ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',

- props={'x-bps-read': 4096})
+ x_bps_read = 4096)


To stay consistent, I think there shouldn’t be spaces around '=' here.

(flake8 thinks so, too)


  assert ret['return'] == {}
  
  ret = vm.qmp('blockdev-add',


[..]


diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index 9a2d33ae5e..65ce02501a 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -103,10 +103,9 @@ def test_concurrent_finish(write_to_stream_node):
  vm.qmp_log('object-add',
 qom_type='throttle-group',
 id='tg',
-   props={
-   'x-iops-write': 1,
-   'x-iops-write-max': 1
-   })
+   x_iops_write=1,
+   x_iops_write_max=1
+   )


This indentation looks weird to me now.  Unfortunately, flake8 finds 
this is the only correct indentation, so I have no reason to complain.


Perhaps putting it on the preceding line would be better?

Re: [PATCH 5/5] iotests: add parallels-read-bitmap test

[Denis V. Lunev]

On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
> Test support for reading bitmap from parallels image format.
> parallels-with-bitmap.bz2 is generated on Virtuozzo by
> parallels-with-bitmap.sh
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  .../sample_images/parallels-with-bitmap.bz2   | Bin 0 -> 203 bytes
>  .../sample_images/parallels-with-bitmap.sh|  33 ++
>  .../qemu-iotests/tests/parallels-read-bitmap  |  57 ++
>  .../tests/parallels-read-bitmap.out   |   6 ++
>  4 files changed, 96 insertions(+)
>  create mode 100644 tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
>  create mode 100755 tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
>  create mode 100755 tests/qemu-iotests/tests/parallels-read-bitmap
>  create mode 100644 tests/qemu-iotests/tests/parallels-read-bitmap.out
>
> diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 
> b/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
> new file mode 100644
> index 
> ..54892fd4d01bf743d395bd4f3d896494146ab5a9
> GIT binary patch
> literal 203
> zcmV;+05tzXT4*^jL0KkKS@=;0bpT+Hf7|^?Km zk&7Szk`SoS002EkfMftPG z5P$(X{` zv(i3x^K~wt!aLPcRBP+PckUsIh6*LgjYSh0`}#7hMC9NR5D)+W0d&8Mxgwk>NPH-R
> Fx`3oHQ9u9y
>
> literal 0
> HcmV?d1
>
> diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh 
> b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
> new file mode 100755
> index 00..e4a1d71277
> --- /dev/null
> +++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
> @@ -0,0 +1,33 @@
do we need Copyright notice here? I am unsure that this script is to be
acceptable in QEMU repo. Anyway, it looks fine :)


> +#!/bin/bash
> +
> +CT=parallels-with-bitmap-ct
> +DIR=$PWD/parallels-with-bitmap-dir
> +IMG=$DIR/root.hds
> +XML=$DIR/DiskDescriptor.xml
> +TARGET=parallels-with-bitmap.bz2
> +
> +rm -rf $DIR
> +
> +prlctl create $CT --vmtype ct
> +prlctl set $CT --device-add hdd --image $DIR --recreate --size 2G
> +
> +# cleanup the image
> +qemu-img create -f parallels $IMG 64G
> +
> +# create bitmap
> +prlctl backup $CT
> +
> +prlctl set $CT --device-del hdd1
> +prlctl destroy $CT
> +
> +dev=$(ploop mount $XML | sed -n 's/^Adding delta 
> dev=\(\/dev\/ploop[0-9]\+\).*/\1/p')
> +dd if=/dev/zero of=$dev bs=64K seek=5 count=2 oflag=direct
> +dd if=/dev/zero of=$dev bs=64K seek=30 count=1 oflag=direct
> +dd if=/dev/zero of=$dev bs=64K seek=10 count=3 oflag=direct
> +ploop umount $XML  # bitmap name will be in the output
> +
> +bzip2 -z $IMG
> +
> +mv $IMG.bz2 $TARGET
> +
> +rm -rf $DIR
> diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap 
> b/tests/qemu-iotests/tests/parallels-read-bitmap
> new file mode 100755
> index 00..b50b79f509
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/parallels-read-bitmap
> @@ -0,0 +1,57 @@
> +#!/usr/bin/env python3
> +#
> +# Test parallels load bitmap
> +#
> +# Copyright (c) 2021 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +
> +import json
> +import iotests
> +from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path
> +
> +iotests.script_initialize(supported_fmts=['parallels'])
> +
> +nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
> +disk = iotests.file_path('disk')
> +bitmap = 'e4f2eed0-37fe-4539-b50b-85d2e7fd235f'
> +nbd_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}' \
> +f',x-dirty-bitmap=qemu:dirty-bitmap:{bitmap}'
> +
> +
> +iotests.unarchive_sample_image('parallels-with-bitmap', disk)
> +
> +iotests.unarchive_sample_image('parallels-with-bitmap', '/work/mega')
no-no-no, '/work/mega' is absolutely no way


> +
> +
> +with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}',
> +f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk):
> +out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts)
> +chunks = json.loads(out)
> +cluster = 64 * 1024
> +
> +log('dirty clusters (cluster size is 64K):')
> +for c in chunks:
> +assert c['start'] % cluster == 0
> +assert c['length'] % cluster == 0
> +if c['data']:
> +continue
> +
> +a = c['start'] // cluster
> +b = (c['start'] + c['length']) // cluster
> +if b - a > 1:
> +log(f'{a}-{b-1}')
> + 

Re: [PATCH 3/5] parallels: support bitmap extension for read-only mode

[Denis V. Lunev]

On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/parallels.h |   6 +-
>  block/parallels-ext.c | 286 ++
>  block/parallels.c |  18 +++
>  block/meson.build |   3 +-
>  4 files changed, 311 insertions(+), 2 deletions(-)
>  create mode 100644 block/parallels-ext.c
>
> diff --git a/block/parallels.h b/block/parallels.h
> index 5aa101cfc8..2dbb7668a3 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -48,7 +48,8 @@ typedef struct ParallelsHeader {
>  uint64_t nb_sectors;
>  uint32_t inuse;
>  uint32_t data_off;
> -char padding[12];
> +uint32_t flags;
> +uint64_t ext_off;
>  } QEMU_PACKED ParallelsHeader;
>  
>  typedef enum ParallelsPreallocMode {
> @@ -84,4 +85,7 @@ typedef struct BDRVParallelsState {
>  Error *migration_blocker;
>  } BDRVParallelsState;
>  
> +int parallels_read_format_extension(BlockDriverState *bs,
> +int64_t ext_off, Error **errp);
> +
>  #endif
> diff --git a/block/parallels-ext.c b/block/parallels-ext.c
> new file mode 100644
> index 00..b825b55124
> --- /dev/null
> +++ b/block/parallels-ext.c
> @@ -0,0 +1,286 @@
> +/*
> + * Support for Parallels Format Extansion. It's a part of parallels format
> + * driver.
s/Extansion/Extension/
s/Support for Parallels/Support of Parallels/
s/It's a part of parallels format/It's a part of Parallels format/
> + *
> + * Copyright (c) 2021 Virtuozzo International GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "block/block_int.h"
> +#include "parallels.h"
> +#include "crypto/hash.h"
> +#include "qemu/uuid.h"
> +
> +#define PARALLELS_FORMAT_EXTENSION_MAGIC 0xAB234CEF23DCEA87ULL
> +
> +#define PARALLELS_END_OF_FEATURES_MAGIC 0x0ULL
> +#define PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC 0x20385FAE252CB34AULL
> +
> +typedef struct ParallelsFormatExtensionHeader {
> +uint64_t magic; /* PARALLELS_FORMAT_EXTENSION_MAGIC */
> +uint8_t check_sum[16];
> +} QEMU_PACKED ParallelsFormatExtensionHeader;
> +
> +typedef struct ParallelsFeatureHeader {
> +uint64_t magic;
> +uint64_t flags;
> +uint32_t data_size;
> +uint32_t _unused;
> +} QEMU_PACKED ParallelsFeatureHeader;
> +
> +typedef struct ParallelsDirtyBitmapFeature {
> +uint64_t size;
> +uint8_t id[16];
> +uint32_t granularity;
> +uint32_t l1_size;
> +/* L1 table follows */
> +} QEMU_PACKED ParallelsDirtyBitmapFeature;
> +
> +/* Given L1 table read bitmap data from the image and populate @bitmap */
> +static int parallels_load_bitmap_data(BlockDriverState *bs,
> +  const uint64_t *l1_table,
> +  uint32_t l1_size,
> +  BdrvDirtyBitmap *bitmap,
> +  Error **errp)
> +{
> +BDRVParallelsState *s = bs->opaque;
> +int ret = 0;
> +uint64_t offset, limit;
> +int cluster_size = s->tracks << BDRV_SECTOR_BITS;

should we save cluster size to BDS or create helper for the purpose?
Such operation through the code is looking terrible. Originally it was
available in one place only and that was acceptable. Now it spreads
over and over.

> +uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +uint8_t *buf = NULL;
> +uint64_t i, tab_size =
> +DIV_ROUND_UP(bdrv_dirty_bitmap_serialization_size(bitmap, 0, 
> bm_size),
> + cluster_size);
> +
> +if (tab_size != l1_size) {
> +error_setg(errp, "Bitmap table size %" PRIu32 " does not correspond "
> +   "to bitmap size and cluster size. Expected %" PRIu64,
> +   l1_size, tab_size);
> +return -EINVAL;
> +}
> +
> +buf = qemu_blockalign(bs, 

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Max Reitz]

On 13.02.21 22:54, Fam Zheng wrote:

On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:

The null-co driver doesn't zeroize buffer in its default config,
because it is designed for testing and tests want to run fast.
However this confuses security researchers (access to uninit
buffers).


I'm a little surprised.

Is changing default the only way to fix this? I'm not opposed to
changing the default but I'm not convinced this is the easiest way.
block/nvme.c also doesn't touch the memory, but defers to the device
DMA, why doesn't that confuse the security checker?

Cannot we just somehow annotate it in a way that the checker can
understand (akin to how we provide coverity models) and be done?


The question is, why wouldn’t we change the default?  read-zeroes=true 
seems the better default to me.  I consider silencing valgrind warnings 
and the like a nice side effect.


Max

[PATCH] virtio-blk: Respect discard granularity

[Akihiko Odaki]

Signed-off-by: Akihiko Odaki 
---
 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index bac2d6fa2b2..692fd17b0e0 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -965,7 +965,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 virtio_stl_p(vdev, _discard_sectors,
  s->conf.max_discard_sectors);
 virtio_stl_p(vdev, _sector_alignment,
- blk_size >> BDRV_SECTOR_BITS);
+ conf->discard_granularity >> BDRV_SECTOR_BITS);
 /*
  * We support only one segment per request since multiple segments
  * are not widely used and there are no userspace APIs that allow
-- 
2.24.3 (Apple Git-128)

[PATCH] block/file-posix: Optimize for macOS

[Akihiko Odaki]

This commit introduces "punch hole" operation and optimizes transfer
block size for macOS.

This commit introduces two additional members,
discard_granularity and opt_io to BlockSizes type in
include/block/block.h. Also, the members of the type are now
optional. Set -1 to discard_granularity and 0 to other members
for the default values.

Signed-off-by: Akihiko Odaki 
---
 block/file-posix.c| 40 ++--
 block/nvme.c  |  2 ++
 hw/block/block.c  | 12 ++--
 include/block/block.h |  2 ++
 4 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 00cdaaa2d41..defbc04c8e7 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -44,6 +44,7 @@
 #if defined(__APPLE__) && (__MACH__)
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1274,6 +1275,8 @@ static int hdev_probe_blocksizes(BlockDriverState *bs, 
BlockSizes *bsz)
 if (check_for_dasd(s->fd) < 0) {
 return -ENOTSUP;
 }
+bsz->opt_io = 0;
+bsz->discard_granularity = -1;
 ret = probe_logical_blocksize(s->fd, >log);
 if (ret < 0) {
 return ret;
@@ -1568,6 +1571,7 @@ out:
 }
 }
 
+G_GNUC_UNUSED
 static int translate_err(int err)
 {
 if (err == -ENODEV || err == -ENOSYS || err == -EOPNOTSUPP ||
@@ -1777,16 +1781,27 @@ static int handle_aiocb_discard(void *opaque)
 }
 } while (errno == EINTR);
 
-ret = -errno;
+ret = translate_err(-errno);
 #endif
 } else {
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
 ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
aiocb->aio_offset, aiocb->aio_nbytes);
+ret = translate_err(-errno);
+#elif defined(__APPLE__) && (__MACH__)
+fpunchhole_t fpunchhole;
+fpunchhole.fp_flags = 0;
+fpunchhole.reserved = 0;
+fpunchhole.fp_offset = aiocb->aio_offset;
+fpunchhole.fp_length = aiocb->aio_nbytes;
+if (fcntl(s->fd, F_PUNCHHOLE, ) == -1) {
+ret = errno == ENODEV ? -ENOTSUP : -errno;
+} else {
+ret = 0;
+}
 #endif
 }
 
-ret = translate_err(ret);
 if (ret == -ENOTSUP) {
 s->has_discard = false;
 }
@@ -2095,6 +2110,26 @@ static int raw_co_flush_to_disk(BlockDriverState *bs)
 return raw_thread_pool_submit(bs, handle_aiocb_flush, );
 }
 
+static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
+{
+#if defined(__APPLE__) && (__MACH__)
+BDRVRawState *s = bs->opaque;
+struct statfs buf;
+
+if (!fstatfs(s->fd, )) {
+bsz->phys = 0;
+bsz->log = 0;
+bsz->opt_io = buf.f_iosize;
+bsz->discard_granularity = buf.f_bsize;
+return 0;
+}
+
+return -errno;
+#else
+return -ENOTSUP;
+#endif
+}
+
 static void raw_aio_attach_aio_context(BlockDriverState *bs,
AioContext *new_context)
 {
@@ -3229,6 +3264,7 @@ BlockDriver bdrv_file = {
 .bdrv_refresh_limits = raw_refresh_limits,
 .bdrv_io_plug = raw_aio_plug,
 .bdrv_io_unplug = raw_aio_unplug,
+.bdrv_probe_blocksizes = raw_probe_blocksizes,
 .bdrv_attach_aio_context = raw_aio_attach_aio_context,
 
 .bdrv_co_truncate = raw_co_truncate,
diff --git a/block/nvme.c b/block/nvme.c
index 5a6fbacf4a5..6d84bbdb632 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -983,6 +983,8 @@ static int nvme_probe_blocksizes(BlockDriverState *bs, 
BlockSizes *bsz)
 uint32_t blocksize = nvme_get_blocksize(bs);
 bsz->phys = blocksize;
 bsz->log = blocksize;
+bsz->opt_io = 0;
+bsz->discard_granularity = -1;
 return 0;
 }
 
diff --git a/hw/block/block.c b/hw/block/block.c
index 1e34573da71..c907e5a7722 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -70,19 +70,27 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
 backend_ret = blk_probe_blocksizes(blk, );
 /* fill in detected values if they are not defined via qemu command line */
 if (!conf->physical_block_size) {
-if (!backend_ret) {
+if (!backend_ret && blocksizes.phys) {
conf->physical_block_size = blocksizes.phys;
 } else {
 conf->physical_block_size = BDRV_SECTOR_SIZE;
 }
 }
 if (!conf->logical_block_size) {
-if (!backend_ret) {
+if (!backend_ret && blocksizes.log) {
 conf->logical_block_size = blocksizes.log;
 } else {
 conf->logical_block_size = BDRV_SECTOR_SIZE;
 }
 }
+if (!backend_ret) {
+if (!conf->opt_io_size) {
+conf->opt_io_size = blocksizes.opt_io;
+}
+if (conf->discard_granularity == -1) {
+conf->discard_granularity = blocksizes.discard_granularity;
+}
+}
 
 if (conf->logical_block_size > conf->physical_block_size) {
 error_setg(errp,
diff --git a/include/block/block.h b/include/block/block.h
index 

Re: [PATCH] block/file-posix: Optimize for macOS

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20210219085148.90545-1-akihiko.od...@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210219085148.90545-1-akihiko.od...@gmail.com
Subject: [PATCH] block/file-posix: Optimize for macOS

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20210219085148.90545-1-akihiko.od...@gmail.com -> 
patchew/20210219085148.90545-1-akihiko.od...@gmail.com
Switched to a new branch 'test'
455b4a3 block/file-posix: Optimize for macOS

=== OUTPUT BEGIN ===
WARNING: architecture specific defines should be avoided
#90: FILE: block/file-posix.c:2133:
+#if defined(__APPLE__) && (__MACH__)

ERROR: suspect code indent for conditional statements (8, 11)
#141: FILE: hw/block/block.c:73:
+if (!backend_ret && blocksizes.phys) {
conf->physical_block_size = blocksizes.phys;

total: 1 errors, 1 warnings, 129 lines checked

Commit 455b4a328821 (block/file-posix: Optimize for macOS) has style problems, 
please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210219085148.90545-1-akihiko.od...@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com