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-f4...@amsat.org/
[b]
https://lore.kernel.org/qemu-devel/20210305235414.2358144-1-f4...@amsat.org/
Thoughts?
Thanks,
Phil.