Re: [Qemu-devel] [PATCH] gt64xxx: access right I/O port when activating byte swapping

[Aurelien Jarno <aurelien@aurel32.net>]

On 2016-05-20 15:05, Hervé Poussineau wrote:
> Incidentally, this fixes YAMON on big endian guest.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/mips/gt64xxx_pci.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 3f4523d..c76ee88 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -177,6 +177,7 @@
>  
>  /* PCI Internal */
>  #define GT_PCI0_CMD    		(0xc00 >> 2)
> +#define   GT_CMD_MWORDSWAP      (1 << 10)
>  #define GT_PCI0_TOR    		(0xc04 >> 2)
>  #define GT_PCI0_BS_SCS10    	(0xc08 >> 2)
>  #define GT_PCI0_BS_SCS32    	(0xc0c >> 2)
> @@ -294,6 +295,62 @@ static void gt64120_isd_mapping(GT64120State *s)
>      memory_region_add_subregion(get_system_memory(), s->ISD_start, &s->ISD_mem);
>  }
>  
> +static uint64_t gt64120_pci_io_read(void *opaque, hwaddr addr,
> +                                    unsigned int size)
> +{
> +    GT64120State *s = opaque;
> +    uint8_t buf[4];
> +
> +    if (s->regs[GT_PCI0_CMD] & GT_CMD_MWORDSWAP) {

First of all, it should be noted that this bit doesn't control byte
swapping, but swaps the 2 4-byte words in a 8-byte word.

> +        addr = (addr & ~3) + 4 - size - (addr & 3);

This looks complicated, and I don't think it is correct. In addition
this doesn't behave correctly at the edges of the address space. For
example a 2 byte access at address 0x3 would access address
0xffffffffffffffff.

For sizes <= 4, swapping the 2 words should be done with addr ^= 4.
Maybe you should also check for MBYTESWAP which also swaps the bytes
within a 8-byte word.

> +    }
> +
> +    address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
> +                       buf, size);
> +
> +    if (size == 1) {
> +        return buf[0];
> +    } else if (size == 2) {
> +        return lduw_le_p(buf);
> +    } else if (size == 4) {
> +        return ldl_le_p(buf);
> +    } else {
> +        g_assert_not_reached();
> +    }

The device is configured is little endian, and then the little endian
value converted into native endianness. Wouldn't it be simple to declare
it as DEVICE_NATIVE_ENDIAN?

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net