Patchwork [12/16] vga: Convert to isa_register_portio_list.

login
register
mail settings
Submitter Richard Henderson
Date Aug. 24, 2011, 12:13 a.m.
Message ID <1314144835-29098-13-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/111215/
State New
Headers show

Comments

Richard Henderson - Aug. 24, 2011, 12:13 a.m.
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/vga-isa.c |   10 ----------
 hw/vga.c     |   55 ++++++++++++++++++++++++++++---------------------------
 2 files changed, 28 insertions(+), 37 deletions(-)
Avi Kivity - Sept. 18, 2011, 1:45 p.m.
On 08/24/2011 03:13 AM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson<rth@twiddle.net>

Breaks qemu-system-ppc -M mac99

> +/* Used by both ISA and PCI */
>   MemoryRegion *vga_init_io(VGACommonState *s)
>   {
>       MemoryRegion *vga_mem;
>
> -    register_ioport_write(0x3c0, 16, 1, vga_ioport_write, s);
> -
> -    register_ioport_write(0x3b4, 2, 1, vga_ioport_write, s);
> -    register_ioport_write(0x3d4, 2, 1, vga_ioport_write, s);
> -    register_ioport_write(0x3ba, 1, 1, vga_ioport_write, s);
> -    register_ioport_write(0x3da, 1, 1, vga_ioport_write, s);
> -
> -    register_ioport_read(0x3c0, 16, 1, vga_ioport_read, s);
> -
> -    register_ioport_read(0x3b4, 2, 1, vga_ioport_read, s);
> -    register_ioport_read(0x3d4, 2, 1, vga_ioport_read, s);
> -    register_ioport_read(0x3ba, 1, 1, vga_ioport_read, s);
> -    register_ioport_read(0x3da, 1, 1, vga_ioport_read, s);
> +    /* The PCI-ISA bridge should have been configured properly such that
> +       this works for PCI devices as well.  This only supports one bridge,
> +       but "secondary" VGA cards are generally accessed by MMIO only anyway.  */
> +    isa_register_portio_list(NULL, 0x3b0, vga_portio_list, s, "vga");
>
>       memory_region_init_io(vga_mem,&vga_mem_ops, s,

This is called even for pci machines which have no ISA bus (and even if 
they did, the code should work wit the pci bus, not ISA).  The code 
should return the portio list of the caller to register, or perhaps 
accept a callback to do the registration.
Richard Henderson - Sept. 18, 2011, 2:16 p.m.
On 09/18/2011 06:45 AM, Avi Kivity wrote:
>> +    /* The PCI-ISA bridge should have been configured properly such that
>> +       this works for PCI devices as well.  This only supports one bridge,
>> +       but "secondary" VGA cards are generally accessed by MMIO only anyway.  */
>> +    isa_register_portio_list(NULL, 0x3b0, vga_portio_list, s, "vga");
>>
>>       memory_region_init_io(vga_mem,&vga_mem_ops, s,
> 
> This is called even for pci machines which have no ISA bus (and even
> if they did, the code should work wit the pci bus, not ISA). The code
> should return the portio list of the caller to register, or perhaps
> accept a callback to do the registration.

You're over-thinking this.  It's all legacy ISA crap full stop.
If the machine doesn't have a PCI-ISA bridge, then the machine will
also be prepared to access the VGA registers via its BARs.

In such a case we just should skip this entire section.  Probably
isa_register_portio_list should simply notice no ISA bus has been
registered and do nothing.


r~
Avi Kivity - Sept. 18, 2011, 2:27 p.m.
On 09/18/2011 05:16 PM, Richard Henderson wrote:
> On 09/18/2011 06:45 AM, Avi Kivity wrote:
> >>  +    /* The PCI-ISA bridge should have been configured properly such that
> >>  +       this works for PCI devices as well.  This only supports one bridge,
> >>  +       but "secondary" VGA cards are generally accessed by MMIO only anyway.  */
> >>  +    isa_register_portio_list(NULL, 0x3b0, vga_portio_list, s, "vga");
> >>
> >>        memory_region_init_io(vga_mem,&vga_mem_ops, s,
> >
> >  This is called even for pci machines which have no ISA bus (and even
> >  if they did, the code should work wit the pci bus, not ISA). The code
> >  should return the portio list of the caller to register, or perhaps
> >  accept a callback to do the registration.
>
> You're over-thinking this.  It's all legacy ISA crap full stop.
> If the machine doesn't have a PCI-ISA bridge, then the machine will
> also be prepared to access the VGA registers via its BARs.
>
> In such a case we just should skip this entire section.  Probably
> isa_register_portio_list should simply notice no ISA bus has been
> registered and do nothing.

Depends, if it doesn't need those ports, then vga_init_io() can be 
passed a parameter not to register them, or perhaps it can be split into 
two.

But is this the case? Alex?
Avi Kivity - Sept. 18, 2011, 2:56 p.m.
On 09/18/2011 05:27 PM, Avi Kivity wrote:
> On 09/18/2011 05:16 PM, Richard Henderson wrote:
>> On 09/18/2011 06:45 AM, Avi Kivity wrote:
>> >>  +    /* The PCI-ISA bridge should have been configured properly 
>> such that
>> >>  +       this works for PCI devices as well.  This only supports 
>> one bridge,
>> >>  +       but "secondary" VGA cards are generally accessed by MMIO 
>> only anyway.  */
>> >>  +    isa_register_portio_list(NULL, 0x3b0, vga_portio_list, s, 
>> "vga");
>> >>
>> >>        memory_region_init_io(vga_mem,&vga_mem_ops, s,
>> >
>> >  This is called even for pci machines which have no ISA bus (and even
>> >  if they did, the code should work wit the pci bus, not ISA). The code
>> >  should return the portio list of the caller to register, or perhaps
>> >  accept a callback to do the registration.
>>
>> You're over-thinking this.  It's all legacy ISA crap full stop.
>> If the machine doesn't have a PCI-ISA bridge, then the machine will
>> also be prepared to access the VGA registers via its BARs.
>>
>> In such a case we just should skip this entire section.  Probably
>> isa_register_portio_list should simply notice no ISA bus has been
>> registered and do nothing.
>
> Depends, if it doesn't need those ports, then vga_init_io() can be 
> passed a parameter not to register them, or perhaps it can be split 
> into two.
>

It's also wrong for cirrus.  Even though it is a legacy address, it's 
not an ISA address, it's on the PCI bus (though not mapped by a BAR).
Richard Henderson - Sept. 18, 2011, 3:15 p.m.
On 09/18/2011 07:56 AM, Avi Kivity wrote:
> It's also wrong for cirrus. Even though it is a legacy address, it's
> not an ISA address, it's on the PCI bus (though not mapped by a BAR).

Huh?  How do define that as not an ISA address?  Especially
since all that's called from isa_cirrus_vga_init?


r~
Avi Kivity - Sept. 18, 2011, 3:19 p.m.
On 09/18/2011 06:15 PM, Richard Henderson wrote:
> On 09/18/2011 07:56 AM, Avi Kivity wrote:
> >  It's also wrong for cirrus. Even though it is a legacy address, it's
> >  not an ISA address, it's on the PCI bus (though not mapped by a BAR).
>
> Huh?  How do define that as not an ISA address?  Especially
> since all that's called from isa_cirrus_vga_init?
>

Ah, sorry, cirrus/pci indeed has its own ioports registration which 
doesn't go through isa.

So it's actually the opposite problem - generic port registration 
instead of bus-specific registration.  As soon as we convert it, we'll 
have the same problem again.

Patch

diff --git a/hw/vga-isa.c b/hw/vga-isa.c
index 0d19901..510ace8 100644
--- a/hw/vga-isa.c
+++ b/hw/vga-isa.c
@@ -54,16 +54,6 @@  static int vga_initfn(ISADevice *dev)
                                         isa_mem_base + 0x000a0000,
                                         vga_io_memory, 1);
     memory_region_set_coalescing(vga_io_memory);
-    isa_init_ioport(dev, 0x3c0);
-    isa_init_ioport(dev, 0x3b4);
-    isa_init_ioport(dev, 0x3ba);
-    isa_init_ioport(dev, 0x3da);
-    isa_init_ioport(dev, 0x3c0);
-#ifdef CONFIG_BOCHS_VBE
-    isa_init_ioport(dev, 0x1ce);
-    isa_init_ioport(dev, 0x1cf);
-    isa_init_ioport(dev, 0x1d0);
-#endif /* CONFIG_BOCHS_VBE */
     s->ds = graphic_console_init(s->update, s->invalidate,
                                  s->screen_dump, s->text_update, s);
 
diff --git a/hw/vga.c b/hw/vga.c
index b0371d5..7a0dedd 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2198,40 +2198,41 @@  void vga_common_init(VGACommonState *s, int vga_ram_size)
     vga_dirty_log_start(s);
 }
 
-/* used by both ISA and PCI */
+static const MemoryRegionPortio vga_portio_list[] = {
+    { 0x04,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3b4 */
+    { 0x0a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3ba */
+    { 0x10, 16, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3c0 */
+    { 0x14,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3d4 */
+    { 0x1a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3da */
+    PORTIO_END_OF_LIST(),
+};
+
+#ifdef CONFIG_BOCHS_VBE
+static const MemoryRegionPortio vbe_portio_list[] = {
+# ifdef TARGET_I386
+    { 0, 1, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index },
+    { 1, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
+# else
+    { 0, 2, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index },
+    { 2, 2, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
+# endif
+    PORTIO_END_OF_LIST(),
+};
+#endif /* CONFIG_BOCHS_VBE */
+
+/* Used by both ISA and PCI */
 MemoryRegion *vga_init_io(VGACommonState *s)
 {
     MemoryRegion *vga_mem;
 
-    register_ioport_write(0x3c0, 16, 1, vga_ioport_write, s);
-
-    register_ioport_write(0x3b4, 2, 1, vga_ioport_write, s);
-    register_ioport_write(0x3d4, 2, 1, vga_ioport_write, s);
-    register_ioport_write(0x3ba, 1, 1, vga_ioport_write, s);
-    register_ioport_write(0x3da, 1, 1, vga_ioport_write, s);
-
-    register_ioport_read(0x3c0, 16, 1, vga_ioport_read, s);
-
-    register_ioport_read(0x3b4, 2, 1, vga_ioport_read, s);
-    register_ioport_read(0x3d4, 2, 1, vga_ioport_read, s);
-    register_ioport_read(0x3ba, 1, 1, vga_ioport_read, s);
-    register_ioport_read(0x3da, 1, 1, vga_ioport_read, s);
+    /* The PCI-ISA bridge should have been configured properly such that
+       this works for PCI devices as well.  This only supports one bridge,
+       but "secondary" VGA cards are generally accessed by MMIO only anyway.  */
+    isa_register_portio_list(NULL, 0x3b0, vga_portio_list, s, "vga");
 
 #ifdef CONFIG_BOCHS_VBE
-#if defined (TARGET_I386)
-    register_ioport_read(0x1ce, 1, 2, vbe_ioport_read_index, s);
-    register_ioport_read(0x1cf, 1, 2, vbe_ioport_read_data, s);
-
-    register_ioport_write(0x1ce, 1, 2, vbe_ioport_write_index, s);
-    register_ioport_write(0x1cf, 1, 2, vbe_ioport_write_data, s);
-#else
-    register_ioport_read(0x1ce, 1, 2, vbe_ioport_read_index, s);
-    register_ioport_read(0x1d0, 1, 2, vbe_ioport_read_data, s);
-
-    register_ioport_write(0x1ce, 1, 2, vbe_ioport_write_index, s);
-    register_ioport_write(0x1d0, 1, 2, vbe_ioport_write_data, s);
+    isa_register_portio_list(NULL, 0x1ce, vbe_portio_list, s, "vbe");
 #endif
-#endif /* CONFIG_BOCHS_VBE */
 
     vga_mem = g_malloc(sizeof(*vga_mem));
     memory_region_init_io(vga_mem, &vga_mem_ops, s,