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

[Qemu-devel] [PATCH v4 0/4] 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. This bit is not relevant
in single block transfers. Also, 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 v3:
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02376.html
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02905.html

Thank you.
--
Prasad J Pandit (4):
  sd: sdhci: mask transfer mode register value
  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 | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v4 1/4] sd: sdhci: mask transfer mode register value

[P J P]

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

In SDHCI protocol, the transfer mode register is defined
to be of 6 bits. Mask its value with '0x0037' so that an
invalid value could not be assigned.

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

Update per: use macro for the mask value
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02774.html

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 5bd5ab6..cf647fa 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -119,6 +119,7 @@
     (SDHC_CAPAB_BASECLKFREQ << 8) | (SDHC_CAPAB_TOUNIT << 7) | \
     (SDHC_CAPAB_TOCLKFREQ))
 
+#define MASK_TRNMOD     0x0037
 #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
 
 static uint8_t sdhci_slotint(SDHCIState *s)
@@ -1050,7 +1051,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 & MASK_TRNMOD);
         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 v4 1/4] sd: sdhci: mask transfer mode register value

[Alistair Francis]

On Tue, Feb 14, 2017 at 10:52 AM, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> In SDHCI protocol, the transfer mode register is defined
> to be of 6 bits. Mask its value with '0x0037' so that an
> invalid value could not be assigned.
>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/sd/sdhci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> Update per: use macro for the mask value
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02774.html
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 5bd5ab6..cf647fa 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -119,6 +119,7 @@
>      (SDHC_CAPAB_BASECLKFREQ << 8) | (SDHC_CAPAB_TOUNIT << 7) | \
>      (SDHC_CAPAB_TOCLKFREQ))
>
> +#define MASK_TRNMOD     0x0037
>  #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
>
>  static uint8_t sdhci_slotint(SDHCIState *s)
> @@ -1050,7 +1051,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 & MASK_TRNMOD);
>          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 v4 2/4] sd: sdhci: check transfer mode register in multi block transfer

[P J P]

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

In the SDHCI protocol, 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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index cf647fa..f8220c0 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -487,6 +487,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) {
+        qemu_log_mask(LOG_UNIMP, "infinite transfer is 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 */
@@ -798,11 +803,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 {
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4 2/4] sd: sdhci: check transfer mode register in multi block transfer

[Alistair Francis]

On Tue, Feb 14, 2017 at 10:52 AM, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> In the SDHCI protocol, 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>

Didn't I already review this?

Can you please make sure to attach all tags from previous patches to
future patches?

Thanks,

Alistair

> ---
>  hw/sd/sdhci.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index cf647fa..f8220c0 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -487,6 +487,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) {
> +        qemu_log_mask(LOG_UNIMP, "infinite transfer is 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 */
> @@ -798,11 +803,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 {
> --
> 2.9.3
>

Re: [Qemu-devel] [PATCH v4 2/4] sd: sdhci: check transfer mode register in multi block transfer

[P J P]

  Hello Alistair,

+-- On Tue, 14 Feb 2017, Alistair Francis wrote --+
| On Tue, Feb 14, 2017 at 10:52 AM, P J P <ppandit@redhat.com> wrote:
| > From: Prasad J Pandit <pjp@fedoraproject.org>
| >
| > In the SDHCI protocol, 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>
| 
| Didn't I already review this?

Yes, you did. I resent it owing to thread

  -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02898.html

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v4 2/4] sd: sdhci: check transfer mode register in multi block transfer

[Peter Maydell]

On 15 February 2017 at 04:58, P J P <ppandit@redhat.com> wrote:
>   Hello Alistair,
>
> +-- On Tue, 14 Feb 2017, Alistair Francis wrote --+
> | On Tue, Feb 14, 2017 at 10:52 AM, P J P <ppandit@redhat.com> wrote:
> | > From: Prasad J Pandit <pjp@fedoraproject.org>
> | >
> | > In the SDHCI protocol, 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>
> |
> | Didn't I already review this?
>
> Yes, you did. I resent it owing to thread
>
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02898.html

Alistair's point is that when you resend a patchset where
some patches have got reviews/acks and those patches haven't
changed significantly, you should include the Reviewed-by:
or Acked-by: tags in the commit messages on the resent patches,
so that people know they don't need to reexamine those patches.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 2/4] sd: sdhci: check transfer mode register in multi block transfer

[P J P]

+-- On Fri, 17 Feb 2017, Peter Maydell wrote --+
| Alistair's point is that when you resend a patchset where some patches have 
| got reviews/acks and those patches haven't changed significantly, you should 
| include the Reviewed-by: or Acked-by: tags in the commit messages on the 
| resent patches, so that people know they don't need to reexamine those 
| patches.

  Ah okay, noted; Will do.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

[Qemu-devel] [PATCH v4 3/4] 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 f8220c0..8ae75fe 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1023,7 +1023,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_multi_blocks(s);
+            } else {
+                sdhci_sdma_transfer_single_block(s);
+            }
         }
         break;
     case SDHC_BLKSIZE:
-- 
2.9.3

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

[P J P]

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

In 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(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8ae75fe..05558d3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -570,7 +570,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;
@@ -589,10 +588,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

Re: [Qemu-devel] [PATCH v4 4/4] sd: sdhci: Remove block count enable check in single block transfers

[Alistair Francis]

On Tue, Feb 14, 2017 at 10:52 AM, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> In 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>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/sd/sdhci.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8ae75fe..05558d3 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -570,7 +570,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;
> @@ -589,10 +588,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
>

Re: [Qemu-devel] [PATCH v4 0/4] sd: sdhci: correct transfer mode register usage

[Peter Maydell]

On 14 February 2017 at 18:52, P J P <ppandit@redhat.com> wrote:
> 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. This bit is not relevant
> in single block transfers. Also, 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 v3:
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02376.html
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02905.html

I've gone back through the mail archives for previous versions of
this series, and I think that we just need review for patch 4 now?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 0/4] sd: sdhci: correct transfer mode register usage

[Alistair Francis]

On Fri, Feb 17, 2017 at 5:21 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 February 2017 at 18:52, P J P <ppandit@redhat.com> wrote:
>> 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. This bit is not relevant
>> in single block transfers. Also, 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 v3:
>>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02376.html
>>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02905.html
>
> I've gone back through the mail archives for previous versions of
> this series, and I think that we just need review for patch 4 now?

Thanks Peter, I thought I had reviewed all of them.

I just reviewed patch 4, these should be good to go now then.

Thanks,

Alistair

>
> thanks
> -- PMM