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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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?

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.