| Message ID | 20180802144430.13870-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/ppc: Convert various devices away from old_mmio | expand |
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.
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)
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(-) -- 2.17.1