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

[Alexandru Elisei <alexandru.elisei@arm.com>]

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