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

[Philippe Mathieu-Daudé]

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.