Re: Thoughts on removing the TARGET_I386 part of hw/display/vga/vbe_portio_list[]

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@linaro.org) wrote:
> Hi,
> 
> I'm trying to understand the x86 architecture-specific code in
> hw/display/vga.c:
> 
>     const MemoryRegionPortio vbe_portio_list[] = {
>         { 0, 1, 2, .read = vbe_ioport_read_index,
>                    .write = vbe_ioport_write_index },
>     # ifdef TARGET_I386
>         { 1, 1, 2, .read = vbe_ioport_read_data,
>                    .write = vbe_ioport_write_data },
>     # endif
>         { 2, 1, 2, .read = vbe_ioport_read_data,
>                    .write = vbe_ioport_write_data },
>         PORTIO_END_OF_LIST(),
>     };
> 
> Having:
> 
>     typedef struct MemoryRegionPortio {
>         uint32_t offset;
>         uint32_t len;
>         unsigned size;
>         uint32_t (*read)(...);
>         void (*write)(...);
>         ...
>     } MemoryRegionPortio;
> 
> So on x86 we can have 16-bit I/O accesses unaligned to 8-bit boundary?

Yes, like most things in x86 the requirement for alignment is a 'should'
followed by a description of what might happen if you don't:

From intel arch manual 19.3:
 '..16-bit ports should be aligned to even addresses (0, 2, 4, ...) so that all 
16 bits can be transferred in a
  single bus cycle. Likewise, 32-bit ports should be aligned to addresses that 
are multiples of four (0, 4, 8, ...). The
  processor supports data transfers to unaligned ports, but there is a 
performance penalty because one or more
  extra bus cycle must be used.'

I think I've even seen it suggested that a 32bit access to ffff might be
defined - although I'm not sure if that's legal.

I don't know that bit of qemu well enough to know whether the cpu part
of qemu should be splitting the unaligned accesses or not.

Dave


> Looking at git-blame we have:
> 
> [1] 0a039dc700 ("vga: Convert to isa_register_portio_list")
> [2] 09a79b4974 ("partial big endian fixes - change VESA VBE ports for non
> i386 targets to avoid unaligned accesses")
> [3] 4fa0f5d292 ("added bochs VBE support")
> 
> 
> [3] added:
> 
>   #ifdef CONFIG_BOCHS_VBE
>      s->vbe_regs[VBE_DISPI_INDEX_ID] = VBE_DISPI_ID0;
>      register_ioport_read(0x1ce, 1, vbe_ioport_read, 2);
>      register_ioport_read(0x1cf, 1, vbe_ioport_read, 2);
> 
>      register_ioport_write(0x1ce, 1, vbe_ioport_write, 2);
>      register_ioport_write(0x1cf, 1, vbe_ioport_write, 2);
>   #endif
> 
> Back then, register_ioport_read() was:
> 
>   /* size is the word size in byte */
>   int register_ioport_read(int start, int length,
>                            IOPortReadFunc *func, int size)
>   {
>     int i, bsize;
> 
>     if (size == 1)
>         bsize = 0;
>     else if (size == 2)
>         bsize = 1;
>     else if (size == 4)
>         bsize = 2;
>     else
>         return -1;
>     for(i = start; i < start + length; i += size)
>         ioport_read_table[bsize][i] = func;
>     return 0;
>   }
> 
> Indeed registering a 16-bit handler at the 8-bit aligned 0x1cf I/O address.
> 
> I wonder if this wasn't a typo, and we wanted to register two 8-bit
> VBE handlers at offsets +0 and +1. IOW the code would have been:
> 
>   #ifdef CONFIG_BOCHS_VBE
>      s->vbe_regs[VBE_DISPI_INDEX_ID] = VBE_DISPI_ID0;
>      register_ioport_read(0x1ce, 1, vbe_ioport_read, 2);
>      register_ioport_read(0x1ce, 2, vbe_ioport_read, 1);
> 
>      register_ioport_write(0x1ce, 1, vbe_ioport_write, 2);
>      register_ioport_write(0x1ce, 2, vbe_ioport_write, 1);
>   #endif
> 
> Because in that case, along with the code added in commit [2]:
> 
>  static uint32_t vga_mem_readw(target_phys_addr_t addr)
>  {
>      uint32_t v;
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    v = vga_mem_readb(addr) << 8;
> +    v |= vga_mem_readb(addr + 1);
> +#else
>      v = vga_mem_readb(addr);
>      v |= vga_mem_readb(addr + 1) << 8;
> +#endif
>      return v;
>  }
> 
> The 'ifdef TARGET_I386' (still from [2], converted in [1])
> wouldn't have been necessary.
> 
> So I _think_ today we should be good with removing the x86 line:
> 
> -- >8 --
>  static const MemoryRegionPortio vbe_portio_list[] = {
>      { 0, 1, 2, .read = vbe_ioport_read_index, .write =
> vbe_ioport_write_index },
> -# ifdef TARGET_I386
> -    { 1, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data
> },
> -# endif
>      { 2, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data
> },
>      PORTIO_END_OF_LIST(),
>  };
> ---
> 
> *Except* if there is some hidden magic logic on the ISA bus...
> Not per the ISA spec, but manufacturer/hardware specific.
> 
> I.e. the Jazz machines use a RC4030 which bridge ISA to the main
> bus, and transparently handles misaligned CPU/DMA accesses to the
> ISA address space.
> 
> This ISA topic was already mentioned before, see:
> 
> [a]
> https://lore.kernel.org/qemu-devel/20200720185758.21280-1-f4bug@amsat.org/
> [b]
> https://lore.kernel.org/qemu-devel/20210305235414.2358144-1-f4bug@amsat.org/
> 
> Thoughts?
> 
> Thanks,
> 
> Phil.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK