* Philippe Mathieu-Daudé (phi...@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-f4...@amsat.org/
> [b]
> https://lore.kernel.org/qemu-devel/20210305235414.2358144-1-f4...@amsat.org/
>
> Thoughts?
>
> Thanks,
>
> Phil.
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
