On Thu, May 30, 2019 at 11:06:57PM -0500, Katherine Rohl wrote:
> Apologies.
>
> ---
>
Hi Katherine,
Thanks for the diff. I think we are getting close! A few comments below.
-ml
> diff --git a/sys/arch/amd64/amd64/vmm.c b/sys/arch/amd64/amd64/vmm.c
> index 4ffb2ff899f..7de38facc78 100644
> --- a/sys/arch/amd64/amd64/vmm.c
> +++ b/sys/arch/amd64/amd64/vmm.c
> @@ -5359,6 +5359,8 @@ svm_handle_inout(struct vcpu *vcpu)
> case IO_ICU1 ... IO_ICU1 + 1:
> case 0x40 ... 0x43:
> case PCKBC_AUX:
> + case 0x60:
> + case 0x64:
> case IO_RTC ... IO_RTC + 1:
> case IO_ICU2 ... IO_ICU2 + 1:
> case 0x3f8 ... 0x3ff:
> diff --git a/usr.sbin/vmd/Makefile b/usr.sbin/vmd/Makefile
> index 8645df7aecf..f39d85b1b14 100644
> --- a/usr.sbin/vmd/Makefile
> +++ b/usr.sbin/vmd/Makefile
> @@ -4,7 +4,7 @@
>
> PROG= vmd
> SRCS= vmd.c control.c log.c priv.c proc.c config.c vmm.c
> -SRCS+= vm.c loadfile_elf.c pci.c virtio.c i8259.c mc146818.c
> +SRCS+= vm.c loadfile_elf.c pci.c virtio.c i8259.c mc146818.c
> i8042.c
> SRCS+= ns8250.c i8253.c vmboot.c ufs.c disklabel.c dhcp.c
> packet.c
> SRCS+= parse.y atomicio.c vioscsi.c vioraw.c vioqcow2.c
> fw_cfg.c
>
> diff --git a/usr.sbin/vmd/i8042.c b/usr.sbin/vmd/i8042.c
> new file mode 100644
> index 00000000000..ced3b43da1c
> --- /dev/null
> +++ b/usr.sbin/vmd/i8042.c
> @@ -0,0 +1,423 @@
> +/* $OpenBSD$ */
> +/*
> + * Copyright (c) 2019 Katherine Rohl <luig...@gmail.com>
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <sys/types.h>
> +
> +#include <dev/ic/i8042reg.h>
> +
> +#include <machine/vmmvar.h>
> +
> +#include <event.h>
> +#include <pthread.h>
> +#include <string.h>
> +#include <stddef.h>
> +#include <unistd.h>
> +
> +#include "i8042.h"
> +#include "proc.h"
> +#include "vmm.h"
> +#include "atomicio.h"
> +#include "vmd.h"
> +
> +struct i8042 {
> + uint8_t data;
> + uint8_t command;
> + uint8_t status;
> + uint8_t internal_ram[16];
> +
> + uint8_t input_port;
> + uint8_t output_port;
> +
> + /* Port 0 is keyboard, Port 1 is mouse */
> + uint8_t ps2_enabled[2];
> +
> + uint32_t vm_id;
> +
> + /* Is the 8042 awaiting a command argument byte? */
> + uint8_t awaiting_argbyte;
> +};
> +
> +struct i8042 kbc;
> +pthread_mutex_t kbc_mtx;
> +
> +/*
> + * i8042_init
> + *
> + * Initialize the emulated i8042 KBC.
> + *
> + * Parameters:
> + * vm_id: vmm(4) assigned ID of the VM
> + */
> +void
> +i8042_init(uint32_t vm_id)
> +{
> + memset(&kbc, 0, sizeof(kbc));
> +
> + if(pthread_mutex_init(&kbc_mtx, NULL) != 0)
> + fatalx("unable to create kbc mutex");
> +
> + kbc.vm_id = vm_id;
> +
> + /* Always initialize the reset bit to 1 for proper operation. */
> + kbc.output_port = KBC_DEFOUTPORT;
> + kbc.input_port = KBC_DEFINPORT;
> +}
> +
> +/*
> + * i8042_set_output_port
> + *
> + * Set the output port of the KBC.
> + * Many bits have side effects, so handle their on/off
> + * transition effects as required.
> + */
> +static void
> +i8042_set_output_port(uint8_t byte)
> +{
> + /* 0x01: reset line
> + * 0x02: A20 gate (not implemented)
> + * 0x04: aux port clock
> + * 0x08: aux port data
> + * 0x10: data in output buffer is from kb port (IRQ1)
> + * 0x20: data in output buffer is from aux port (IRQ12)
> + * 0x40: kb port clock
> + * 0x80: kb port data
> + */
> +
> + uint8_t old_output_port = kbc.output_port;
> + kbc.output_port = byte;
> +
> + /* 0 -> 1 transition */
> + if (((old_output_port & 0x10) == 0) &&
> + ((kbc.output_port & 0x10) == 1))
> + vcpu_assert_pic_irq(kbc.vm_id, 0, KBC_KBDIRQ);
For this IRQ, if it's edge triggered, please assert then deassert the line.
The i8259 code should handle that properly. What you have here is a level
triggered interrupt (eg, the line will stay asserted until someone
does a 1 -> 0 transition below). Same goes for the next few cases.
> + if (((old_output_port & 0x20) == 0) &&
> + ((kbc.output_port & 0x20) == 1))
> + vcpu_assert_pic_irq(kbc.vm_id, 0, KBC_AUXIRQ);
> +
> + /* 1 -> 0 transition */
> + if (((old_output_port & 0x10) == 1) &&
> + ((kbc.output_port & 0x10) == 0))
> + vcpu_deassert_pic_irq(kbc.vm_id, 0, KBC_KBDIRQ);
> + if (((old_output_port & 0x20) == 1) &&
> + ((kbc.output_port & 0x20) == 0))
> + vcpu_deassert_pic_irq(kbc.vm_id, 0, KBC_AUXIRQ);
> +}
> +
> +/*
> + * i8042_perform_twobyte_command
> + *
> + * Handles the two-byte command (one byte of command and
> + * one byte of data) after the data has been received.
> + */
> +static void
> +i8042_perform_twobyte_command(void)
> +{
> + /*
> + * The command is in kbc.command and the argument
> + * is in kbc.data.
> + **/
Extra *
> +
> + switch (kbc.command) {
> + case 0x60 ... 0x7F: /* KBC_RAMWRITE */
> + kbc.internal_ram[kbc.command - 0x60] = kbc.data;
> + break;
> + case KBC_CMDWOUT:
> + i8042_set_output_port(kbc.data);
> + break;
> + case KBC_KBDECHO:
> + i8042_set_output_port(kbc.output_port|0x10);
> + break;
> + case KBC_AUXECHO:
> + i8042_set_output_port(kbc.output_port|0x20);
> + break;
> + case KBC_AUXWRITE:
> + /* No auxiliary device for this to go to. */
> + break;
> + }
> +
> + kbc.awaiting_argbyte = 0;
> +}
> +
> +/*
> + * i8042_process_command
> + *
> + * Handles the command byte currently in the KBC's command
> + * register, setting the data register accordingly.
> + */
> +static void
> +i8042_process_command(void)
> +{
> +#define SET_DIB_BIT (kbc.status |= KBS_DIB)
> + kbc.awaiting_argbyte = 0;
> +
> + switch (kbc.command) {
> + case 0x20 ... 0x3F: /* KBC_RAMREAD */
> + kbc.data = kbc.internal_ram[kbc.command - 0x20];
> + SET_DIB_BIT;
> + break;
> + case 0x60 ... 0x7F: /* KBC_RAMWRITE */
> + case KBC_CMDWOUT:
> + case KBC_KBDECHO:
> + case KBC_AUXECHO:
> + case KBC_AUXWRITE:
> + kbc.awaiting_argbyte = 1;
> + break;
> + case KBC_AUXENABLE:
> + kbc.ps2_enabled[KBC_AUXPORT] = 1;
> + break;
> + case KBC_AUXDISABLE:
> + kbc.ps2_enabled[KBC_AUXPORT] = 0;
> + break;
> + case KBC_AUXTEST:
> + kbc.data = 0;
> + SET_DIB_BIT;
> + break;
> + case KBC_KBDTEST:
> + kbc.data = 0;
> + SET_DIB_BIT;
> + break;
> + case KBC_KBDENABLE:
> + kbc.ps2_enabled[KBC_KBDPORT] = 1;
> + break;
> + case KBC_KBDDISABLE:
> + kbc.ps2_enabled[KBC_KBDPORT] = 0;
> + break;
> + case KBC_READINPORT:
> + kbc.data = kbc.input_port;
> + SET_DIB_BIT;
> + break;
> + case KBC_POLLINLO:
> + kbc.status &= 0x0F;
> + kbc.status |= (kbc.input_port << 4);
> + break;
> + case KBC_POLLINHI:
> + kbc.status &= 0x0F;
> + kbc.status |= (kbc.input_port & 0xF0);
> + break;
> + case KBC_CMDROUT:
> + kbc.data = kbc.output_port;
> + SET_DIB_BIT;
> + break;
> + case KBC_SELFTEST:
> + kbc.data = 0x55;
> + SET_DIB_BIT;
> + break;
> + case KBC_PULSE0:
> + kbc.output_port &= 0xFE;
> + break;
> + case KBC_PULSE1:
> + case KBC_PULSE2:
> + case KBC_PULSE3:
> + /* Ignore these commands. */
> + break;
> + }
> +}
> +
> +/*
> + * i8042_read_status_register
> + *
> + * Handles status register reads.
> + *
> + * Parameters:
> + *
> + * Return value:
> + * Value of the KBC status register.
> + */
> +static uint8_t
> +i8042_read_status_register(void)
> +{
> + return kbc.status;
> +}
> +
> +/*
> + * i8042_read_data_register
> + *
> + * Reads the data register of the KBC.
> + *
> + * Parameters:
> + *
> + * Return value:
> + * Value of the KBC data register.
> + */
> +static uint8_t
> +i8042_read_data_register(void)
> +{
> + /* Clear the output buffer full bit. */
> + kbc.status &= 0xFE;
> +
> + return kbc.data;
> +}
> +
> +/*
> + * i8042_write_command_register
> + *
> + * Writes a byte to the 8042 command register.
> + *
> + * Parameters:
> + * data: The byte to write.
> + */
> +static void
> +i8042_write_command_register(uint8_t data)
> +{
> + kbc.command = data;
> + i8042_process_command();
> +}
> +
> +/*
> + * i8042_write_data_register
> + *
> + * Write a byte to the KBC data register.
> + *
> + * Parameters:
> + * data: The byte to write.
> + */
> +static void
> +i8042_write_data_register(uint8_t data)
> +{
> + kbc.data = data;
> +
> + if (kbc.awaiting_argbyte) {
> + i8042_perform_twobyte_command();
> + }
> +}
> +
> +/*
> + * i8042_io_read
> + *
> + * Handles emulated reads of the KBC's I/O ports.
> + *
> + * Parameters:
> + * vei:
vei: exit information for this i8042 command
> + *
> + * Return value:
> + * The byte read from the addressed port.
> + *
> + */
> +static uint8_t
> +i8042_io_read(struct vm_exit *vei)
> +{
> + uint16_t port = vei->vei.vei_port;
> + uint8_t rv;
> +
> + mutex_lock(&kbc_mtx);
> +
> + switch (port) {
> + case KBC_CMD:
> + rv = i8042_read_status_register();;
> + break;
> + case KBC_DATA:
> + rv = i8042_read_data_register();
> + break;
> + default:
> + fatal("%s: invalid port 0x%x", __func__, port);
> + }
> +
> + mutex_unlock(&kbc_mtx);
> + return(rv);
> +}
> +
> +/*
> + * i8042_io_write
> + *
> + * Handles writes to the emulated KBC device, setting the
> + * value of the selected register.
> + *
> + * Parameters:
> + * vei:
vei: exit information for this i8042 command
> + *
> + */
> +static void
> +i8042_io_write(struct vm_exit *vei)
> +{
> + uint16_t port = vei->vei.vei_port;
> + uint32_t data;
> +
> + get_input_data(vei, &data);
> +
> + mutex_lock(&kbc_mtx);
> + switch (port) {
> + case KBC_DATA:
> + i8042_write_data_register(data);
> + break;
> + case KBC_CMD:
> + i8042_write_command_register(data);
> + break;
> + default:
> + fatal("%s: invalid port 0x%x", __func__, port);
> + }
> +
> + if ((kbc.output_port & 0x01) == 0) /* Deasserted the reset line? */
> + {
> + /* TODO: Reset the CPU. */
I would recommend changing vcpu_exit_inout to return a value, and if we
return EAGAIN here it should reset the VM (look at vm.c for how we handle
triple faults in the same way).
If you would rather get this in later, that's fine. It's probably a different
change, to be honest.
> + }
> +
> + mutex_unlock(&kbc_mtx);
> +}
> +
> +/*
> + * vcpu_exit_i8042
> + *
> + * Handles the 0x60 and 0x64 i8042 KBC in/out exits.
> + *
> + * Parameters:
> + * vrp: vm run parameters containing exit information for the I/O
> + * instruction being performed.
> + *
> + * Return value:
> + * 0xFF (?)
This means no interrupt will be injected. I'm not sure if that's what you want.
See vm.c: vcpu_exit_inout(..). It looks like you may have manually asserted the
IRQ in this file, which is a bit different than what we do in other devices.
That
may be okay, though.
> + *
> + */
> +uint8_t
> +vcpu_exit_i8042(struct vm_run_params *vrp)
> +{
> + struct vm_exit *vei = vrp->vrp_exit;
> +
> + if (vei->vei.vei_dir == VEI_DIR_OUT)
> + i8042_io_write(vei);
> + else
> + set_return_data(vei, i8042_io_read(vei));
> +
> + return (0xFF);
> +}
> +
> +int
> +i8042_restore(int fd)
> +{
> + log_debug("%s: restoring KBC", __func__);
> + if (atomicio(read, fd, &kbc, sizeof(kbc)) != sizeof(kbc)) {
> + log_warnx("%s: error reading KBC from fd", __func__);
> + return (-1);
> + }
> +
> + if (pthread_mutex_init(&kbc_mtx, NULL) != 0)
> + fatalx("unable to create kbc mutex");
> +
> + return (0);
> +}
> +
> +int
> +i8042_dump(int fd)
> +{
> + log_debug("%s: sending KBC", __func__);
> + if (atomicio(vwrite, fd, &kbc, sizeof(kbc)) != sizeof(kbc)) {
> + log_warnx("%s: error writing KBC to fd", __func__);
> + return (-1);
> + }
> +
> + return (0);
> +
Previous line has a trailing tab.
Also, please bump the revision in the vcpu struct for send/receive
as we will be sending a new struct layout now.
> +}
> diff --git a/usr.sbin/vmd/i8042.h b/usr.sbin/vmd/i8042.h
> new file mode 100644
> index 00000000000..67af8f54f2a
> --- /dev/null
> +++ b/usr.sbin/vmd/i8042.h
> @@ -0,0 +1,45 @@
> +/* $OpenBSD: i8042.c,v 1.00 2019/05/25 18:18:00 rohl Exp $ */
> +/*
> + * Copyright (c) 2019 Katherine Rohl <luig...@gmail.com>
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +/*
> + * Emulated i8042 (keyboard controller, based on 8048)
> + */
> +
> +#define KBC_DEFINPORT 0xB0 /* Default input port value. */
> +#define KBC_DEFOUTPORT 0x01 /* Default output port value. */
> +
> +#define KBC_KBDPORT 0 /* Keyboard is port 0. */
> +#define KBC_AUXPORT 1 /* Auxiliary is port 1. */
> +
> +#define KBC_KBDIRQ 1 /* Keyboard IRQ. */
> +#define KBC_AUXIRQ 12 /* Secondary IRQ. */
> +
> +#define KBC_DATA 0x60 /* Data is I/O port 60h. */
> +#define KBC_CMD 0x64 /* Command is I/O port 64h. */
> +
> +/* Commands that aren't useful for public includes. */
> +#define KBC_READINPORT 0xC0 /* Read the input port, place in data.
> */
> +#define KBC_POLLINLO 0xC1 /* Poll low 4 bits of input, place in status. */
> +#define KBC_POLLINHI 0xC2 /* Poll high 4 bits of input, place in status.
> */
> +#define KBC_CMDROUT 0xD0 /* Read the output port. */
> +
> +uint8_t vcpu_exit_i8042(struct vm_run_params *);
> +
> +void i8042_init(uint32_t);
> +
> +int i8042_restore(int);
> +int i8042_dump(int);
> diff --git a/usr.sbin/vmd/vm.c b/usr.sbin/vmd/vm.c
> index 72b2e379ac3..35adf25c1cf 100644
> --- a/usr.sbin/vmd/vm.c
> +++ b/usr.sbin/vmd/vm.c
> @@ -61,6 +61,7 @@
> #include "i8259.h"
> #include "ns8250.h"
> #include "mc146818.h"
> +#include "i8042.h"
> #include "fw_cfg.h"
> #include "atomicio.h"
>
> @@ -949,6 +950,11 @@ init_emulated_hw(struct vmop_create_params *vmc, int
> child_cdrom,
> ioports_map[ELCR0] = vcpu_exit_elcr;
> ioports_map[ELCR1] = vcpu_exit_elcr;
>
> + /* Init i8042 keyboard controller */
> + i8042_init(vcp->vcp_id);
> + ioports_map[0x64] = vcpu_exit_i8042;
> + ioports_map[0x60] = vcpu_exit_i8042;
> +
> /* Init ns8250 UART */
> ns8250_init(con_fd, vcp->vcp_id);
> for (i = COM1_DATA; i <= COM1_SCR; i++)
> @@ -1002,6 +1008,11 @@ restore_emulated_hw(struct vm_create_params *vcp, int
> fd,
> ioports_map[IO_ICU2] = vcpu_exit_i8259;
> ioports_map[IO_ICU2 + 1] = vcpu_exit_i8259;
>
> + /* Init i8042 keyboard controller */
> + i8042_restore(fd);
> + ioports_map[0x64] = vcpu_exit_i8042;
> + ioports_map[0x60]= vcpu_exit_i8042;
> +
> /* Init ns8250 UART */
> ns8250_restore(fd, con_fd, vcp->vcp_id);
> for (i = COM1_DATA; i <= COM1_SCR; i++)
> @@ -1291,7 +1302,7 @@ vcpu_run_loop(void *arg)
>
> for (;;) {
> ret = pthread_mutex_lock(&vcpu_run_mtx[n]);
> -
> +
Previous line has extra whitespace.
> if (ret) {
> log_warnx("%s: can't lock vcpu run mtx (%d)",
> __func__, (int)ret);
>
