Hi Andre, On 2/25/21 12:58 AM, 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 <[email protected]> > --- > hw/i8042.c | 68 +++++++++++++++++++++++++++--------------------------- > 1 file changed, 34 insertions(+), 34 deletions(-) > > diff --git a/hw/i8042.c b/hw/i8042.c > index 7d1f9772..ab866662 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; > } > > + 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, 0, NULL); > + > 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, 1, NULL);
I think I made a mistake when I suggested that is_write should 0 or 1. I followed the emulation path in kvmtool and it turns out that is_write is supposed to represent the field with the same name from the anonymous struct mmio from struct kvm_run (more precisely, the struct defined at line 4980 from Documentation/virt/kvm/api.rst, Linux tag v5.11). I think the is_write field is declared in kvm_run as an u8 for compatibility with an userspace which might not know about the bool type. is_write is also treated as a bool in the MMIO exit path by KVM (see arch/arm64/kvm/mmio.c::io_mem_abort()). kvmtool assigns is_write a boolean value everywhere except in this file, would you mind changing it back to a bool? Sorry for suggesting the wrong thing. I checked that the code remains functionally identical, with the above change: Reviewed-by: Alexandru Elisei <[email protected]> Thanks, Alex > > return true; > } _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
