[Qemu-devel] [PATCH 0/3] hw/ppc: Convert various devices away from old_mmio

[Qemu-devel] [PATCH 0/3] hw/ppc: Convert various devices away from old_mmio

[Peter Maydell]

This patchset removes various uses of old_mmio from minor PPC
devices:
 * hw/ppc/prep had an entirely ifdeffed-out stub of an XCSR device,
   which we remove
 * hw/ppc/ppc_boards had ref405ep_fpga
 * hw/ppc/ppc405_uc had three minor devices

As you can see from the diffstat, the new API provides much
cleaner ways to handle the various different access sizes.

This knocks another five old_mmio uses out of the codebase,
leaving us with just five to go.

NB: Tested only with 'make check'.

thanks
-- PMM

Peter Maydell (3):
  hw/ppc/prep: Remove ifdeffed-out stub of XCSR code
  hw/ppc/ppc_boards: Don't use old_mmio for ref405ep_fpga
  hw/ppc/ppc405_uc: Convert away from old_mmio

 hw/ppc/ppc405_boards.c |  60 +++-----------
 hw/ppc/ppc405_uc.c     | 173 ++++++-----------------------------------
 hw/ppc/prep.c          |  97 +----------------------
 3 files changed, 39 insertions(+), 291 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/3] hw/ppc/prep: Remove ifdeffed-out stub of XCSR code

[Peter Maydell]

The prep machine has some code which is stubs of accessors
for XCSR registers. This has been disabled via #if 0
since commit b6b8bd1819ff in 2004, and doesn't have any
actual interesting content. It also uses the deprecated
old_mmio accessor functions. Remove it entirely.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/prep.c | 97 +++------------------------------------------------
 1 file changed, 4 insertions(+), 93 deletions(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 3401570d981..b26138e5c47 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -78,94 +78,6 @@ static int ne2000_irq[NE2000_NB_MAX] = { 9, 10, 11, 3, 4, 5 };
 /* ISA IO ports bridge */
 #define PPC_IO_BASE 0x80000000
 
-/* PowerPC control and status registers */
-#if 0 // Not used
-static struct {
-    /* IDs */
-    uint32_t veni_devi;
-    uint32_t revi;
-    /* Control and status */
-    uint32_t gcsr;
-    uint32_t xcfr;
-    uint32_t ct32;
-    uint32_t mcsr;
-    /* General purpose registers */
-    uint32_t gprg[6];
-    /* Exceptions */
-    uint32_t feen;
-    uint32_t fest;
-    uint32_t fema;
-    uint32_t fecl;
-    uint32_t eeen;
-    uint32_t eest;
-    uint32_t eecl;
-    uint32_t eeint;
-    uint32_t eemck0;
-    uint32_t eemck1;
-    /* Error diagnostic */
-} XCSR;
-
-static void PPC_XCSR_writeb (void *opaque,
-                             hwaddr addr, uint32_t value)
-{
-    printf("%s: 0x" TARGET_FMT_plx " => 0x%08" PRIx32 "\n", __func__, addr,
-           value);
-}
-
-static void PPC_XCSR_writew (void *opaque,
-                             hwaddr addr, uint32_t value)
-{
-    printf("%s: 0x" TARGET_FMT_plx " => 0x%08" PRIx32 "\n", __func__, addr,
-           value);
-}
-
-static void PPC_XCSR_writel (void *opaque,
-                             hwaddr addr, uint32_t value)
-{
-    printf("%s: 0x" TARGET_FMT_plx " => 0x%08" PRIx32 "\n", __func__, addr,
-           value);
-}
-
-static uint32_t PPC_XCSR_readb (void *opaque, hwaddr addr)
-{
-    uint32_t retval = 0;
-
-    printf("%s: 0x" TARGET_FMT_plx " <= %08" PRIx32 "\n", __func__, addr,
-           retval);
-
-    return retval;
-}
-
-static uint32_t PPC_XCSR_readw (void *opaque, hwaddr addr)
-{
-    uint32_t retval = 0;
-
-    printf("%s: 0x" TARGET_FMT_plx " <= %08" PRIx32 "\n", __func__, addr,
-           retval);
-
-    return retval;
-}
-
-static uint32_t PPC_XCSR_readl (void *opaque, hwaddr addr)
-{
-    uint32_t retval = 0;
-
-    printf("%s: 0x" TARGET_FMT_plx " <= %08" PRIx32 "\n", __func__, addr,
-           retval);
-
-    return retval;
-}
-
-static const MemoryRegionOps PPC_XCSR_ops = {
-    .old_mmio = {
-        .read = { PPC_XCSR_readb, PPC_XCSR_readw, PPC_XCSR_readl, },
-        .write = { PPC_XCSR_writeb, PPC_XCSR_writew, PPC_XCSR_writel, },
-    },
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-#endif
-
 /* Fake super-io ports for PREP platform (Intel 82378ZB) */
 typedef struct sysctrl_t {
     qemu_irq reset_irq;
@@ -648,11 +560,10 @@ static void ppc_prep_init(MachineState *machine)
     portio_list_init(&prep_port_list, NULL, prep_portio_list, sysctrl, "prep");
     portio_list_add(&prep_port_list, isa_address_space_io(isa), 0x0);
 
-    /* PowerPC control and status register group */
-#if 0
-    memory_region_init_io(xcsr, NULL, &PPC_XCSR_ops, NULL, "ppc-xcsr", 0x1000);
-    memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
-#endif
+    /*
+     * PowerPC control and status register group: unimplemented,
+     * would be at address 0xFEFF0000.
+     */
 
     if (machine_usb(machine)) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/3] hw/ppc/prep: Remove ifdeffed-out stub of XCSR code

[Philippe Mathieu-Daudé]

On 08/02/2018 11:44 AM, Peter Maydell wrote:
> The prep machine has some code which is stubs of accessors
> for XCSR registers. This has been disabled via #if 0
> since commit b6b8bd1819ff in 2004, and doesn't have any
> actual interesting content. It also uses the deprecated
> old_mmio accessor functions. Remove it entirely.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/ppc/prep.c | 97 +++------------------------------------------------
>  1 file changed, 4 insertions(+), 93 deletions(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 3401570d981..b26138e5c47 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -78,94 +78,6 @@ static int ne2000_irq[NE2000_NB_MAX] = { 9, 10, 11, 3, 4, 5 };
>  /* ISA IO ports bridge */
>  #define PPC_IO_BASE 0x80000000
>  
> -/* PowerPC control and status registers */
> -#if 0 // Not used
> -static struct {
> -    /* IDs */
> -    uint32_t veni_devi;
> -    uint32_t revi;
> -    /* Control and status */
> -    uint32_t gcsr;
> -    uint32_t xcfr;
> -    uint32_t ct32;
> -    uint32_t mcsr;
> -    /* General purpose registers */
> -    uint32_t gprg[6];
> -    /* Exceptions */
> -    uint32_t feen;
> -    uint32_t fest;
> -    uint32_t fema;
> -    uint32_t fecl;
> -    uint32_t eeen;
> -    uint32_t eest;
> -    uint32_t eecl;
> -    uint32_t eeint;
> -    uint32_t eemck0;
> -    uint32_t eemck1;
> -    /* Error diagnostic */
> -} XCSR;
> -
> -static void PPC_XCSR_writeb (void *opaque,
> -                             hwaddr addr, uint32_t value)
> -{
> -    printf("%s: 0x" TARGET_FMT_plx " => 0x%08" PRIx32 "\n", __func__, addr,
> -           value);
> -}
> -
> -static void PPC_XCSR_writew (void *opaque,
> -                             hwaddr addr, uint32_t value)
> -{
> -    printf("%s: 0x" TARGET_FMT_plx " => 0x%08" PRIx32 "\n", __func__, addr,
> -           value);
> -}
> -
> -static void PPC_XCSR_writel (void *opaque,
> -                             hwaddr addr, uint32_t value)
> -{
> -    printf("%s: 0x" TARGET_FMT_plx " => 0x%08" PRIx32 "\n", __func__, addr,
> -           value);
> -}
> -
> -static uint32_t PPC_XCSR_readb (void *opaque, hwaddr addr)
> -{
> -    uint32_t retval = 0;
> -
> -    printf("%s: 0x" TARGET_FMT_plx " <= %08" PRIx32 "\n", __func__, addr,
> -           retval);
> -
> -    return retval;
> -}
> -
> -static uint32_t PPC_XCSR_readw (void *opaque, hwaddr addr)
> -{
> -    uint32_t retval = 0;
> -
> -    printf("%s: 0x" TARGET_FMT_plx " <= %08" PRIx32 "\n", __func__, addr,
> -           retval);
> -
> -    return retval;
> -}
> -
> -static uint32_t PPC_XCSR_readl (void *opaque, hwaddr addr)
> -{
> -    uint32_t retval = 0;
> -
> -    printf("%s: 0x" TARGET_FMT_plx " <= %08" PRIx32 "\n", __func__, addr,
> -           retval);
> -
> -    return retval;
> -}
> -
> -static const MemoryRegionOps PPC_XCSR_ops = {
> -    .old_mmio = {
> -        .read = { PPC_XCSR_readb, PPC_XCSR_readw, PPC_XCSR_readl, },
> -        .write = { PPC_XCSR_writeb, PPC_XCSR_writew, PPC_XCSR_writel, },
> -    },
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
> -#endif
> -
>  /* Fake super-io ports for PREP platform (Intel 82378ZB) */
>  typedef struct sysctrl_t {
>      qemu_irq reset_irq;
> @@ -648,11 +560,10 @@ static void ppc_prep_init(MachineState *machine)
>      portio_list_init(&prep_port_list, NULL, prep_portio_list, sysctrl, "prep");
>      portio_list_add(&prep_port_list, isa_address_space_io(isa), 0x0);
>  
> -    /* PowerPC control and status register group */
> -#if 0
> -    memory_region_init_io(xcsr, NULL, &PPC_XCSR_ops, NULL, "ppc-xcsr", 0x1000);
> -    memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
> -#endif
> +    /*
> +     * PowerPC control and status register group: unimplemented,
> +     * would be at address 0xFEFF0000.
> +     */

While not directly use the harmless UnimplementedDevice?

       create_unimplemented_device("ppc-xcsr", 0xfeff0000, 0x1000);

Anyway,
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>  
>      if (machine_usb(machine)) {
>          pci_create_simple(pci_bus, -1, "pci-ohci");
> 

Re: [Qemu-devel] [PATCH 1/3] hw/ppc/prep: Remove ifdeffed-out stub of XCSR code

[Peter Maydell]

On 2 August 2018 at 16:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> While not directly use the harmless UnimplementedDevice?
>
>        create_unimplemented_device("ppc-xcsr", 0xfeff0000, 0x1000);

I preferred not to change the current behaviour for this
API conversion. If the PPC/prep maintainers would like to
use unimplemented-device they can easily do so as a a
different patch...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] hw/ppc/prep: Remove ifdeffed-out stub of XCSR code

[Philippe Mathieu-Daudé]

On 08/02/2018 12:54 PM, Peter Maydell wrote:
> On 2 August 2018 at 16:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> While not directly use the harmless UnimplementedDevice?
>>
>>        create_unimplemented_device("ppc-xcsr", 0xfeff0000, 0x1000);
> 
> I preferred not to change the current behaviour for this
> API conversion. If the PPC/prep maintainers would like to
> use unimplemented-device they can easily do so as a a
> different patch...

Now I remember there is a behaviour change in using this device:
currently an access to this address space triggers
cpu::do_transaction_failed();
using UnimplementedDevice doesn't.

So better to do this change in a different patch indeed :)

Re: [Qemu-devel] [PATCH 1/3] hw/ppc/prep: Remove ifdeffed-out stub of XCSR code

[Hervé Poussineau]

Le 02/08/2018 à 16:44, Peter Maydell a écrit :
> The prep machine has some code which is stubs of accessors
> for XCSR registers. This has been disabled via #if 0
> since commit b6b8bd1819ff in 2004, and doesn't have any
> actual interesting content. It also uses the deprecated
> old_mmio accessor functions. Remove it entirely.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/ppc/prep.c | 97 +++------------------------------------------------
>   1 file changed, 4 insertions(+), 93 deletions(-)
> 

Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>

[Qemu-devel] [PATCH 2/3] hw/ppc/ppc_boards: Don't use old_mmio for ref405ep_fpga

[Peter Maydell]

Switch the ref405ep_fpga device away from using the old_mmio
MemoryRegion accessors.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/ppc405_boards.c | 60 +++++++-----------------------------------
 1 file changed, 10 insertions(+), 50 deletions(-)

diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index 70111075b33..f5a9c24b6ce 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -66,7 +66,7 @@ struct ref405ep_fpga_t {
     uint8_t reg1;
 };
 
-static uint32_t ref405ep_fpga_readb (void *opaque, hwaddr addr)
+static uint64_t ref405ep_fpga_readb(void *opaque, hwaddr addr, unsigned size)
 {
     ref405ep_fpga_t *fpga;
     uint32_t ret;
@@ -87,8 +87,8 @@ static uint32_t ref405ep_fpga_readb (void *opaque, hwaddr addr)
     return ret;
 }
 
-static void ref405ep_fpga_writeb (void *opaque,
-                                  hwaddr addr, uint32_t value)
+static void ref405ep_fpga_writeb(void *opaque, hwaddr addr, uint64_t value,
+                                 unsigned size)
 {
     ref405ep_fpga_t *fpga;
 
@@ -105,54 +105,14 @@ static void ref405ep_fpga_writeb (void *opaque,
     }
 }
 
-static uint32_t ref405ep_fpga_readw (void *opaque, hwaddr addr)
-{
-    uint32_t ret;
-
-    ret = ref405ep_fpga_readb(opaque, addr) << 8;
-    ret |= ref405ep_fpga_readb(opaque, addr + 1);
-
-    return ret;
-}
-
-static void ref405ep_fpga_writew (void *opaque,
-                                  hwaddr addr, uint32_t value)
-{
-    ref405ep_fpga_writeb(opaque, addr, (value >> 8) & 0xFF);
-    ref405ep_fpga_writeb(opaque, addr + 1, value & 0xFF);
-}
-
-static uint32_t ref405ep_fpga_readl (void *opaque, hwaddr addr)
-{
-    uint32_t ret;
-
-    ret = ref405ep_fpga_readb(opaque, addr) << 24;
-    ret |= ref405ep_fpga_readb(opaque, addr + 1) << 16;
-    ret |= ref405ep_fpga_readb(opaque, addr + 2) << 8;
-    ret |= ref405ep_fpga_readb(opaque, addr + 3);
-
-    return ret;
-}
-
-static void ref405ep_fpga_writel (void *opaque,
-                                  hwaddr addr, uint32_t value)
-{
-    ref405ep_fpga_writeb(opaque, addr, (value >> 24) & 0xFF);
-    ref405ep_fpga_writeb(opaque, addr + 1, (value >> 16) & 0xFF);
-    ref405ep_fpga_writeb(opaque, addr + 2, (value >> 8) & 0xFF);
-    ref405ep_fpga_writeb(opaque, addr + 3, value & 0xFF);
-}
-
 static const MemoryRegionOps ref405ep_fpga_ops = {
-    .old_mmio = {
-        .read = {
-            ref405ep_fpga_readb, ref405ep_fpga_readw, ref405ep_fpga_readl,
-        },
-        .write = {
-            ref405ep_fpga_writeb, ref405ep_fpga_writew, ref405ep_fpga_writel,
-        },
-    },
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .read = ref405ep_fpga_readb,
+    .write = ref405ep_fpga_writeb,
+    .impl.min_access_size = 1,
+    .impl.max_access_size = 1,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_BIG_ENDIAN,
 };
 
 static void ref405ep_fpga_reset (void *opaque)
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/3] hw/ppc/ppc_boards: Don't use old_mmio for ref405ep_fpga

[Philippe Mathieu-Daudé]

Hi Peter,

On 08/02/2018 11:44 AM, Peter Maydell wrote:
> Switch the ref405ep_fpga device away from using the old_mmio
> MemoryRegion accessors.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/ppc/ppc405_boards.c | 60 +++++++-----------------------------------
>  1 file changed, 10 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 70111075b33..f5a9c24b6ce 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -66,7 +66,7 @@ struct ref405ep_fpga_t {
>      uint8_t reg1;
>  };
>  
> -static uint32_t ref405ep_fpga_readb (void *opaque, hwaddr addr)
> +static uint64_t ref405ep_fpga_readb(void *opaque, hwaddr addr, unsigned size)
>  {
>      ref405ep_fpga_t *fpga;
>      uint32_t ret;
> @@ -87,8 +87,8 @@ static uint32_t ref405ep_fpga_readb (void *opaque, hwaddr addr)
>      return ret;
>  }
>  
> -static void ref405ep_fpga_writeb (void *opaque,
> -                                  hwaddr addr, uint32_t value)
> +static void ref405ep_fpga_writeb(void *opaque, hwaddr addr, uint64_t value,
> +                                 unsigned size)
>  {
>      ref405ep_fpga_t *fpga;
>  
> @@ -105,54 +105,14 @@ static void ref405ep_fpga_writeb (void *opaque,
>      }
>  }
>  
> -static uint32_t ref405ep_fpga_readw (void *opaque, hwaddr addr)
> -{
> -    uint32_t ret;
> -
> -    ret = ref405ep_fpga_readb(opaque, addr) << 8;
> -    ret |= ref405ep_fpga_readb(opaque, addr + 1);
> -
> -    return ret;
> -}
> -
> -static void ref405ep_fpga_writew (void *opaque,
> -                                  hwaddr addr, uint32_t value)
> -{
> -    ref405ep_fpga_writeb(opaque, addr, (value >> 8) & 0xFF);
> -    ref405ep_fpga_writeb(opaque, addr + 1, value & 0xFF);
> -}
> -
> -static uint32_t ref405ep_fpga_readl (void *opaque, hwaddr addr)
> -{
> -    uint32_t ret;
> -
> -    ret = ref405ep_fpga_readb(opaque, addr) << 24;
> -    ret |= ref405ep_fpga_readb(opaque, addr + 1) << 16;
> -    ret |= ref405ep_fpga_readb(opaque, addr + 2) << 8;
> -    ret |= ref405ep_fpga_readb(opaque, addr + 3);
> -
> -    return ret;
> -}
> -
> -static void ref405ep_fpga_writel (void *opaque,
> -                                  hwaddr addr, uint32_t value)
> -{
> -    ref405ep_fpga_writeb(opaque, addr, (value >> 24) & 0xFF);
> -    ref405ep_fpga_writeb(opaque, addr + 1, (value >> 16) & 0xFF);
> -    ref405ep_fpga_writeb(opaque, addr + 2, (value >> 8) & 0xFF);
> -    ref405ep_fpga_writeb(opaque, addr + 3, value & 0xFF);
> -}
> -
>  static const MemoryRegionOps ref405ep_fpga_ops = {
> -    .old_mmio = {
> -        .read = {
> -            ref405ep_fpga_readb, ref405ep_fpga_readw, ref405ep_fpga_readl,
> -        },
> -        .write = {
> -            ref405ep_fpga_writeb, ref405ep_fpga_writew, ref405ep_fpga_writel,
> -        },
> -    },
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .read = ref405ep_fpga_readb,
> +    .write = ref405ep_fpga_writeb,
> +    .impl.min_access_size = 1,
> +    .impl.max_access_size = 1,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_BIG_ENDIAN,

Hopefully this is a good case to show the bug I'm having with
access_with_adjusted_size().

I agree with your change, so:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

However IMO little endian guest access is likely to fail.

The bug I'm having looks like, we have BE data is 'aabbccdd', I expect
16-bit access @2 return 'ccdd' but returns 'bbaa' (IIRC).

I used those cripple tests:
https://github.com/philmd/qemu/commit/671ce501a5301849a91384e6ba6f2f3affabcd0d#diff-da1e7a2e0582a05aa232a4baf37f4572

I'll try go get some free time to resurrect/rebase this branch.

Regards,

Phil.

>  };
>  
>  static void ref405ep_fpga_reset (void *opaque)
> 

Re: [Qemu-devel] [PATCH 2/3] hw/ppc/ppc_boards: Don't use old_mmio for ref405ep_fpga

[Peter Maydell]

On 2 August 2018 at 16:58, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hopefully this is a good case to show the bug I'm having with
> access_with_adjusted_size().
>
> I agree with your change, so:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> However IMO little endian guest access is likely to fail.
>
> The bug I'm having looks like, we have BE data is 'aabbccdd', I expect
> 16-bit access @2 return 'ccdd' but returns 'bbaa' (IIRC).

Behaviour here is going to depend on (a) what the TARGET_ENDIANNESS
setting is for the system (b) whether the device is DEVICE_NATIVE_ENDIAN,
DEVICE_BIG_ENDIAN or DEVICE_LITTLE_ENDIAN (c) whether the guest
CPU is in "little endian" or "big endian" mode (if the guest CPU
architecture is bi-endian). I would not be surprised if device
models which were only ever expected to work with (say) big endian
MIPS didn't behave correctly when run with a little endian
MIPS OS, but that's usually an error in the device model and/or
its choice of .endianness in the memory region ops struct.

If there's something wrong with access_with_adjusted_size(),
I would suggest starting a different thread for that. I don't
think these changes should alter the behaviour of this device.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/ppc/ppc_boards: Don't use old_mmio for ref405ep_fpga

[Philippe Mathieu-Daudé]

On 08/02/2018 01:40 PM, Peter Maydell wrote:
> On 2 August 2018 at 16:58, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hopefully this is a good case to show the bug I'm having with
>> access_with_adjusted_size().
>>
>> I agree with your change, so:
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> However IMO little endian guest access is likely to fail.
>>
>> The bug I'm having looks like, we have BE data is 'aabbccdd', I expect
>> 16-bit access @2 return 'ccdd' but returns 'bbaa' (IIRC).
> 
> Behaviour here is going to depend on (a) what the TARGET_ENDIANNESS
> setting is for the system (b) whether the device is DEVICE_NATIVE_ENDIAN,
> DEVICE_BIG_ENDIAN or DEVICE_LITTLE_ENDIAN (c) whether the guest
> CPU is in "little endian" or "big endian" mode (if the guest CPU
> architecture is bi-endian). I would not be surprised if device
> models which were only ever expected to work with (say) big endian
> MIPS didn't behave correctly when run with a little endian
> MIPS OS, but that's usually an error in the device model and/or
> its choice of .endianness in the memory region ops struct.
> 
> If there's something wrong with access_with_adjusted_size(),
> I would suggest starting a different thread for that. I don't
> think these changes should alter the behaviour of this device.

Sure, I'll do when I continue to work on this.

I started my mail with "Hi Peter" but the access_with_adjusted_size()
comments were directed to the PPC reviewers, I'll reword to be more
explicit.

PPC reviewers: watch out I'm hitting an issue on MIPS boards when using
bi-endian cpu in little-endian configuration, and accessing big-endian
ordered devices (usually when .valid access size is bigger than device
.impl). I don't know how to test this with PPC images.
This patch as it looks correct to me, but since now
access_with_adjusted_size() is involved, it might trigger the previous
described issue.

Regards,

Phil.

[Qemu-devel] [PATCH 3/3] hw/ppc/ppc405_uc: Convert away from old_mmio

[Peter Maydell]

Convert the devices in ppc405_uc away from using the old_mmio
MemoryRegion accessors:

 * opba's 32-bit and 16-bit accessors were just calling the
   8-bit accessors and assembling a big-endian order number,
   which we can do by setting the .impl.max_access_size to 1
   and the endianness to DEVICE_BIG_ENDIAN, and letting the
   core memory code do the assembly
 * ppc405_gpio's accessors were all just stubs
 * ppc4xx_gpt's 8-bit and 16-bit accessors were treating the
   access as invalid, which we can do by setting the
   .valid.min_access_size and .valid.max_access_size fields

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/ppc405_uc.c | 173 +++++++--------------------------------------
 1 file changed, 25 insertions(+), 148 deletions(-)

diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
index 4bd9fbcc1ef..5c58415cf1f 100644
--- a/hw/ppc/ppc405_uc.c
+++ b/hw/ppc/ppc405_uc.c
@@ -283,7 +283,7 @@ struct ppc4xx_opba_t {
     uint8_t pr;
 };
 
-static uint32_t opba_readb (void *opaque, hwaddr addr)
+static uint64_t opba_readb(void *opaque, hwaddr addr, unsigned size)
 {
     ppc4xx_opba_t *opba;
     uint32_t ret;
@@ -307,8 +307,8 @@ static uint32_t opba_readb (void *opaque, hwaddr addr)
     return ret;
 }
 
-static void opba_writeb (void *opaque,
-                         hwaddr addr, uint32_t value)
+static void opba_writeb(void *opaque, hwaddr addr, uint64_t value,
+                        unsigned size)
 {
     ppc4xx_opba_t *opba;
 
@@ -328,61 +328,14 @@ static void opba_writeb (void *opaque,
         break;
     }
 }
-
-static uint32_t opba_readw (void *opaque, hwaddr addr)
-{
-    uint32_t ret;
-
-#ifdef DEBUG_OPBA
-    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-    ret = opba_readb(opaque, addr) << 8;
-    ret |= opba_readb(opaque, addr + 1);
-
-    return ret;
-}
-
-static void opba_writew (void *opaque,
-                         hwaddr addr, uint32_t value)
-{
-#ifdef DEBUG_OPBA
-    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
-           value);
-#endif
-    opba_writeb(opaque, addr, value >> 8);
-    opba_writeb(opaque, addr + 1, value);
-}
-
-static uint32_t opba_readl (void *opaque, hwaddr addr)
-{
-    uint32_t ret;
-
-#ifdef DEBUG_OPBA
-    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-    ret = opba_readb(opaque, addr) << 24;
-    ret |= opba_readb(opaque, addr + 1) << 16;
-
-    return ret;
-}
-
-static void opba_writel (void *opaque,
-                         hwaddr addr, uint32_t value)
-{
-#ifdef DEBUG_OPBA
-    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
-           value);
-#endif
-    opba_writeb(opaque, addr, value >> 24);
-    opba_writeb(opaque, addr + 1, value >> 16);
-}
-
 static const MemoryRegionOps opba_ops = {
-    .old_mmio = {
-        .read = { opba_readb, opba_readw, opba_readl, },
-        .write = { opba_writeb, opba_writew, opba_writel, },
-    },
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .read = opba_readb,
+    .write = opba_writeb,
+    .impl.min_access_size = 1,
+    .impl.max_access_size = 1,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_BIG_ENDIAN,
 };
 
 static void ppc4xx_opba_reset (void *opaque)
@@ -750,65 +703,27 @@ struct ppc405_gpio_t {
     uint32_t isr1l;
 };
 
-static uint32_t ppc405_gpio_readb (void *opaque, hwaddr addr)
+static uint64_t ppc405_gpio_read(void *opaque, hwaddr addr, unsigned size)
 {
 #ifdef DEBUG_GPIO
-    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
+    printf("%s: addr " TARGET_FMT_plx " size %d\n", __func__, addr, size);
 #endif
 
     return 0;
 }
 
-static void ppc405_gpio_writeb (void *opaque,
-                                hwaddr addr, uint32_t value)
+static void ppc405_gpio_write(void *opaque, hwaddr addr, uint64_t value,
+                              unsigned size)
 {
 #ifdef DEBUG_GPIO
-    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
-           value);
-#endif
-}
-
-static uint32_t ppc405_gpio_readw (void *opaque, hwaddr addr)
-{
-#ifdef DEBUG_GPIO
-    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-
-    return 0;
-}
-
-static void ppc405_gpio_writew (void *opaque,
-                                hwaddr addr, uint32_t value)
-{
-#ifdef DEBUG_GPIO
-    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
-           value);
-#endif
-}
-
-static uint32_t ppc405_gpio_readl (void *opaque, hwaddr addr)
-{
-#ifdef DEBUG_GPIO
-    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-
-    return 0;
-}
-
-static void ppc405_gpio_writel (void *opaque,
-                                hwaddr addr, uint32_t value)
-{
-#ifdef DEBUG_GPIO
-    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
-           value);
+    printf("%s: addr " TARGET_FMT_plx " size %d val %08" PRIx32 "\n",
+           __func__, addr, size, value);
 #endif
 }
 
 static const MemoryRegionOps ppc405_gpio_ops = {
-    .old_mmio = {
-        .read = { ppc405_gpio_readb, ppc405_gpio_readw, ppc405_gpio_readl, },
-        .write = { ppc405_gpio_writeb, ppc405_gpio_writew, ppc405_gpio_writel, },
-    },
+    .read = ppc405_gpio_read,
+    .write = ppc405_gpio_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
@@ -1017,44 +932,6 @@ struct ppc4xx_gpt_t {
     uint32_t mask[5];
 };
 
-static uint32_t ppc4xx_gpt_readb (void *opaque, hwaddr addr)
-{
-#ifdef DEBUG_GPT
-    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-    /* XXX: generate a bus fault */
-    return -1;
-}
-
-static void ppc4xx_gpt_writeb (void *opaque,
-                               hwaddr addr, uint32_t value)
-{
-#ifdef DEBUG_I2C
-    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
-           value);
-#endif
-    /* XXX: generate a bus fault */
-}
-
-static uint32_t ppc4xx_gpt_readw (void *opaque, hwaddr addr)
-{
-#ifdef DEBUG_GPT
-    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-    /* XXX: generate a bus fault */
-    return -1;
-}
-
-static void ppc4xx_gpt_writew (void *opaque,
-                               hwaddr addr, uint32_t value)
-{
-#ifdef DEBUG_I2C
-    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
-           value);
-#endif
-    /* XXX: generate a bus fault */
-}
-
 static int ppc4xx_gpt_compare (ppc4xx_gpt_t *gpt, int n)
 {
     /* XXX: TODO */
@@ -1107,7 +984,7 @@ static void ppc4xx_gpt_compute_timer (ppc4xx_gpt_t *gpt)
     /* XXX: TODO */
 }
 
-static uint32_t ppc4xx_gpt_readl (void *opaque, hwaddr addr)
+static uint64_t ppc4xx_gpt_read(void *opaque, hwaddr addr, unsigned size)
 {
     ppc4xx_gpt_t *gpt;
     uint32_t ret;
@@ -1162,8 +1039,8 @@ static uint32_t ppc4xx_gpt_readl (void *opaque, hwaddr addr)
     return ret;
 }
 
-static void ppc4xx_gpt_writel (void *opaque,
-                               hwaddr addr, uint32_t value)
+static void ppc4xx_gpt_write(void *opaque, hwaddr addr, uint64_t value,
+                             unsigned size)
 {
     ppc4xx_gpt_t *gpt;
     int idx;
@@ -1225,10 +1102,10 @@ static void ppc4xx_gpt_writel (void *opaque,
 }
 
 static const MemoryRegionOps gpt_ops = {
-    .old_mmio = {
-        .read = { ppc4xx_gpt_readb, ppc4xx_gpt_readw, ppc4xx_gpt_readl, },
-        .write = { ppc4xx_gpt_writeb, ppc4xx_gpt_writew, ppc4xx_gpt_writel, },
-    },
+    .read = ppc4xx_gpt_read,
+    .write = ppc4xx_gpt_write,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH 3/3] hw/ppc/ppc405_uc: Convert away from old_mmio

[Philippe Mathieu-Daudé]

On 08/02/2018 11:44 AM, Peter Maydell wrote:
> Convert the devices in ppc405_uc away from using the old_mmio
> MemoryRegion accessors:
> 
>  * opba's 32-bit and 16-bit accessors were just calling the
>    8-bit accessors and assembling a big-endian order number,
>    which we can do by setting the .impl.max_access_size to 1
>    and the endianness to DEVICE_BIG_ENDIAN, and letting the
>    core memory code do the assembly
>  * ppc405_gpio's accessors were all just stubs
>  * ppc4xx_gpt's 8-bit and 16-bit accessors were treating the
>    access as invalid, which we can do by setting the
>    .valid.min_access_size and .valid.max_access_size fields
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/ppc/ppc405_uc.c | 173 +++++++--------------------------------------
>  1 file changed, 25 insertions(+), 148 deletions(-)
> 
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index 4bd9fbcc1ef..5c58415cf1f 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -283,7 +283,7 @@ struct ppc4xx_opba_t {
>      uint8_t pr;
>  };
>  
> -static uint32_t opba_readb (void *opaque, hwaddr addr)
> +static uint64_t opba_readb(void *opaque, hwaddr addr, unsigned size)
>  {
>      ppc4xx_opba_t *opba;
>      uint32_t ret;
> @@ -307,8 +307,8 @@ static uint32_t opba_readb (void *opaque, hwaddr addr)
>      return ret;
>  }
>  
> -static void opba_writeb (void *opaque,
> -                         hwaddr addr, uint32_t value)
> +static void opba_writeb(void *opaque, hwaddr addr, uint64_t value,
> +                        unsigned size)
>  {
>      ppc4xx_opba_t *opba;
>  
> @@ -328,61 +328,14 @@ static void opba_writeb (void *opaque,
>          break;
>      }
>  }
> -
> -static uint32_t opba_readw (void *opaque, hwaddr addr)
> -{
> -    uint32_t ret;
> -
> -#ifdef DEBUG_OPBA
> -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
> -#endif
> -    ret = opba_readb(opaque, addr) << 8;
> -    ret |= opba_readb(opaque, addr + 1);
> -
> -    return ret;
> -}
> -
> -static void opba_writew (void *opaque,
> -                         hwaddr addr, uint32_t value)
> -{
> -#ifdef DEBUG_OPBA
> -    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
> -           value);
> -#endif
> -    opba_writeb(opaque, addr, value >> 8);
> -    opba_writeb(opaque, addr + 1, value);
> -}
> -
> -static uint32_t opba_readl (void *opaque, hwaddr addr)
> -{
> -    uint32_t ret;
> -
> -#ifdef DEBUG_OPBA
> -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
> -#endif
> -    ret = opba_readb(opaque, addr) << 24;
> -    ret |= opba_readb(opaque, addr + 1) << 16;
> -
> -    return ret;
> -}
> -
> -static void opba_writel (void *opaque,
> -                         hwaddr addr, uint32_t value)
> -{
> -#ifdef DEBUG_OPBA
> -    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
> -           value);
> -#endif
> -    opba_writeb(opaque, addr, value >> 24);
> -    opba_writeb(opaque, addr + 1, value >> 16);
> -}
> -
>  static const MemoryRegionOps opba_ops = {
> -    .old_mmio = {
> -        .read = { opba_readb, opba_readw, opba_readl, },
> -        .write = { opba_writeb, opba_writew, opba_writel, },
> -    },
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .read = opba_readb,
> +    .write = opba_writeb,
> +    .impl.min_access_size = 1,
> +    .impl.max_access_size = 1,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_BIG_ENDIAN,

Except the eventual issue commented in the previous patch,
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>  };
>  
>  static void ppc4xx_opba_reset (void *opaque)
> @@ -750,65 +703,27 @@ struct ppc405_gpio_t {
>      uint32_t isr1l;
>  };
>  
> -static uint32_t ppc405_gpio_readb (void *opaque, hwaddr addr)
> +static uint64_t ppc405_gpio_read(void *opaque, hwaddr addr, unsigned size)
>  {
>  #ifdef DEBUG_GPIO
> -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
> +    printf("%s: addr " TARGET_FMT_plx " size %d\n", __func__, addr, size);
>  #endif
>  
>      return 0;
>  }
>  
> -static void ppc405_gpio_writeb (void *opaque,
> -                                hwaddr addr, uint32_t value)
> +static void ppc405_gpio_write(void *opaque, hwaddr addr, uint64_t value,
> +                              unsigned size)
>  {
>  #ifdef DEBUG_GPIO
> -    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
> -           value);
> -#endif
> -}
> -
> -static uint32_t ppc405_gpio_readw (void *opaque, hwaddr addr)
> -{
> -#ifdef DEBUG_GPIO
> -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
> -#endif
> -
> -    return 0;
> -}
> -
> -static void ppc405_gpio_writew (void *opaque,
> -                                hwaddr addr, uint32_t value)
> -{
> -#ifdef DEBUG_GPIO
> -    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
> -           value);
> -#endif
> -}
> -
> -static uint32_t ppc405_gpio_readl (void *opaque, hwaddr addr)
> -{
> -#ifdef DEBUG_GPIO
> -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
> -#endif
> -
> -    return 0;
> -}
> -
> -static void ppc405_gpio_writel (void *opaque,
> -                                hwaddr addr, uint32_t value)
> -{
> -#ifdef DEBUG_GPIO
> -    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
> -           value);
> +    printf("%s: addr " TARGET_FMT_plx " size %d val %08" PRIx32 "\n",
> +           __func__, addr, size, value);
>  #endif
>  }
>  
>  static const MemoryRegionOps ppc405_gpio_ops = {
> -    .old_mmio = {
> -        .read = { ppc405_gpio_readb, ppc405_gpio_readw, ppc405_gpio_readl, },
> -        .write = { ppc405_gpio_writeb, ppc405_gpio_writew, ppc405_gpio_writel, },
> -    },
> +    .read = ppc405_gpio_read,
> +    .write = ppc405_gpio_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> @@ -1017,44 +932,6 @@ struct ppc4xx_gpt_t {
>      uint32_t mask[5];
>  };
>  
> -static uint32_t ppc4xx_gpt_readb (void *opaque, hwaddr addr)
> -{
> -#ifdef DEBUG_GPT
> -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
> -#endif
> -    /* XXX: generate a bus fault */
> -    return -1;
> -}
> -
> -static void ppc4xx_gpt_writeb (void *opaque,
> -                               hwaddr addr, uint32_t value)
> -{
> -#ifdef DEBUG_I2C
> -    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
> -           value);
> -#endif
> -    /* XXX: generate a bus fault */
> -}
> -
> -static uint32_t ppc4xx_gpt_readw (void *opaque, hwaddr addr)
> -{
> -#ifdef DEBUG_GPT
> -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
> -#endif
> -    /* XXX: generate a bus fault */
> -    return -1;
> -}
> -
> -static void ppc4xx_gpt_writew (void *opaque,
> -                               hwaddr addr, uint32_t value)
> -{
> -#ifdef DEBUG_I2C
> -    printf("%s: addr " TARGET_FMT_plx " val %08" PRIx32 "\n", __func__, addr,
> -           value);
> -#endif
> -    /* XXX: generate a bus fault */
> -}
> -
>  static int ppc4xx_gpt_compare (ppc4xx_gpt_t *gpt, int n)
>  {
>      /* XXX: TODO */
> @@ -1107,7 +984,7 @@ static void ppc4xx_gpt_compute_timer (ppc4xx_gpt_t *gpt)
>      /* XXX: TODO */
>  }
>  
> -static uint32_t ppc4xx_gpt_readl (void *opaque, hwaddr addr)
> +static uint64_t ppc4xx_gpt_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      ppc4xx_gpt_t *gpt;
>      uint32_t ret;
> @@ -1162,8 +1039,8 @@ static uint32_t ppc4xx_gpt_readl (void *opaque, hwaddr addr)
>      return ret;
>  }
>  
> -static void ppc4xx_gpt_writel (void *opaque,
> -                               hwaddr addr, uint32_t value)
> +static void ppc4xx_gpt_write(void *opaque, hwaddr addr, uint64_t value,
> +                             unsigned size)
>  {
>      ppc4xx_gpt_t *gpt;
>      int idx;
> @@ -1225,10 +1102,10 @@ static void ppc4xx_gpt_writel (void *opaque,
>  }
>  
>  static const MemoryRegionOps gpt_ops = {
> -    .old_mmio = {
> -        .read = { ppc4xx_gpt_readb, ppc4xx_gpt_readw, ppc4xx_gpt_readl, },
> -        .write = { ppc4xx_gpt_writeb, ppc4xx_gpt_writew, ppc4xx_gpt_writel, },
> -    },
> +    .read = ppc4xx_gpt_read,
> +    .write = ppc4xx_gpt_write,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> 

Re: [Qemu-devel] [PATCH 0/3] hw/ppc: Convert various devices away from old_mmio

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

On Thu, Aug 02, 2018 at 03:44:27PM +0100, Peter Maydell wrote:
> This patchset removes various uses of old_mmio from minor PPC
> devices:
>  * hw/ppc/prep had an entirely ifdeffed-out stub of an XCSR device,
>    which we remove
>  * hw/ppc/ppc_boards had ref405ep_fpga
>  * hw/ppc/ppc405_uc had three minor devices
> 
> As you can see from the diffstat, the new API provides much
> cleaner ways to handle the various different access sizes.
> 
> This knocks another five old_mmio uses out of the codebase,
> leaving us with just five to go.
> 
> NB: Tested only with 'make check'.

Applied to ppc-for-3.1, thanks.

> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>   hw/ppc/prep: Remove ifdeffed-out stub of XCSR code
>   hw/ppc/ppc_boards: Don't use old_mmio for ref405ep_fpga
>   hw/ppc/ppc405_uc: Convert away from old_mmio
> 
>  hw/ppc/ppc405_boards.c |  60 +++-----------
>  hw/ppc/ppc405_uc.c     | 173 ++++++-----------------------------------
>  hw/ppc/prep.c          |  97 +----------------------
>  3 files changed, 39 insertions(+), 291 deletions(-)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]