From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56083) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bENAj-0003R9-Rj for qemu-devel@nongnu.org; Sat, 18 Jun 2016 16:48:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bENAf-0003u2-My for qemu-devel@nongnu.org; Sat, 18 Jun 2016 16:48:52 -0400 Received: from smtp5-g21.free.fr ([212.27.42.5]:61258) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bENAf-0003t1-Cq for qemu-devel@nongnu.org; Sat, 18 Jun 2016 16:48:49 -0400 References: <1463749503-27374-1-git-send-email-hpoussin@reactos.org> <20160520195604.GA19765@aurel32.net> From: =?UTF-8?Q?Herv=c3=a9_Poussineau?= Message-ID: <12a1396e-6454-cda7-675d-163748e5a3d0@reactos.org> Date: Sat, 18 Jun 2016 22:48:34 +0200 MIME-Version: 1.0 In-Reply-To: <20160520195604.GA19765@aurel32.net> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] gt64xxx: access right I/O port when activating byte swapping List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aurelien Jarno Cc: qemu-devel@nongnu.org, Leon Alrae Hi Aur=E9lien, Le 20/05/2016 =E0 21:56, Aurelien Jarno a =E9crit : > On 2016-05-20 15:05, Herv=E9 Poussineau wrote: >> Incidentally, this fixes YAMON on big endian guest. >> >> Signed-off-by: Herv=E9 Poussineau >> --- >> 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 =3D 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 =3D (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 <=3D 4, swapping the 2 words should be done with addr ^=3D 4. > Maybe you should also check for MBYTESWAP which also swaps the bytes > within a 8-byte word. The real word problem (ie the one from Yamon) is: In LE Yamon, there is a read a 0x4d1 (len =3D 1). MWORDSWAP and MBYTESWAP= are disabled In BE Yamon, the same read is at address 0x4d2. MWORDSWAP is enabled whil= e MBYTESWAP is disabled. MWORDSWAP documentation is: "The GT-64120 PCI master swaps the words of the incoming and outgoing PCI= data (swap the 2 words of a long word)" Do we have to ignore it, as QEMU only handles 4-bytes accesses? Then, how to change this address 0x4d2 to 0x4d1, address where is located= the i8259 ELCR register? Next accesses are for the RTC, at address 0x72 in BE and address 0x71 in = LE. I think I'm missing something. > >> + } >> + >> + address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIE= D, >> + buf, size); >> + >> + if (size =3D=3D 1) { >> + return buf[0]; >> + } else if (size =3D=3D 2) { >> + return lduw_le_p(buf); >> + } else if (size =3D=3D 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 declar= e > it as DEVICE_NATIVE_ENDIAN? OK for this part, DEVICE_NATIVE_ENDIAN is indeed better and simplifies co= de. Herv=E9