[2/2] lsi: use ldn_le_p()/stn_le_p()

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/scsi/lsi53c895a.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

Hi Sven,

On 2/18/19 6:55 PM, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  hw/scsi/lsi53c895a.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index c493e3c4c7..93c4434bfb 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -2078,29 +2078,14 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
>                            uint64_t val, unsigned size)
>  {
>      LSIState *s = opaque;
> -    uint32_t newval;
> -    uint32_t mask;
> -    int shift;
> -
> -    newval = s->script_ram[addr >> 2];
> -    shift = (addr & 3) * 8;
> -    mask = ((uint64_t)1 << (size * 8)) - 1;
> -    newval &= ~(mask << shift);
> -    newval |= val << shift;
> -    s->script_ram[addr >> 2] = newval;
> +    stn_le_p(((void*)s->script_ram) + addr, size, val);

If you want to do pointer arithmetic, it is safer to cast to a uintptr_t.
But since you update all the places that use script_ram[], it seems
pointless to keep it as an array of uint32_t. We can simply convert it
to an array of char.

Your patch looks sane otherwise,

Thanks,

Phil.

>  }
>  
>  static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
>                               unsigned size)
>  {
>      LSIState *s = opaque;
> -    uint32_t val;
> -    uint32_t mask;
> -
> -    val = s->script_ram[addr >> 2];
> -    mask = ((uint64_t)1 << (size * 8)) - 1;
> -    val >>= (addr & 3) * 8;
> -    return val & mask;
> +    return ldn_le_p(((void *)s->script_ram) + addr, size);
>  }
>  
>  static const MemoryRegionOps lsi_ram_ops = {
>

On Mon, 18 Feb 2019 at 21:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Sven,
>
> On 2/18/19 6:55 PM, Sven Schnelle wrote:
> > Signed-off-by: Sven Schnelle <svens@stackframe.org>
> > ---
> >  hw/scsi/lsi53c895a.c | 19 ++-----------------
> >  1 file changed, 2 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> > index c493e3c4c7..93c4434bfb 100644
> > --- a/hw/scsi/lsi53c895a.c
> > +++ b/hw/scsi/lsi53c895a.c
> > @@ -2078,29 +2078,14 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
> >                            uint64_t val, unsigned size)
> >  {
> >      LSIState *s = opaque;
> > -    uint32_t newval;
> > -    uint32_t mask;
> > -    int shift;
> > -
> > -    newval = s->script_ram[addr >> 2];
> > -    shift = (addr & 3) * 8;
> > -    mask = ((uint64_t)1 << (size * 8)) - 1;
> > -    newval &= ~(mask << shift);
> > -    newval |= val << shift;
> > -    s->script_ram[addr >> 2] = newval;
> > +    stn_le_p(((void*)s->script_ram) + addr, size, val);
>
> If you want to do pointer arithmetic, it is safer to cast to a uintptr_t.
> But since you update all the places that use script_ram[], it seems
> pointless to keep it as an array of uint32_t. We can simply convert it
> to an array of char.

Ah, yes -- when I suggested the cast on #qemu I hadn't
realized that these read and write functions were the
only places that access script_ram[] -- I'd assumed
that some code in the device model was going to read it to
execute whatever these scripts are.

thanks
-- PMM

Hi Philippe,

On Mon, Feb 18, 2019 at 10:39:54PM +0100, Philippe Mathieu-Daudé wrote:

> > -    newval = s->script_ram[addr >> 2];
> > -    shift = (addr & 3) * 8;
> > -    mask = ((uint64_t)1 << (size * 8)) - 1;
> > -    newval &= ~(mask << shift);
> > -    newval |= val << shift;
> > -    s->script_ram[addr >> 2] = newval;
> > +    stn_le_p(((void*)s->script_ram) + addr, size, val);
> 
> If you want to do pointer arithmetic, it is safer to cast to a uintptr_t.
> But since you update all the places that use script_ram[], it seems
> pointless to keep it as an array of uint32_t. We can simply convert it
> to an array of char.

You're right, i was assuming that the array is used somewhere else in the code,
but all the accesses are routed through these two functions, so it makes sense
to convert the type. However, i'm not sure whether i have to increase the version
number in the VMSTATE_BUFFER_UNSAFE() macro in that case? Are there possible
endianess issues with that change?

My current version looks like this:


commit 286d45946e235d5fdf2f95bf349b3048e3180392
Author: Sven Schnelle <svens@stackframe.org>
Date:   Tue Feb 19 06:59:23 2019 +0100

    lsi: use ldn_le_p()/stn_le_p()
    
    Signed-off-by: Sven Schnelle <svens@stackframe.org>

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index c493e3c4c7..0f9591016a 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -290,7 +290,7 @@ typedef struct {
     uint32_t adder;
 
     /* Script ram is stored as 32-bit words in host byteorder.  */
-    uint32_t script_ram[2048];
+    uint8_t script_ram[8192];
 } LSIState;
 
 #define TYPE_LSI53C810  "lsi53c810"
@@ -2078,29 +2078,14 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
                           uint64_t val, unsigned size)
 {
     LSIState *s = opaque;
-    uint32_t newval;
-    uint32_t mask;
-    int shift;
-
-    newval = s->script_ram[addr >> 2];
-    shift = (addr & 3) * 8;
-    mask = ((uint64_t)1 << (size * 8)) - 1;
-    newval &= ~(mask << shift);
-    newval |= val << shift;
-    s->script_ram[addr >> 2] = newval;
+    stn_le_p(s->script_ram + addr, size, val);
 }
 
 static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
                              unsigned size)
 {
     LSIState *s = opaque;
-    uint32_t val;
-    uint32_t mask;
-
-    val = s->script_ram[addr >> 2];
-    mask = ((uint64_t)1 << (size * 8)) - 1;
-    val >>= (addr & 3) * 8;
-    return val & mask;
+    return ldn_le_p(s->script_ram + addr, size);
 }
 
 static const MemoryRegionOps lsi_ram_ops = {
@@ -2243,7 +2228,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
         VMSTATE_BUFFER_UNSAFE(scratch, LSIState, 0, 18 * sizeof(uint32_t)),
         VMSTATE_UINT8(sbr, LSIState),
 
-        VMSTATE_BUFFER_UNSAFE(script_ram, LSIState, 0, 2048 * sizeof(uint32_t)),
+        VMSTATE_BUFFER_UNSAFE(script_ram, LSIState, 0, 8192),
         VMSTATE_END_OF_LIST()
     }
 };