| Message ID | 20201210142908.169597-8-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unify I/O port and MMIO trap handling | expand |
Hi Andre, On 12/10/20 2:28 PM, Andre Przywara wrote: > Now that the PC keyboard has a trap handler adhering to the MMIO fault > handler prototype, let's switch over to the joint registration routine. > > This allows us to get rid of the ioport shim routines. > > Make the kbd_init() function static on the way. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > hw/i8042.c | 30 ++++-------------------------- > include/kvm/i8042.h | 1 - > 2 files changed, 4 insertions(+), 27 deletions(-) > > diff --git a/hw/i8042.c b/hw/i8042.c > index eb1f9d28..91d79dc4 100644 > --- a/hw/i8042.c > +++ b/hw/i8042.c > @@ -325,40 +325,18 @@ static void kbd_io(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, > ioport__write8(data, value); > } > > -/* > - * Called when the OS has written to one of the keyboard's ports (0x60 or 0x64) > - */ > -static bool kbd_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) > -{ > - kbd_io(vcpu, port, data, size, false, NULL); > - > - return true; > -} > - > -static bool kbd_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) > -{ > - kbd_io(vcpu, port, data, size, true, NULL); > - > - return true; > -} > - > -static struct ioport_operations kbd_ops = { > - .io_in = kbd_in, > - .io_out = kbd_out, > -}; > - > -int kbd__init(struct kvm *kvm) > +static int kbd__init(struct kvm *kvm) > { > int r; > > kbd_reset(); > state.kvm = kvm; > - r = ioport__register(kvm, I8042_DATA_REG, &kbd_ops, 2, NULL); > + r = kvm__register_pio(kvm, I8042_DATA_REG, 2, kbd_io, NULL); I guess you are registering two addresses here to cover I8042_PORT_B_REG, right? Might be worth a comment. > if (r < 0) > return r; > - r = ioport__register(kvm, I8042_COMMAND_REG, &kbd_ops, 2, NULL); > + r = kvm__register_pio(kvm, I8042_COMMAND_REG, 2, kbd_io, NULL); Shouldn't length be 1? The emulation should work only for address 0x64 (command/status register), right? Or am I missing something? Thanks, Alex > if (r < 0) { > - ioport__unregister(kvm, I8042_DATA_REG); > + kvm__deregister_pio(kvm, I8042_DATA_REG); > return r; > } > > diff --git a/include/kvm/i8042.h b/include/kvm/i8042.h > index 3b4ab688..cd4ae6bb 100644 > --- a/include/kvm/i8042.h > +++ b/include/kvm/i8042.h > @@ -7,6 +7,5 @@ struct kvm; > > void mouse_queue(u8 c); > void kbd_queue(u8 c); > -int kbd__init(struct kvm *kvm); > > #endif
On Fri, 12 Feb 2021 10:41:20 +0000 Alexandru Elisei <alexandru.elisei@arm.com> wrote: Hi, > On 12/10/20 2:28 PM, Andre Przywara wrote: > > Now that the PC keyboard has a trap handler adhering to the MMIO fault > > handler prototype, let's switch over to the joint registration routine. > > > > This allows us to get rid of the ioport shim routines. > > > > Make the kbd_init() function static on the way. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > hw/i8042.c | 30 ++++-------------------------- > > include/kvm/i8042.h | 1 - > > 2 files changed, 4 insertions(+), 27 deletions(-) > > > > diff --git a/hw/i8042.c b/hw/i8042.c > > index eb1f9d28..91d79dc4 100644 > > --- a/hw/i8042.c > > +++ b/hw/i8042.c > > @@ -325,40 +325,18 @@ static void kbd_io(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, > > ioport__write8(data, value); > > } > > > > -/* > > - * Called when the OS has written to one of the keyboard's ports (0x60 or 0x64) > > - */ > > -static bool kbd_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) > > -{ > > - kbd_io(vcpu, port, data, size, false, NULL); > > - > > - return true; > > -} > > - > > -static bool kbd_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) > > -{ > > - kbd_io(vcpu, port, data, size, true, NULL); > > - > > - return true; > > -} > > - > > -static struct ioport_operations kbd_ops = { > > - .io_in = kbd_in, > > - .io_out = kbd_out, > > -}; > > - > > -int kbd__init(struct kvm *kvm) > > +static int kbd__init(struct kvm *kvm) > > { > > int r; > > > > kbd_reset(); > > state.kvm = kvm; > > - r = ioport__register(kvm, I8042_DATA_REG, &kbd_ops, 2, NULL); > > + r = kvm__register_pio(kvm, I8042_DATA_REG, 2, kbd_io, NULL); > > I guess you are registering two addresses here to cover I8042_PORT_B_REG, right? > Might be worth a comment. I am registering two ports because the original code did, and I didn't dare to touch this. I guess we put this on the wishlist for the device emulation fixup series? ;-) Maybe the intention was to just *reserve* those ports? > > > if (r < 0) > > return r; > > - r = ioport__register(kvm, I8042_COMMAND_REG, &kbd_ops, 2, NULL); > > + r = kvm__register_pio(kvm, I8042_COMMAND_REG, 2, kbd_io, NULL); > > Shouldn't length be 1? The emulation should work only for address 0x64 > (command/status register), right? Or am I missing something? I don't think you are, same as above. Maybe some weird guest is using half-word accesses (outw)? Cheers, Andre > > Thanks, > > Alex > > > if (r < 0) { > > - ioport__unregister(kvm, I8042_DATA_REG); > > + kvm__deregister_pio(kvm, I8042_DATA_REG); > > return r; > > } > > > > diff --git a/include/kvm/i8042.h b/include/kvm/i8042.h > > index 3b4ab688..cd4ae6bb 100644 > > --- a/include/kvm/i8042.h > > +++ b/include/kvm/i8042.h > > @@ -7,6 +7,5 @@ struct kvm; > > > > void mouse_queue(u8 c); > > void kbd_queue(u8 c); > > -int kbd__init(struct kvm *kvm); > > > > #endif
Hi Andre, On 2/18/21 12:09 PM, Andre Przywara wrote: > On Fri, 12 Feb 2021 10:41:20 +0000 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi, > >> On 12/10/20 2:28 PM, Andre Przywara wrote: >>> Now that the PC keyboard has a trap handler adhering to the MMIO fault >>> handler prototype, let's switch over to the joint registration routine. >>> >>> This allows us to get rid of the ioport shim routines. >>> >>> Make the kbd_init() function static on the way. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> hw/i8042.c | 30 ++++-------------------------- >>> include/kvm/i8042.h | 1 - >>> 2 files changed, 4 insertions(+), 27 deletions(-) >>> >>> diff --git a/hw/i8042.c b/hw/i8042.c >>> index eb1f9d28..91d79dc4 100644 >>> --- a/hw/i8042.c >>> +++ b/hw/i8042.c >>> @@ -325,40 +325,18 @@ static void kbd_io(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, >>> ioport__write8(data, value); >>> } >>> >>> -/* >>> - * Called when the OS has written to one of the keyboard's ports (0x60 or 0x64) >>> - */ >>> -static bool kbd_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) >>> -{ >>> - kbd_io(vcpu, port, data, size, false, NULL); >>> - >>> - return true; >>> -} >>> - >>> -static bool kbd_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) >>> -{ >>> - kbd_io(vcpu, port, data, size, true, NULL); >>> - >>> - return true; >>> -} >>> - >>> -static struct ioport_operations kbd_ops = { >>> - .io_in = kbd_in, >>> - .io_out = kbd_out, >>> -}; >>> - >>> -int kbd__init(struct kvm *kvm) >>> +static int kbd__init(struct kvm *kvm) >>> { >>> int r; >>> >>> kbd_reset(); >>> state.kvm = kvm; >>> - r = ioport__register(kvm, I8042_DATA_REG, &kbd_ops, 2, NULL); >>> + r = kvm__register_pio(kvm, I8042_DATA_REG, 2, kbd_io, NULL); >> I guess you are registering two addresses here to cover I8042_PORT_B_REG, right? >> Might be worth a comment. > I am registering two ports because the original code did, and I didn't > dare to touch this. I guess we put this on the wishlist for the device > emulation fixup series? ;-) > > Maybe the intention was to just *reserve* those ports? Considering that I8042_DATA_REG = 0x60 and I8042_PORT_B_REG = 0x61, and the emulation handlers handle both of them, I'm pretty sure the intention was to reserve memory to cover both ports. > >>> if (r < 0) >>> return r; >>> - r = ioport__register(kvm, I8042_COMMAND_REG, &kbd_ops, 2, NULL); >>> + r = kvm__register_pio(kvm, I8042_COMMAND_REG, 2, kbd_io, NULL); >> Shouldn't length be 1? The emulation should work only for address 0x64 >> (command/status register), right? Or am I missing something? > I don't think you are, same as above. Maybe some weird guest is using > half-word accesses (outw)? I think you're right, let's not mess with the device emulation right now, after all that's not the purpose of the series. And I'm fairly confident you know more about the device and the x86 architecture than me, so I'll trust your judgement on this. Thanks, Alex > > Cheers, > Andre > > >> Thanks, >> >> Alex >> >>> if (r < 0) { >>> - ioport__unregister(kvm, I8042_DATA_REG); >>> + kvm__deregister_pio(kvm, I8042_DATA_REG); >>> return r; >>> } >>> >>> diff --git a/include/kvm/i8042.h b/include/kvm/i8042.h >>> index 3b4ab688..cd4ae6bb 100644 >>> --- a/include/kvm/i8042.h >>> +++ b/include/kvm/i8042.h >>> @@ -7,6 +7,5 @@ struct kvm; >>> >>> void mouse_queue(u8 c); >>> void kbd_queue(u8 c); >>> -int kbd__init(struct kvm *kvm); >>> >>> #endif
diff --git a/hw/i8042.c b/hw/i8042.c index eb1f9d28..91d79dc4 100644 --- a/hw/i8042.c +++ b/hw/i8042.c @@ -325,40 +325,18 @@ static void kbd_io(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, ioport__write8(data, value); } -/* - * Called when the OS has written to one of the keyboard's ports (0x60 or 0x64) - */ -static bool kbd_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) -{ - kbd_io(vcpu, port, data, size, false, NULL); - - return true; -} - -static bool kbd_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) -{ - kbd_io(vcpu, port, data, size, true, NULL); - - return true; -} - -static struct ioport_operations kbd_ops = { - .io_in = kbd_in, - .io_out = kbd_out, -}; - -int kbd__init(struct kvm *kvm) +static int kbd__init(struct kvm *kvm) { int r; kbd_reset(); state.kvm = kvm; - r = ioport__register(kvm, I8042_DATA_REG, &kbd_ops, 2, NULL); + r = kvm__register_pio(kvm, I8042_DATA_REG, 2, kbd_io, NULL); if (r < 0) return r; - r = ioport__register(kvm, I8042_COMMAND_REG, &kbd_ops, 2, NULL); + r = kvm__register_pio(kvm, I8042_COMMAND_REG, 2, kbd_io, NULL); if (r < 0) { - ioport__unregister(kvm, I8042_DATA_REG); + kvm__deregister_pio(kvm, I8042_DATA_REG); return r; } diff --git a/include/kvm/i8042.h b/include/kvm/i8042.h index 3b4ab688..cd4ae6bb 100644 --- a/include/kvm/i8042.h +++ b/include/kvm/i8042.h @@ -7,6 +7,5 @@ struct kvm; void mouse_queue(u8 c); void kbd_queue(u8 c); -int kbd__init(struct kvm *kvm); #endif
Now that the PC keyboard has a trap handler adhering to the MMIO fault handler prototype, let's switch over to the joint registration routine. This allows us to get rid of the ioport shim routines. Make the kbd_init() function static on the way. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- hw/i8042.c | 30 ++++-------------------------- include/kvm/i8042.h | 1 - 2 files changed, 4 insertions(+), 27 deletions(-)