[Qemu-devel] [PATCH v2 0/3] sd: sdhci: correct transfer mode register usage

[Qemu-devel] [PATCH v2 0/3] sd: sdhci: correct transfer mode register usage

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

In SDHCI protocol, the 'Block Count Enable' bit of the Transfer Mode
register is used to control 's->blkcnt' value. One, this bit is not
relevant in single block transfers. Second, Transfer Mode register
value could be set such that 's->blkcnt' would not see an update
during multi block transfers. Thus leading to an infinite loop.

This patch set attempts to correct 'Block Count Enable' bit usage.

This series incorporates changes suggested in patch set v1:
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg06476.html

Thank you.
--
Prasad J Pandit (3):
  sd: sdhci: check transfer mode register in multi block transfer
  sd: sdhci: conditionally invoke multi block transfer
  sd: sdhci: Remove block count enable check in single block transfers

 hw/sd/sdhci.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

--
2.9.3

[Qemu-devel] [PATCH v2 1/3] sd: sdhci: check transfer mode register in multi block transfer

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

In SDHCI device emulation the transfer mode register value
is used during multi block transfer to check if block count
register is enabled and should be updated. Transfer mode
register could be set such that, block count register would
not be updated, thus leading to an infinite loop. Add check
to avoid it.

Reported-by: Wjjzhang <wjjzhang@tencent.com>
Reported-by: Jiang Xin <jiangxin1@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/sd/sdhci.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Update per: print an error message before return
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg01567.html

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 5bd5ab6..cf400c8 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -486,6 +486,11 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
     uint32_t boundary_chk = 1 << (((s->blksize & 0xf000) >> 12) + 12);
     uint32_t boundary_count = boundary_chk - (s->sdmasysad % boundary_chk);
 
+    if (!(s->trnmod & SDHC_TRNS_BLK_CNT_EN) || !s->blkcnt) {
+        ERRPRINT("Infinite transfers are not supported\n");
+        return;
+    }
+
     /* XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
      * possible stop at page boundary if initial address is not page aligned,
      * allow them to work properly */
@@ -797,11 +802,6 @@ static void sdhci_data_transfer(void *opaque)
     if (s->trnmod & SDHC_TRNS_DMA) {
         switch (SDHC_DMA_TYPE(s->hostctl)) {
         case SDHC_CTRL_SDMA:
-            if ((s->trnmod & SDHC_TRNS_MULTI) &&
-                    (!(s->trnmod & SDHC_TRNS_BLK_CNT_EN) || s->blkcnt == 0)) {
-                break;
-            }
-
             if ((s->blkcnt == 1) || !(s->trnmod & SDHC_TRNS_MULTI)) {
                 sdhci_sdma_transfer_single_block(s);
             } else {
@@ -1050,7 +1050,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         if (!(s->capareg & SDHC_CAN_DO_DMA)) {
             value &= ~SDHC_TRNS_DMA;
         }
-        MASKED_WRITE(s->trnmod, mask, value);
+        MASKED_WRITE(s->trnmod, mask, value & 0x0037);
         MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16);
 
         /* Writing to the upper byte of CMDREG triggers SD command generation */
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 1/3] sd: sdhci: check transfer mode register in multi block transfer

[Alistair Francis]

On Tue, Feb 7, 2017 at 10:42 PM, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> In SDHCI device emulation the transfer mode register value
> is used during multi block transfer to check if block count
> register is enabled and should be updated. Transfer mode
> register could be set such that, block count register would
> not be updated, thus leading to an infinite loop. Add check
> to avoid it.
>
> Reported-by: Wjjzhang <wjjzhang@tencent.com>
> Reported-by: Jiang Xin <jiangxin1@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/sd/sdhci.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> Update per: print an error message before return
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg01567.html
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 5bd5ab6..cf400c8 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -486,6 +486,11 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
>      uint32_t boundary_chk = 1 << (((s->blksize & 0xf000) >> 12) + 12);
>      uint32_t boundary_count = boundary_chk - (s->sdmasysad % boundary_chk);
>
> +    if (!(s->trnmod & SDHC_TRNS_BLK_CNT_EN) || !s->blkcnt) {
> +        ERRPRINT("Infinite transfers are not supported\n");
> +        return;
> +    }

You should use the qemu_log_mask() function. It'll look something like this:

qemu_log_mask(LOG_UNIMP, ....

> +
>      /* XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
>       * possible stop at page boundary if initial address is not page aligned,
>       * allow them to work properly */
> @@ -797,11 +802,6 @@ static void sdhci_data_transfer(void *opaque)
>      if (s->trnmod & SDHC_TRNS_DMA) {
>          switch (SDHC_DMA_TYPE(s->hostctl)) {
>          case SDHC_CTRL_SDMA:
> -            if ((s->trnmod & SDHC_TRNS_MULTI) &&
> -                    (!(s->trnmod & SDHC_TRNS_BLK_CNT_EN) || s->blkcnt == 0)) {
> -                break;
> -            }
> -
>              if ((s->blkcnt == 1) || !(s->trnmod & SDHC_TRNS_MULTI)) {
>                  sdhci_sdma_transfer_single_block(s);
>              } else {
> @@ -1050,7 +1050,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>          if (!(s->capareg & SDHC_CAN_DO_DMA)) {
>              value &= ~SDHC_TRNS_DMA;
>          }
> -        MASKED_WRITE(s->trnmod, mask, value);
> +        MASKED_WRITE(s->trnmod, mask, value & 0x0037);

Maybe this is worth being in a separate patch with an explanation.

Thanks,

Alistair

>          MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16);
>
>          /* Writing to the upper byte of CMDREG triggers SD command generation */
> --
> 2.9.3
>

[Qemu-devel] [PATCH v2 2/3] sd: sdhci: conditionally invoke multi block transfer

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

In sdhci_write invoke multi block transfer if it is enabled
in the transfer mode register 's->trnmod'.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/sd/sdhci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index cf400c8..532ef87 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1022,7 +1022,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         /* Writing to last byte of sdmasysad might trigger transfer */
         if (!(mask & 0xFF000000) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt &&
                 s->blksize && SDHC_DMA_TYPE(s->hostctl) == SDHC_CTRL_SDMA) {
-            sdhci_sdma_transfer_multi_blocks(s);
+            if (!(s->trnmod & SDHC_TRNS_MULTI)) {
+                sdhci_sdma_transfer_single_block(s);
+            } else {
+                sdhci_sdma_transfer_multi_blocks(s);
+            }
         }
         break;
     case SDHC_BLKSIZE:
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/3] sd: sdhci: conditionally invoke multi block transfer

[Alistair Francis]

On Tue, Feb 7, 2017 at 10:42 PM, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> In sdhci_write invoke multi block transfer if it is enabled
> in the transfer mode register 's->trnmod'.
>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/sd/sdhci.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index cf400c8..532ef87 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1022,7 +1022,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>          /* Writing to last byte of sdmasysad might trigger transfer */
>          if (!(mask & 0xFF000000) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt &&
>                  s->blksize && SDHC_DMA_TYPE(s->hostctl) == SDHC_CTRL_SDMA) {
> -            sdhci_sdma_transfer_multi_blocks(s);
> +            if (!(s->trnmod & SDHC_TRNS_MULTI)) {

You should test if true first.

Thanks,

Alistair

> +                sdhci_sdma_transfer_single_block(s);
> +            } else {
> +                sdhci_sdma_transfer_multi_blocks(s);
> +            }
>          }
>          break;
>      case SDHC_BLKSIZE:
> --
> 2.9.3
>

[Qemu-devel] [PATCH v2 3/3] sd: sdhci: Remove block count enable check in single block transfers

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

In the SDHCI protocol the 'Block count enable' bit
of the Transfer Mode register is relevant only in multi block
transfers. We need not check it in single block transfers.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/sd/sdhci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Update: change commit title and log message
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg01568.html

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 532ef87..95e11cd 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -569,7 +569,6 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 }
 
 /* single block SDMA transfer */
-
 static void sdhci_sdma_transfer_single_block(SDHCIState *s)
 {
     int n;
@@ -588,10 +587,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
             sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
         }
     }
-
-    if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
-        s->blkcnt--;
-    }
+    s->blkcnt--;
 
     sdhci_end_transfer(s);
 }
-- 
2.9.3