[Qemu-devel] [PATCH 1/2] lsi: 810/895A are always little endian

[Qemu-devel] [PATCH 1/2] lsi: 810/895A are always little endian

[Sven Schnelle]

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

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index bcff859bac..c493e3c4c7 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -2061,14 +2061,13 @@ static uint64_t lsi_mmio_read(void *opaque, hwaddr addr,
                               unsigned size)
 {
     LSIState *s = opaque;
-
     return lsi_reg_readb(s, addr & 0xff);
 }
 
 static const MemoryRegionOps lsi_mmio_ops = {
     .read = lsi_mmio_read,
     .write = lsi_mmio_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
         .min_access_size = 1,
         .max_access_size = 1,
@@ -2107,7 +2106,7 @@ static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
 static const MemoryRegionOps lsi_ram_ops = {
     .read = lsi_ram_read,
     .write = lsi_ram_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static uint64_t lsi_io_read(void *opaque, hwaddr addr,
@@ -2127,7 +2126,7 @@ static void lsi_io_write(void *opaque, hwaddr addr,
 static const MemoryRegionOps lsi_io_ops = {
     .read = lsi_io_read,
     .write = lsi_io_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
         .min_access_size = 1,
         .max_access_size = 1,
-- 
2.20.1

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

[Sven Schnelle]

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);
 }
 
 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 = {
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

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 = {
> 

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

[Peter Maydell]

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

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

Re: [Qemu-devel] [PATCH 1/2] lsi: 810/895A are always little endian

[Sven Schnelle]

Hi Paolo,

can you please also queue this patch? This is the last one required to get HP-UX
10.20 running in QEMU. It still needs -d nochain, but that's a different story...

Thanks
Sven

On Mon, Feb 18, 2019 at 06:55:28PM +0100, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  hw/scsi/lsi53c895a.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index bcff859bac..c493e3c4c7 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -2061,14 +2061,13 @@ static uint64_t lsi_mmio_read(void *opaque, hwaddr addr,
>                                unsigned size)
>  {
>      LSIState *s = opaque;
> -
>      return lsi_reg_readb(s, addr & 0xff);
>  }
>  
>  static const MemoryRegionOps lsi_mmio_ops = {
>      .read = lsi_mmio_read,
>      .write = lsi_mmio_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>      .impl = {
>          .min_access_size = 1,
>          .max_access_size = 1,
> @@ -2107,7 +2106,7 @@ static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
>  static const MemoryRegionOps lsi_ram_ops = {
>      .read = lsi_ram_read,
>      .write = lsi_ram_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
>  static uint64_t lsi_io_read(void *opaque, hwaddr addr,
> @@ -2127,7 +2126,7 @@ static void lsi_io_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps lsi_io_ops = {
>      .read = lsi_io_read,
>      .write = lsi_io_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>      .impl = {
>          .min_access_size = 1,
>          .max_access_size = 1,
> -- 
> 2.20.1
> 
>