| Message ID | 1314144835-29098-13-git-send-email-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
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.
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~
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?
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).
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~
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.
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,
Signed-off-by: Richard Henderson <rth@twiddle.net> --- hw/vga-isa.c | 10 ---------- hw/vga.c | 55 ++++++++++++++++++++++++++++--------------------------- 2 files changed, 28 insertions(+), 37 deletions(-)