On Fri, 12 Feb 2021 10:41:20 +0000
Alexandru Elisei <alexandru.elisei@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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
