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

[Aurelien Jarno]

On 2016-06-18 22:48, Hervé Poussineau wrote:
> Hi Aurélien,
> 
> Le 20/05/2016 à 21:56, Aurelien Jarno a écrit :
> > On 2016-05-20 15:05, Hervé Poussineau wrote:
> > > Incidentally, this fixes YAMON on big endian guest.
> > > 
> > > Signed-off-by: Hervé Poussineau <address@hidden>
> > > ---
> > >  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.
> 
> The real word problem (ie the one from Yamon) is:
> In LE Yamon, there is a read a 0x4d1 (len = 1). MWORDSWAP and MBYTESWAP are 
> disabled
> In BE Yamon, the same read is at address 0x4d2. MWORDSWAP is enabled while 
> 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?

I think indeed that it should be ignored.

> 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.

If you talk about byte accesses, it really looks like a simple byteswap.
0x4d2 ^ 3 = 0x4d1 and 0x72 ^ 3 = 0x71. This could be there is a byteswap
that is missing somewhere. It would be interesting to see how 16-bit and
32-bit accesses are changed between big and little endian.

Aurelien

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