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
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)
>
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
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.
© 2016 - 2025 Red Hat, Inc.