| Message ID | AANLkTi=LqDt1FvzvQEdRYdkqWAFjjeDpNStcB8Zf7YLh@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 28.12.2010 um 22:24 schrieb Blue Swirl: > 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a introduced a bug concerning > reset bit for port 92. > > Since the keyboard output port and port 92 are not compatible anyway, > let's separate them. > > Reported-by: Peter Lieven <pl@dlh.net> > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > --- > hw/pc.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > hw/pckbd.c | 19 +--------------- > 2 files changed, 67 insertions(+), 20 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 18a4a9f..e63b397 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -411,11 +411,71 @@ void pc_cmos_init(ram_addr_t ram_size, > ram_addr_t above_4g_mem_size, > qemu_register_reset(pc_cmos_init_late, &arg); > } > > +/* port 92 stuff: could be split off */ > +typedef struct Port92State { > + ISADevice dev; > + uint8_t outport; > + qemu_irq *a20_out; > +} Port92State; > + > +static void port92_write(void *opaque, uint32_t addr, uint32_t val) > +{ > + Port92State *s = opaque; > + > + DPRINTF("port92: write 0x%02x\n", val); > + s->outport = val; > + qemu_set_irq(*s->a20_out, (val >> 1) & 1); > + if (val & 1) { > + qemu_system_reset_request(); > + } > +} > + > +static uint32_t port92_read(void *opaque, uint32_t addr) > +{ > + Port92State *s = opaque; > + uint32_t ret; > + > + ret = s->outport; > + DPRINTF("port92: read 0x%02x\n", ret); > + return ret; > +} > + > +static void port92_init(ISADevice *dev, qemu_irq *a20_out) > +{ > + Port92State *s = DO_UPCAST(Port92State, dev, dev); > + > + s->a20_out = a20_out; > +} > + > +static int port92_initfn(ISADevice *dev) > +{ > + Port92State *s = DO_UPCAST(Port92State, dev, dev); > + > + register_ioport_read(0x92, 1, 1, port92_read, s); > + register_ioport_write(0x92, 1, 1, port92_write, s); > + isa_init_ioport(dev, 0x92); > + return 0; > +} > + > +static ISADeviceInfo port92_info = { > + .qdev.name = "port92", > + .qdev.size = sizeof(Port92State), > + .qdev.no_user = 1, > + .init = port92_initfn, > +}; > + > +static void port92_register(void) > +{ > + isa_qdev_register(&port92_info); > +} > +device_init(port92_register) > + > static void handle_a20_line_change(void *opaque, int irq, int level) > { > CPUState *cpu = opaque; > > /* XXX: send to all CPUs ? */ > + /* XXX: add logic to handle multiple A20 line sources */ > cpu_x86_set_a20(cpu, level); > } > > @@ -1027,7 +1087,7 @@ void pc_basic_device_init(qemu_irq *isa_irq, > PITState *pit; > qemu_irq rtc_irq = NULL; > qemu_irq *a20_line; > - ISADevice *i8042; > + ISADevice *i8042, *port92; > qemu_irq *cpu_exit_irq; > > register_ioport_write(0x80, 1, 1, ioport80_write, NULL); > @@ -1061,10 +1121,12 @@ void pc_basic_device_init(qemu_irq *isa_irq, > } > } > > - a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1); > + a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2); > i8042 = isa_create_simple("i8042"); > - i8042_setup_a20_line(i8042, a20_line); > + i8042_setup_a20_line(i8042, &a20_line[0]); > vmmouse_init(i8042); > + port92 = isa_create_simple("port92"); > + port92_init(port92, &a20_line[1]); > > cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); > DMA_init(0, cpu_exit_irq); > diff --git a/hw/pckbd.c b/hw/pckbd.c > index 863b485..958de0a 100644 > --- a/hw/pckbd.c > +++ b/hw/pckbd.c > @@ -211,10 +211,8 @@ static void kbd_queue(KBDState *s, int b, int aux) > ps2_queue(s->kbd, b); > } > > -static void ioport92_write(void *opaque, uint32_t addr, uint32_t val) > +static void outport_write(KBDState *s, uint32_t addr, uint32_t val) > { > - KBDState *s = opaque; > - > DPRINTF("kbd: write outport=0x%02x\n", val); > s->outport = val; > if (s->a20_out) { > @@ -225,16 +223,6 @@ static void ioport92_write(void *opaque, uint32_t > addr, uint32_t val) > } > } > > -static uint32_t ioport92_read(void *opaque, uint32_t addr) > -{ > - KBDState *s = opaque; > - uint32_t ret; > - > - ret = s->outport; > - DPRINTF("kbd: read outport=0x%02x\n", ret); > - return ret; > -} > - > static void kbd_write_command(void *opaque, uint32_t addr, uint32_t val) > { > KBDState *s = opaque; > @@ -357,7 +345,7 @@ static void kbd_write_data(void *opaque, uint32_t > addr, uint32_t val) > kbd_queue(s, val, 1); > break; > case KBD_CCMD_WRITE_OUTPORT: > - ioport92_write(s, 0, val); > + outport_write(s, 0, val); > break; > case KBD_CCMD_WRITE_MOUSE: > ps2_write_mouse(s->mouse, val); > @@ -489,9 +477,6 @@ static int i8042_initfn(ISADevice *dev) > register_ioport_read(0x64, 1, 1, kbd_read_status, s); > register_ioport_write(0x64, 1, 1, kbd_write_command, s); > isa_init_ioport(dev, 0x64); > - register_ioport_read(0x92, 1, 1, ioport92_read, s); > - register_ioport_write(0x92, 1, 1, ioport92_write, s); > - isa_init_ioport(dev, 0x92); > > s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s); > s->mouse = ps2_mouse_init(kbd_update_aux_irq, s); > -- > 1.6.2.4 What happens if I set the A20 Gate via the keyboard controller and then read its status via ioport 0x92? This doesn't work, or does it? What was wrong reading the A20 via int ioport_get_a20(void) { return ((first_cpu->a20_mask >> 20) & 1); } every time ioport 0x92 or the keyobard outport is read? I also found somewhere that bit 0 (reset) of port 0x92 and the keyboard controller is read only. Maybe it should be set by default to 0 in the port92_read and to 1 when the keyboard outport is read. Peter
Am 28.12.2010 um 22:24 schrieb Blue Swirl: > 956a3e6bb7386de48b642d4fee11f7f86a2fcf9a introduced a bug concerning > reset bit for port 92. > > Since the keyboard output port and port 92 are not compatible anyway, > let's separate them. > > Reported-by: Peter Lieven <pl@dlh.net> > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> Looks good to me and would fix the PReP regression I reported, too. Assuming you tested it, Acked-by: Andreas Färber <andreas.faerber@web.de> Thanks, Andreas
On Tue, Dec 28, 2010 at 9:45 PM, Peter Lieven <pl@dlh.net> wrote: > What happens if I set the A20 Gate via the keyboard controller and then read its status via ioport 0x92? This doesn't work, or does it? > What was wrong reading the A20 via > > int ioport_get_a20(void) > { > return ((first_cpu->a20_mask >> 20) & 1); > } > > every time ioport 0x92 or the keyobard outport is read? I wonder the same thing. Also, stashing the uint8_t output value away is not migration-friendly at the moment? Stefan
On Wed, Dec 29, 2010 at 5:28 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Tue, Dec 28, 2010 at 9:45 PM, Peter Lieven <pl@dlh.net> wrote: >> What happens if I set the A20 Gate via the keyboard controller and then read its status via ioport 0x92? This doesn't work, or does it? >> What was wrong reading the A20 via >> >> int ioport_get_a20(void) >> { >> return ((first_cpu->a20_mask >> 20) & 1); >> } >> >> every time ioport 0x92 or the keyobard outport is read? > > I wonder the same thing. The manuals are very vague on this. The keyboard A20 signal is different from port 92 signal and the combined signal seen by the CPU is another one. What can be read back from either keyboard port or port 92 is probably highly implementation dependent. For example, if these are combined with an external logic (AND, NAND etc) gate, both ports could see different levels. With wired-OR logic it's possible to get the same signal as seen by CPU to both ports.But also the ports may not be fully bidirectional (so that instead of output signal, true signal level would be sampled) but the data from output buffer may be simply returned on reads. Only if the logic was implemented with something transparent like wired-OR and the output ports were bidirectional, then the read back values would match what the CPU sees. I doubt this, so I'd keep this version unless somebody can show that a real machine works this way. The problem with the original function was that CPUState was used inside a device, which is not OK. > Also, stashing the uint8_t output value away is not migration-friendly > at the moment? Will fix, also reset handling is missing.
diff --git a/hw/pc.c b/hw/pc.c index 18a4a9f..e63b397 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -411,11 +411,71 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, qemu_register_reset(pc_cmos_init_late, &arg); } +/* port 92 stuff: could be split off */ +typedef struct Port92State { + ISADevice dev; + uint8_t outport; + qemu_irq *a20_out; +} Port92State; + +static void port92_write(void *opaque, uint32_t addr, uint32_t val) +{ + Port92State *s = opaque; + + DPRINTF("port92: write 0x%02x\n", val); + s->outport = val; + qemu_set_irq(*s->a20_out, (val >> 1) & 1); + if (val & 1) { + qemu_system_reset_request(); + } +} + +static uint32_t port92_read(void *opaque, uint32_t addr) +{ + Port92State *s = opaque; + uint32_t ret; + + ret = s->outport; + DPRINTF("port92: read 0x%02x\n", ret); + return ret; +} + +static void port92_init(ISADevice *dev, qemu_irq *a20_out) +{ + Port92State *s = DO_UPCAST(Port92State, dev, dev); + + s->a20_out = a20_out; +} + +static int port92_initfn(ISADevice *dev) +{ + Port92State *s = DO_UPCAST(Port92State, dev, dev); + + register_ioport_read(0x92, 1, 1, port92_read, s); + register_ioport_write(0x92, 1, 1, port92_write, s); + isa_init_ioport(dev, 0x92); + return 0; +} + +static ISADeviceInfo port92_info = { + .qdev.name = "port92", + .qdev.size = sizeof(Port92State), + .qdev.no_user = 1, + .init = port92_initfn, +}; + +static void port92_register(void) +{ + isa_qdev_register(&port92_info); +} +device_init(port92_register) + static void handle_a20_line_change(void *opaque, int irq, int level) { CPUState *cpu = opaque; /* XXX: send to all CPUs ? */ + /* XXX: add logic to handle multiple A20 line sources */ cpu_x86_set_a20(cpu, level); } @@ -1027,7 +1087,7 @@ void pc_basic_device_init(qemu_irq *isa_irq, PITState *pit; qemu_irq rtc_irq = NULL; qemu_irq *a20_line; - ISADevice *i8042; + ISADevice *i8042, *port92; qemu_irq *cpu_exit_irq; register_ioport_write(0x80, 1, 1, ioport80_write, NULL); @@ -1061,10 +1121,12 @@ void pc_basic_device_init(qemu_irq *isa_irq, } } - a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1); + a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2); i8042 = isa_create_simple("i8042"); - i8042_setup_a20_line(i8042, a20_line); + i8042_setup_a20_line(i8042, &a20_line[0]); vmmouse_init(i8042); + port92 = isa_create_simple("port92"); + port92_init(port92, &a20_line[1]); cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); DMA_init(0, cpu_exit_irq); diff --git a/hw/pckbd.c b/hw/pckbd.c index 863b485..958de0a 100644 --- a/hw/pckbd.c +++ b/hw/pckbd.c @@ -211,10 +211,8 @@ static void kbd_queue(KBDState *s, int b, int aux) ps2_queue(s->kbd, b); } -static void ioport92_write(void *opaque, uint32_t addr, uint32_t val) +static void outport_write(KBDState *s, uint32_t addr, uint32_t val) { - KBDState *s = opaque; - DPRINTF("kbd: write outport=0x%02x\n", val); s->outport = val; if (s->a20_out) { @@ -225,16 +223,6 @@ static void ioport92_write(void *opaque, uint32_t addr, uint32_t val) } } -static uint32_t ioport92_read(void *opaque, uint32_t addr) -{ - KBDState *s = opaque; - uint32_t ret; - - ret = s->outport; - DPRINTF("kbd: read outport=0x%02x\n", ret); - return ret; -} - static void kbd_write_command(void *opaque, uint32_t addr, uint32_t val) {
956a3e6bb7386de48b642d4fee11f7f86a2fcf9a introduced a bug concerning reset bit for port 92. Since the keyboard output port and port 92 are not compatible anyway, let's separate them. Reported-by: Peter Lieven <pl@dlh.net> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- hw/pc.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/pckbd.c | 19 +--------------- 2 files changed, 67 insertions(+), 20 deletions(-) KBDState *s = opaque; @@ -357,7 +345,7 @@ static void kbd_write_data(void *opaque, uint32_t addr, uint32_t val) kbd_queue(s, val, 1); break; case KBD_CCMD_WRITE_OUTPORT: - ioport92_write(s, 0, val); + outport_write(s, 0, val); break; case KBD_CCMD_WRITE_MOUSE: ps2_write_mouse(s->mouse, val); @@ -489,9 +477,6 @@ static int i8042_initfn(ISADevice *dev) register_ioport_read(0x64, 1, 1, kbd_read_status, s); register_ioport_write(0x64, 1, 1, kbd_write_command, s); isa_init_ioport(dev, 0x64); - register_ioport_read(0x92, 1, 1, ioport92_read, s); - register_ioport_write(0x92, 1, 1, ioport92_write, s); - isa_init_ioport(dev, 0x92); s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s); s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);