[PATCH kvmtool 06/21] hw/i8042: Refactor trap handler

[Andre Przywara]

On Thu, 11 Feb 2021 17:23:13 +0000
Alexandru Elisei <alexandru.elisei at arm.com> wrote:

> Hi Andre,
> 
> On 12/10/20 2:28 PM, Andre Przywara wrote:
> > With the planned retirement of the special ioport emulation code, we
> > need to provide an emulation function compatible with the MMIO
> > prototype.
> >
> > Adjust the trap handler to use that new function, and provide shims to
> > implement the old ioport interface, for now.
> >
> > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > ---
> >  hw/i8042.c | 68 +++++++++++++++++++++++++++---------------------------
> >  1 file changed, 34 insertions(+), 34 deletions(-)
> >
> > diff --git a/hw/i8042.c b/hw/i8042.c
> > index 36ee183f..eb1f9d28 100644
> > --- a/hw/i8042.c
> > +++ b/hw/i8042.c
> > @@ -292,52 +292,52 @@ static void kbd_reset(void)
> >  	};
> >  }
> >  
> > -/*
> > - * 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)
> > +static void kbd_io(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len,
> > +		   u8 is_write, void *ptr)
> >  {
> > -	switch (port) {
> > -	case I8042_COMMAND_REG: {
> > -		u8 value = kbd_read_status();
> > -		ioport__write8(data, value);
> > +	u8 value;
> > +
> > +	if (is_write)
> > +		value = ioport__read8(data);
> > +
> > +	switch (addr) {
> > +	case I8042_COMMAND_REG:
> > +		if (is_write)
> > +			kbd_write_command(vcpu->kvm, value);
> > +		else
> > +			value = kbd_read_status();
> >  		break;
> > -	}
> > -	case I8042_DATA_REG: {
> > -		u8 value = kbd_read_data();
> > -		ioport__write8(data, value);
> > +	case I8042_DATA_REG:
> > +		if (is_write)
> > +			kbd_write_data(value);
> > +		else
> > +			value = kbd_read_data();
> >  		break;
> > -	}
> > -	case I8042_PORT_B_REG: {
> > -		ioport__write8(data, 0x20);
> > +	case I8042_PORT_B_REG:
> > +		if (!is_write)
> > +			value = 0x20;
> >  		break;
> > -	}
> >  	default:
> > -		return false;
> > +		return;  
> 
> Any particular reason for removing the false return value? I don't see it
> mentioned in the commit message. Otherwise this is identical to the two functions
> it replaces.

Because the MMIO handler prototype is void, in contrast to the PIO one.
Since on returning "false" we only seem to panic kvmtool, this is of
quite limited use, I'd say.

> >  	}
> >  
> > +	if (!is_write)
> > +		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);  
> 
> is_write is an u8, not a bool.

Right, will fix this.

> I never could wrap my head around the ioport convention of "in" (read) and "out"
> (write). To be honest, changing is_write changed to an enum so it's crystal clear
> what is happening would help with that a lot, but I guess that's a separate patch.

"in" and "out" are the x86 assembly mnemonics, but it's indeed
confusing, because the device side has a different view (CPU "in" means
pushing data "out" of the device). I usually look at the code to see
what it's actually meant to do.
So yeah, I feel like a lot of those device emulations could use
some update. but that's indeed something for another day.

Cheers,
Andre

> > +
> >  	return true;
> >  }
> >  
> >  static bool kbd_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
> >  {
> > -	switch (port) {
> > -	case I8042_COMMAND_REG: {
> > -		u8 value = ioport__read8(data);
> > -		kbd_write_command(vcpu->kvm, value);
> > -		break;
> > -	}
> > -	case I8042_DATA_REG: {
> > -		u8 value = ioport__read8(data);
> > -		kbd_write_data(value);
> > -		break;
> > -	}
> > -	case I8042_PORT_B_REG: {
> > -		break;
> > -	}
> > -	default:
> > -		return false;
> > -	}
> > +	kbd_io(vcpu, port, data, size, true, NULL);
> >  
> >  	return true;
> >  }