Re: [PATCH kvmtool 07/21] hw/i8042: Switch to new trap handlers

Alexandru Elisei
2021-02-12T10:43:45Z

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