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

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.

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

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

[Philippe Mathieu-Daudé]

On 6/12/22 13:30, Dr. David Alan Gilbert wrote:
> * 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.'

So you confirm this is a architecture behavior, not a device one, thanks.

> 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.

Easy to test :) If unspecified and there is some ISA-to-XXX bridge, then 
I expect this to be implementation dependent of the bridge.

> 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.
All I/O accesses are gated thru access_with_adjusted_size() in 
softmmu/memory.c.

There is an old access_with_adjusted_size_unaligned() version [1] from
Andrew and a more recent series [2] from Richard. Maybe the latter fixes
some long-standing bug [3] we have here?

[1] 
https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/
[2] 
https://lore.kernel.org/qemu-devel/20210619172626.875885-15-richard.henderson@linaro.org/
[3] 
https://lore.kernel.org/qemu-devel/CAFEAcA-fMUrwnpu90Qf1LWGsQ36M-PmX2uC1+kenT__otLxjTg@mail.gmail.com/

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

[Peter Maydell]

On Tue, 6 Dec 2022 at 15:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 6/12/22 13:30, Dr. David Alan Gilbert wrote:
> > 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.
> All I/O accesses are gated thru access_with_adjusted_size() in
> softmmu/memory.c.
>
> There is an old access_with_adjusted_size_unaligned() version [1] from
> Andrew and a more recent series [2] from Richard. Maybe the latter fixes
> some long-standing bug [3] we have here?

There definitely are some unaddressed bugs there -- maybe this
is the time to work through what semantics we want that
softmmu code to provide and fix the bugs...

-- PMM

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

[Richard Henderson]

On 12/6/22 10:02, Peter Maydell wrote:
> On Tue, 6 Dec 2022 at 15:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 6/12/22 13:30, Dr. David Alan Gilbert wrote:
>>> 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.
>> All I/O accesses are gated thru access_with_adjusted_size() in
>> softmmu/memory.c.
>>
>> There is an old access_with_adjusted_size_unaligned() version [1] from
>> Andrew and a more recent series [2] from Richard. Maybe the latter fixes
>> some long-standing bug [3] we have here?
> 
> There definitely are some unaddressed bugs there -- maybe this
> is the time to work through what semantics we want that
> softmmu code to provide and fix the bugs...

Yes, indeed.  Let's not forget Mark C-A's m68k bug[1] which so far has no resolution.


r~

[1] https://gitlab.com/qemu-project/qemu/-/issues/360

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

[Mark Cave-Ayland]

On 06/12/2022 16:23, Richard Henderson wrote:

> On 12/6/22 10:02, Peter Maydell wrote:
>> On Tue, 6 Dec 2022 at 15:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>
>>> On 6/12/22 13:30, Dr. David Alan Gilbert wrote:
>>>> 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.
>>> All I/O accesses are gated thru access_with_adjusted_size() in
>>> softmmu/memory.c.
>>>
>>> There is an old access_with_adjusted_size_unaligned() version [1] from
>>> Andrew and a more recent series [2] from Richard. Maybe the latter fixes
>>> some long-standing bug [3] we have here?
>>
>> There definitely are some unaddressed bugs there -- maybe this
>> is the time to work through what semantics we want that
>> softmmu code to provide and fix the bugs...
> 
> Yes, indeed.  Let's not forget Mark C-A's m68k bug[1] which so far has no resolution.
> 
> r~
> 
> [1] https://gitlab.com/qemu-project/qemu/-/issues/360

That would definitely be useful: since Richard worked on this series, I managed to 
develop a hack that allows me to work around the issue for my particular use-case 
which is why I haven't been focusing on this.

The main concerns are listed in the above issue at 
https://gitlab.com/qemu-project/qemu/-/issues/360#note_597130838. Defining the 
behaviour doesn't seem too bad, but it is likely some things that unintentionally 
depend upon the existing behaviour will break.


ATB,

Mark.

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

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

On Tue, Dec 6, 2022 at 5:32 AM Dr. David Alan Gilbert <dgilbert@redhat.com>
wrote:

> 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 how well defined it is from an Official Intel Bus Definition
perspective, but on at least one 486-era core and one Pentium-era core it
would wrap. So an inl(0xffff) would result in an inb(0xffff), inw(0),
inb(3) showing up on the bus. I hit this as a bug in debugging a custom
driver way too many years ago. The cores weren't from intel, but were AMD
and/or some other third party (I don't recall which ones). I'd rate my
surity of this knowledge as medium, so if there's some other resource that
contradicts this, I'd tend to believe that source for this edge-case
behavior.

Warner

[-- Attachment #2: Type: text/html, Size: 1651 bytes --]

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

[Gerd Hoffmann]

  Hi,

> So on x86 we can have 16-bit I/O accesses unaligned to 8-bit boundary?

Yes.

> So I _think_ today we should be good with removing the x86 line:
> 
> -# ifdef TARGET_I386
> -    { 1, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
> -# endif

Nope.  Breaks vgabios.  Testcase:

qemu-system-x86_64 -kernel /boot/vmlinuz-$(uname -r) -append vga=ask

All graphics modes are gone.

take care,
  Gerd

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

[Philippe Mathieu-Daudé]

On 6/12/22 15:38, Gerd Hoffmann wrote:
>    Hi,
> 
>> So on x86 we can have 16-bit I/O accesses unaligned to 8-bit boundary?
> 
> Yes.
> 
>> So I _think_ today we should be good with removing the x86 line:
>>
>> -# ifdef TARGET_I386
>> -    { 1, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
>> -# endif
> 
> Nope.  Breaks vgabios.  Testcase:
> 
> qemu-system-x86_64 -kernel /boot/vmlinuz-$(uname -r) -append vga=ask

Adding

  -trace memory_region_ops_\*

I get:

memory_region_ops_write cpu 0 mr 0x13eefbf60 addr 0x1ce value 0x0 size 2 
name 'vbe'
memory_region_ops_write cpu 0 mr 0x13eefbf60 addr 0x1cf value 0xb0c0 
size 2 name 'vbe'
memory_region_ops_write cpu 0 mr 0x13eefbf60 addr 0x1ce value 0x0 size 2 
name 'vbe'
memory_region_ops_read cpu 0 mr 0x13eefbf60 addr 0x1cf value 0xffff size 
2 name 'vbe'

> All graphics modes are gone.

Yeah I'll investigate, thanks for the easy test case.