[PATCH for-5.2 v2 0/4] hw/net/can/ctucan: fix Coverity and other issues

[PATCH for-5.2 v2 0/4] hw/net/can/ctucan: fix Coverity and other issues

[Peter Maydell]

This patchset fixes a couple of issues spotted by Coverity:
 * incorrect address checks meant the guest could write off the
   end of the tx_buffer arrays
 * we had an unused value in ctucan_send_ready_buffers()
and also some I noticed while reading the code:
 * we don't adjust the device's non-portable use of bitfields
   on bigendian hosts
 * we should use stl_le_p() rather than casting uint_t* to
   uint32_t*

Tested with "make check" only.

Changes v1->v2: don't assert() the can't-happen case in patch 1,
to allow for future adjustment of #defines that correspond to
h/w synthesis parameters.

thanks
 -- PMM

Peter Maydell (4):
  hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer
  hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()
  hw/net/can/ctucan_core: Handle big-endian hosts
  hw/net/can/ctucan_core: Use stl_le_p to write to tx_buffers

 hw/net/can/ctucan_core.h |  3 +--
 hw/net/can/ctucan_core.c | 23 +++++++----------------
 2 files changed, 8 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH for-5.2 v2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer

[Peter Maydell]

The ctucan device has 4 CAN bus cores, each of which has a set of 20
32-bit registers for writing the transmitted data. The registers are
however not contiguous; each core's buffers is 0x100 bytes after
the last.

We got the checks on the address wrong in the ctucan_mem_write()
function:
 * the first "is addr in range at all" check allowed
   addr == CTUCAN_CORE_MEM_SIZE, which is actually the first
   byte off the end of the range
 * the decode of addresses into core-number plus offset in the
   tx buffer for that core failed to check that the offset was
   in range, so the guest could write off the end of the
   tx_buffer[] array

NB: currently the values of CTUCAN_CORE_MEM_SIZE, CTUCAN_CORE_TXBUF_NUM,
etc, make "buff_num >= CTUCAN_CORE_TXBUF_NUM" impossible, but we
retain this as a runtime check rather than an assertion to permit
those values to be changed in future (in hardware they are
configurable synthesis parameters).

Fix the top level check, and check the offset is within the buffer.

Fixes: Coverity CID 1432874
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/can/ctucan_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index d20835cd7e9..538270e62f9 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -303,7 +303,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr, uint64_t val,
     DPRINTF("write 0x%02llx addr 0x%02x\n",
             (unsigned long long)val, (unsigned int)addr);
 
-    if (addr > CTUCAN_CORE_MEM_SIZE) {
+    if (addr >= CTUCAN_CORE_MEM_SIZE) {
         return;
     }
 
@@ -312,7 +312,8 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr, uint64_t val,
         addr -= CTU_CAN_FD_TXTB1_DATA_1;
         buff_num = addr / CTUCAN_CORE_TXBUFF_SPAN;
         addr %= CTUCAN_CORE_TXBUFF_SPAN;
-        if (buff_num < CTUCAN_CORE_TXBUF_NUM) {
+        if ((buff_num < CTUCAN_CORE_TXBUF_NUM) ||
+            (addr < sizeof(s->tx_buffer[buff_num].data))) {
             uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data + addr);
             *bufp = cpu_to_le32(val);
         }
-- 
2.20.1

Re: [PATCH for-5.2 v2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer

[Pavel Pisa]

Hello Peter,

On Tuesday 10 of November 2020 18:06:01 Peter Maydell wrote:
> The ctucan device has 4 CAN bus cores, each of which has a set of 20
> 32-bit registers for writing the transmitted data. The registers are
> however not contiguous; each core's buffers is 0x100 bytes after
> the last.
>
> We got the checks on the address wrong in the ctucan_mem_write()
> function:
>  * the first "is addr in range at all" check allowed
>    addr == CTUCAN_CORE_MEM_SIZE, which is actually the first
>    byte off the end of the range
>  * the decode of addresses into core-number plus offset in the
>    tx buffer for that core failed to check that the offset was
>    in range, so the guest could write off the end of the
>    tx_buffer[] array
>
> NB: currently the values of CTUCAN_CORE_MEM_SIZE, CTUCAN_CORE_TXBUF_NUM,
> etc, make "buff_num >= CTUCAN_CORE_TXBUF_NUM" impossible, but we
> retain this as a runtime check rather than an assertion to permit
> those values to be changed in future (in hardware they are
> configurable synthesis parameters).
>
> Fix the top level check, and check the offset is within the buffer.
>
> Fixes: Coverity CID 1432874
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/can/ctucan_core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> index d20835cd7e9..538270e62f9 100644
> --- a/hw/net/can/ctucan_core.c
> +++ b/hw/net/can/ctucan_core.c
> @@ -303,7 +303,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> uint64_t val, DPRINTF("write 0x%02llx addr 0x%02x\n",
>              (unsigned long long)val, (unsigned int)addr);
>
> -    if (addr > CTUCAN_CORE_MEM_SIZE) {
> +    if (addr >= CTUCAN_CORE_MEM_SIZE) {
>          return;
>      }

Ack

> @@ -312,7 +312,8 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> uint64_t val, addr -= CTU_CAN_FD_TXTB1_DATA_1;
>          buff_num = addr / CTUCAN_CORE_TXBUFF_SPAN;
>          addr %= CTUCAN_CORE_TXBUFF_SPAN;
> -        if (buff_num < CTUCAN_CORE_TXBUF_NUM) {
> +        if ((buff_num < CTUCAN_CORE_TXBUF_NUM) ||
> +            (addr < sizeof(s->tx_buffer[buff_num].data))) {

should be &&

I would use 

+        if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
+            addr < CTUCAN_CORE_MSG_MAX_LEN) {

But that is equal. There can be problem that last three bytes of the uint32_t 
type can fall after the end. The correct changes to fully support
unaligned writes is not so easy an dis unnecessary for actual drivers
and use. So suggest

+        addr &= ~3;
+        if ((buff_num < CTUCAN_CORE_TXBUF_NUM) &&
+            (addr < sizeof(s->tx_buffer[buff_num].data))) {

You can consider that as Acked by me

>              uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data +
> addr); *bufp = cpu_to_le32(val);
>          }

Re: [PATCH for-5.2 v2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer

[Peter Maydell]

On Tue, 10 Nov 2020 at 18:02, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> On Tuesday 10 of November 2020 18:06:01 Peter Maydell wrote:
> > The ctucan device has 4 CAN bus cores, each of which has a set of 20
> > 32-bit registers for writing the transmitted data. The registers are
> > however not contiguous; each core's buffers is 0x100 bytes after
> > the last.
> >
> > We got the checks on the address wrong in the ctucan_mem_write()
> > function:
> >  * the first "is addr in range at all" check allowed
> >    addr == CTUCAN_CORE_MEM_SIZE, which is actually the first
> >    byte off the end of the range
> >  * the decode of addresses into core-number plus offset in the
> >    tx buffer for that core failed to check that the offset was
> >    in range, so the guest could write off the end of the
> >    tx_buffer[] array
> >
> > NB: currently the values of CTUCAN_CORE_MEM_SIZE, CTUCAN_CORE_TXBUF_NUM,
> > etc, make "buff_num >= CTUCAN_CORE_TXBUF_NUM" impossible, but we
> > retain this as a runtime check rather than an assertion to permit
> > those values to be changed in future (in hardware they are
> > configurable synthesis parameters).
> >
> > Fix the top level check, and check the offset is within the buffer.
> >
> > Fixes: Coverity CID 1432874
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/net/can/ctucan_core.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> > index d20835cd7e9..538270e62f9 100644
> > --- a/hw/net/can/ctucan_core.c
> > +++ b/hw/net/can/ctucan_core.c
> > @@ -303,7 +303,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> > uint64_t val, DPRINTF("write 0x%02llx addr 0x%02x\n",
> >              (unsigned long long)val, (unsigned int)addr);
> >
> > -    if (addr > CTUCAN_CORE_MEM_SIZE) {
> > +    if (addr >= CTUCAN_CORE_MEM_SIZE) {
> >          return;
> >      }
>
> Ack
>
> > @@ -312,7 +312,8 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> > uint64_t val, addr -= CTU_CAN_FD_TXTB1_DATA_1;
> >          buff_num = addr / CTUCAN_CORE_TXBUFF_SPAN;
> >          addr %= CTUCAN_CORE_TXBUFF_SPAN;
> > -        if (buff_num < CTUCAN_CORE_TXBUF_NUM) {
> > +        if ((buff_num < CTUCAN_CORE_TXBUF_NUM) ||
> > +            (addr < sizeof(s->tx_buffer[buff_num].data))) {
>
> should be &&

Whoops, that's a silly mistake on my part.

> I would use
>
> +        if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
> +            addr < CTUCAN_CORE_MSG_MAX_LEN) {
>
> But that is equal. There can be problem that last three bytes of the uint32_t
> type can fall after the end. The correct changes to fully support
> unaligned writes is not so easy an dis unnecessary for actual drivers
> and use. So suggest

>> +        addr &= ~3;
> +        if ((buff_num < CTUCAN_CORE_TXBUF_NUM) &&
> +            (addr < sizeof(s->tx_buffer[buff_num].data))) {

Hmm, yeah, the code is currently doing a 32-bit read regardless.

> You can consider that as Acked by me

OK, let's go with your version for 5.2.

For unaligned accesses, for 6.0, I think the code for doing
them to the txbuff at least is straightforward:

   if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
       (addr + size) < CTUCAN_CORE_MSG_MAX_LEN) {
      stn_le_p(s->tx_buffer[buff_num].data + addr, size, val);
   }

(stn_le_p takes care of doing an appropriate-width write.)

thanks
-- PMM

Re: [PATCH for-5.2 v2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer

[Pavel Pisa]

Hello Peter,

On Tuesday 10 of November 2020 19:24:03 Peter Maydell wrote:
> For unaligned accesses, for 6.0, I think the code for doing
> them to the txbuff at least is straightforward:
>
>    if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
>        (addr + size) < CTUCAN_CORE_MSG_MAX_LEN) {
>       stn_le_p(s->tx_buffer[buff_num].data + addr, size, val);
>    }
>
> (stn_le_p takes care of doing an appropriate-width write.)

Thanks, great to know, I like that much.
Only small nitpicking, it should be (addr + size) <= CTUCAN_CORE_MSG_MAX_LEN

So whole code I am testing now

    if (addr >= CTU_CAN_FD_TXTB1_DATA_1) {
        int buff_num;
        addr -= CTU_CAN_FD_TXTB1_DATA_1;
        buff_num = addr / CTUCAN_CORE_TXBUFF_SPAN;
        addr %= CTUCAN_CORE_TXBUFF_SPAN;
        if ((buff_num < CTUCAN_CORE_TXBUF_NUM) &&
            ((addr + size) <= sizeof(s->tx_buffer[buff_num].data))) {
            stn_le_p(s->tx_buffer[buff_num].data + addr, size, val);
        }
    } else {

So I have applied you whole series with above update. All works correctly
on x86_64 Linux host and with Linux x86_64 and MIPS big endian guests.

Please update to this combination.
I do not expect to have byte writes in our drivers but real core
supports byte enable bus signals.

Thanks much for teaching me QEMU stn_le_p.
In the fact, we are discussion about similar slution of peripherals
access for our https://github.com/cvut/QtMips/ education emulator
(performance vise a total toy when compared to QEMU).

It would worth to enable byte writes into registers as well.
But I would not do it before release. It would be more complex.
The reads supports bytes by reading 32/bit word and then shifting
and masking right bits into result. Cross word unaligned reads
are not supported. Again no reason for them now.

You can add

Tested-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

to whole series.

Thanks,

Pavel

Re: [PATCH for-5.2 v2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer

[Peter Maydell]

On Tue, 10 Nov 2020 at 19:32, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> On Tuesday 10 of November 2020 19:24:03 Peter Maydell wrote:
> > For unaligned accesses, for 6.0, I think the code for doing
> > them to the txbuff at least is straightforward:
> >
> >    if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
> >        (addr + size) < CTUCAN_CORE_MSG_MAX_LEN) {
> >       stn_le_p(s->tx_buffer[buff_num].data + addr, size, val);
> >    }
> >
> > (stn_le_p takes care of doing an appropriate-width write.)
>
> Thanks, great to know, I like that much.
> Only small nitpicking, it should be (addr + size) <= CTUCAN_CORE_MSG_MAX_LEN
>
> So whole code I am testing now
>
>     if (addr >= CTU_CAN_FD_TXTB1_DATA_1) {
>         int buff_num;
>         addr -= CTU_CAN_FD_TXTB1_DATA_1;
>         buff_num = addr / CTUCAN_CORE_TXBUFF_SPAN;
>         addr %= CTUCAN_CORE_TXBUFF_SPAN;
>         if ((buff_num < CTUCAN_CORE_TXBUF_NUM) &&
>             ((addr + size) <= sizeof(s->tx_buffer[buff_num].data))) {
>             stn_le_p(s->tx_buffer[buff_num].data + addr, size, val);
>         }
>     } else {
>
> So I have applied you whole series with above update. All works correctly
> on x86_64 Linux host and with Linux x86_64 and MIPS big endian guests.
>
> Please update to this combination.

If you've got a modified patch set that you've tested, would
you mind sending it out to the list? That would avoid my
possibly making mistakes in updating patches on my end and
then requiring you to repeat the testing.

thanks
-- PMM

Re: [PATCH for-5.2 v2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer

[Pavel Pisa]

Hello Peter,

On Tuesday 10 of November 2020 22:18:45 Peter Maydell wrote:
> If you've got a modified patch set that you've tested, would
> you mind sending it out to the list? That would avoid my
> possibly making mistakes in updating patches on my end and
> then requiring you to repeat the testing.

OK, I have tried to send it with your authorship and my
Signed-of-by at these patches which I have slightly
modified and with Acked-by of these which should stay
exactly same. If you prefer another style, send me a hint.

Thanks much to help us to make our code better,

         Pavel Pisa

[PATCH for-5.2 v2 2/4] hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()

[Peter Maydell]

Coverity points out that in ctucan_send_ready_buffers() we
set buff_st_mask = 0xf << (i * 4) inside the loop, but then
we never use it before overwriting it later.

The only thing we use the mask for is as part of the code that is
inserting the new buff_st field into tx_status.  That is more
comprehensibly written using deposit32(), so do that and drop the
mask variable entirely.

We also update the buff_st local variable at multiple points
during this function, but nothing can ever see these
intermediate values, so just drop those, write the final
TXT_TOK as a fixed constant value, and collapse the only
remaining set/use of buff_st down into an extract32().

Fixes: Coverity CID 1432869
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/can/ctucan_core.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index 538270e62f9..a400ad13a43 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -240,8 +240,6 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
     uint8_t *pf;
     int buff2tx_idx;
     uint32_t tx_prio_max;
-    unsigned int buff_st;
-    uint32_t buff_st_mask;
 
     if (!s->mode_settings.s.ena) {
         return;
@@ -256,10 +254,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
         for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
             uint32_t prio;
 
-            buff_st_mask = 0xf << (i * 4);
-            buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;
-
-            if (buff_st != TXT_RDY) {
+            if (extract32(s->tx_status.u32, i * 4, 4) != TXT_RDY) {
                 continue;
             }
             prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
@@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
         if (buff2tx_idx == -1) {
             break;
         }
-        buff_st_mask = 0xf << (buff2tx_idx * 4);
-        buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
         int_stat.u32 = 0;
-        buff_st = TXT_RDY;
         pf = s->tx_buffer[buff2tx_idx].data;
         ctucan_buff2frame(pf, &frame);
         s->status.s.idle = 0;
@@ -283,12 +275,11 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
         s->status.s.idle = 1;
         s->status.s.txs = 0;
         s->tx_fr_ctr.s.tx_fr_ctr_val++;
-        buff_st = TXT_TOK;
         int_stat.s.txi = 1;
         int_stat.s.txbhci = 1;
         s->int_stat.u32 |= int_stat.u32 & ~s->int_mask.u32;
-        s->tx_status.u32 = (s->tx_status.u32 & ~buff_st_mask) |
-                        (buff_st << (buff2tx_idx * 4));
+        s->tx_status.u32 = deposit32(s->tx_status.u32,
+                                     buff2tx_idx * 4, 4, TXT_TOK);
     } while (1);
 }
 
-- 
2.20.1

Re: [PATCH for-5.2 v2 2/4] hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()

[Pavel Pisa]

Hello Peter,

On Tuesday 10 of November 2020 18:06:02 Peter Maydell wrote:
> Coverity points out that in ctucan_send_ready_buffers() we
> set buff_st_mask = 0xf << (i * 4) inside the loop, but then
> we never use it before overwriting it later.
>
> The only thing we use the mask for is as part of the code that is
> inserting the new buff_st field into tx_status.  That is more
> comprehensibly written using deposit32(), so do that and drop the
> mask variable entirely.
>
> We also update the buff_st local variable at multiple points
> during this function, but nothing can ever see these
> intermediate values, so just drop those, write the final
> TXT_TOK as a fixed constant value, and collapse the only
> remaining set/use of buff_st down into an extract32().
>
> Fixes: Coverity CID 1432869
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/can/ctucan_core.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> index 538270e62f9..a400ad13a43 100644
> --- a/hw/net/can/ctucan_core.c
> +++ b/hw/net/can/ctucan_core.c
> @@ -240,8 +240,6 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) uint8_t *pf;
>      int buff2tx_idx;
>      uint32_t tx_prio_max;
> -    unsigned int buff_st;
> -    uint32_t buff_st_mask;
>
>      if (!s->mode_settings.s.ena) {
>          return;
> @@ -256,10 +254,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
>              uint32_t prio;
>
> -            buff_st_mask = 0xf << (i * 4);
> -            buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;
> -
> -            if (buff_st != TXT_RDY) {
> +            if (extract32(s->tx_status.u32, i * 4, 4) != TXT_RDY) {
>                  continue;
>              }
>              prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
> @@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) if (buff2tx_idx == -1) {
>              break;
>          }
> -        buff_st_mask = 0xf << (buff2tx_idx * 4);
> -        buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
>          int_stat.u32 = 0;
> -        buff_st = TXT_RDY;

I would prefer to add there next line even that it has no real effect

 +        s->tx_status.u32 = deposit32(s->tx_status.u32,
 +                                     buff2tx_idx * 4, 4, TXT_RDY);

But if it generates warning or you have some other reason not to put
it there, I add my

Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

When we separated processsing to call of message submit for Tx
and then separate callback to confirm arbitration win,
we would need to reintroduce this assignment. But there would
be much moresignificant changes that this small notice is not
so important. 

>          pf = s->tx_buffer[buff2tx_idx].data;
>          ctucan_buff2frame(pf, &frame);
>          s->status.s.idle = 0;
> @@ -283,12 +275,11 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) s->status.s.idle = 1;
>          s->status.s.txs = 0;
>          s->tx_fr_ctr.s.tx_fr_ctr_val++;
> -        buff_st = TXT_TOK;
>          int_stat.s.txi = 1;
>          int_stat.s.txbhci = 1;
>          s->int_stat.u32 |= int_stat.u32 & ~s->int_mask.u32;
> -        s->tx_status.u32 = (s->tx_status.u32 & ~buff_st_mask) |
> -                        (buff_st << (buff2tx_idx * 4));
> +        s->tx_status.u32 = deposit32(s->tx_status.u32,
> +                                     buff2tx_idx * 4, 4, TXT_TOK);
>      } while (1);
>  }


-- 
Yours sincerely

                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://dce.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/

Re: [PATCH for-5.2 v2 2/4] hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()

[Peter Maydell]

On Tue, 10 Nov 2020 at 19:37, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> On Tuesday 10 of November 2020 18:06:02 Peter Maydell wrote:
> > @@ -256,10 +254,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> > *s) for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
> >              uint32_t prio;
> >
> > -            buff_st_mask = 0xf << (i * 4);
> > -            buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;
> > -
> > -            if (buff_st != TXT_RDY) {
> > +            if (extract32(s->tx_status.u32, i * 4, 4) != TXT_RDY) {
> >                  continue;
> >              }
> >              prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
> > @@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> > *s) if (buff2tx_idx == -1) {
> >              break;
> >          }
> > -        buff_st_mask = 0xf << (buff2tx_idx * 4);
> > -        buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
> >          int_stat.u32 = 0;
> > -        buff_st = TXT_RDY;
>
> I would prefer to add there next line even that it has no real effect
>
>  +        s->tx_status.u32 = deposit32(s->tx_status.u32,
>  +                                     buff2tx_idx * 4, 4, TXT_RDY);

I mentioned this in a reply to my v1 series. The buffer status
in the tx_status field is already TXT_RDY, so there is no state
change happening here to document as far as I can tell ?

thanks
-- PMM

[PATCH for-5.2 v2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts

[Peter Maydell]

The ctucan driver defines types for its registers which are a union
of a uint32_t with a struct with bitfields for the individual
fields within that register. This is a bad idea, because bitfields
aren't portable. The ctu_can_fd_regs.h header works around the
most glaring of the portability issues by defining the
fields in two different orders depending on the setting of the
__LITTLE_ENDIAN_BITFIELD define. However, in ctucan_core.h this
is unconditionally set to 1, which is wrong for big-endian hosts.

Set it only if HOST_WORDS_BIGENDIAN is not set. There is no need
for a "have we defined it already" guard, because the only place
that should set it is ctucan_core.h, which has the usual
double-inclusion guard.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/can/ctucan_core.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h
index f21cb1c5ec3..bbc09ae0678 100644
--- a/hw/net/can/ctucan_core.h
+++ b/hw/net/can/ctucan_core.h
@@ -31,8 +31,7 @@
 #include "exec/hwaddr.h"
 #include "net/can_emu.h"
 
-
-#ifndef __LITTLE_ENDIAN_BITFIELD
+#ifndef HOST_WORDS_BIGENDIAN
 #define __LITTLE_ENDIAN_BITFIELD 1
 #endif
 
-- 
2.20.1

Re: [PATCH for-5.2 v2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts

[Pavel Pisa]

Hello Peter,

On Tuesday 10 of November 2020 18:06:03 Peter Maydell wrote:
> The ctucan driver defines types for its registers which are a union
> of a uint32_t with a struct with bitfields for the individual
> fields within that register. This is a bad idea, because bitfields
> aren't portable. The ctu_can_fd_regs.h header works around the
> most glaring of the portability issues by defining the
> fields in two different orders depending on the setting of the
> __LITTLE_ENDIAN_BITFIELD define. However, in ctucan_core.h this
> is unconditionally set to 1, which is wrong for big-endian hosts.
>
> Set it only if HOST_WORDS_BIGENDIAN is not set. There is no need
> for a "have we defined it already" guard, because the only place
> that should set it is ctucan_core.h, which has the usual
> double-inclusion guard.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/net/can/ctucan_core.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/net/can/ctucan_core.h b/hw/net/can/ctucan_core.h
> index f21cb1c5ec3..bbc09ae0678 100644
> --- a/hw/net/can/ctucan_core.h
> +++ b/hw/net/can/ctucan_core.h
> @@ -31,8 +31,7 @@
>  #include "exec/hwaddr.h"
>  #include "net/can_emu.h"
>
> -
> -#ifndef __LITTLE_ENDIAN_BITFIELD
> +#ifndef HOST_WORDS_BIGENDIAN
>  #define __LITTLE_ENDIAN_BITFIELD 1
>  #endif

Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

Thanks,

Pavel

[PATCH for-5.2 v2 4/4] hw/net/can/ctucan_core: Use stl_le_p to write to tx_buffers

[Peter Maydell]

Instead of casting an address within a uint8_t array to a
uint32_t*, use stl_le_p(). This handles possibly misaligned
addresses which would otherwise crash on some hosts.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
---
 hw/net/can/ctucan_core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index a400ad13a43..0ef528eb879 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -305,8 +305,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr, uint64_t val,
         addr %= CTUCAN_CORE_TXBUFF_SPAN;
         if ((buff_num < CTUCAN_CORE_TXBUF_NUM) ||
             (addr < sizeof(s->tx_buffer[buff_num].data))) {
-            uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data + addr);
-            *bufp = cpu_to_le32(val);
+            stl_le_p(s->tx_buffer[buff_num].data + addr, val);
         }
     } else {
         switch (addr & ~3) {
-- 
2.20.1

Re: [PATCH for-5.2 v2 4/4] hw/net/can/ctucan_core: Use stl_le_p to write to tx_buffers

[Pavel Pisa]

Hello Peter,

On Tuesday 10 of November 2020 18:06:04 Peter Maydell wrote:
> Instead of casting an address within a uint8_t array to a
> uint32_t*, use stl_le_p(). This handles possibly misaligned
> addresses which would otherwise crash on some hosts.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> ---
>  hw/net/can/ctucan_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> index a400ad13a43..0ef528eb879 100644
> --- a/hw/net/can/ctucan_core.c
> +++ b/hw/net/can/ctucan_core.c
> @@ -305,8 +305,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> uint64_t val, addr %= CTUCAN_CORE_TXBUFF_SPAN;
>          if ((buff_num < CTUCAN_CORE_TXBUF_NUM) ||
>              (addr < sizeof(s->tx_buffer[buff_num].data))) {
> -            uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data +
> addr); -            *bufp = cpu_to_le32(val);
> +            stl_le_p(s->tx_buffer[buff_num].data + addr, val);
>          }
>      } else {
>          switch (addr & ~3) {

I test change soon, but this seems obvious so

Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>