Re: [Qemu-devel] [PATCH 2/2] lsi: use ldn_le_p()/stn_le_p()

[Sven Schnelle]

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 <address@hidden>
Date:   Tue Feb 19 06:59:23 2019 +0100

    lsi: use ldn_le_p()/stn_le_p()
    
    Signed-off-by: Sven Schnelle <address@hidden>

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()
     }
 };