| Message ID | 20220928195633.2348848-15-quic_eberman@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Drivers for gunyah hypervisor | expand |
On Wed, Sep 28, 2022 at 12:56:33PM -0700, Elliot Berman wrote: > Gunyah provides a console for each VM using the VM console resource > manager APIs. This driver allows console data from other > VMs to be accessed via a TTY device and exports a console device to dump > Linux's own logs to our console. > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > --- > MAINTAINERS | 1 + > drivers/tty/Kconfig | 8 + > drivers/tty/Makefile | 1 + > drivers/tty/gunyah_tty.c | 409 +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 419 insertions(+) > create mode 100644 drivers/tty/gunyah_tty.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a0cba618e5f6..e8d4a6d9491a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8890,6 +8890,7 @@ F: Documentation/devicetree/bindings/firmware/gunyah-hypervisor.yaml > F: Documentation/virt/gunyah/ > F: arch/arm64/gunyah/ > F: drivers/mailbox/gunyah-msgq.c > +F: drivers/tty/gunyah_tty.c > F: drivers/virt/gunyah/ > F: include/asm-generic/gunyah.h > F: include/linux/gunyah*.h > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig > index cc30ff93e2e4..ff86e977f9ac 100644 > --- a/drivers/tty/Kconfig > +++ b/drivers/tty/Kconfig > @@ -380,6 +380,14 @@ config RPMSG_TTY > To compile this driver as a module, choose M here: the module will be > called rpmsg_tty. > > +config GUNYAH_CONSOLE > + tristate "Gunyah Consoles" > + depends on GUNYAH > + help > + This enables support for console output using Gunyah's Resource Manager RPC. > + This is normally used when a secondary VM which does not have exclusive access > + to a real or virtualized serial device and virtio-console is unavailable. module name? > + > endif # TTY > > source "drivers/tty/serdev/Kconfig" > diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile > index 07aca5184a55..d183fbfd835b 100644 > --- a/drivers/tty/Makefile > +++ b/drivers/tty/Makefile > @@ -27,5 +27,6 @@ obj-$(CONFIG_GOLDFISH_TTY) += goldfish.o > obj-$(CONFIG_MIPS_EJTAG_FDC_TTY) += mips_ejtag_fdc.o > obj-$(CONFIG_VCC) += vcc.o > obj-$(CONFIG_RPMSG_TTY) += rpmsg_tty.o > +obj-$(CONFIG_GUNYAH_CONSOLE) += gunyah_tty.o > > obj-y += ipwireless/ > diff --git a/drivers/tty/gunyah_tty.c b/drivers/tty/gunyah_tty.c > new file mode 100644 > index 000000000000..80a20da11ad0 > --- /dev/null > +++ b/drivers/tty/gunyah_tty.c > @@ -0,0 +1,409 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#define pr_fmt(fmt) "gh_rsc_mgr_console: " fmt You are a driver, use dev_printk() functions, no need for pr_fmt() at all, right? > + > +#include <linux/gunyah_rsc_mgr.h> > +#include <linux/auxiliary_bus.h> > +#include <linux/workqueue.h> > +#include <linux/spinlock.h> > +#include <linux/tty_flip.h> > +#include <linux/console.h> > +#include <linux/module.h> > +#include <linux/kfifo.h> > +#include <linux/kref.h> > +#include <linux/slab.h> > +#include <linux/tty.h> > + > +/* > + * The Linux TTY code does not support dynamic addition of tty derived devices so we need to know > + * how many tty devices we might need when space is allocated for the tty device. Since VMs might be > + * added/removed dynamically, we need to make sure we have enough allocated. Wrap comments at 80 columns please. > + */ > +#define RSC_MGR_TTY_ADAPTERS 16 We can have dynamic tty devices, so I don't understand this comment. What really is the problem here? > + > +/* # of payload bytes that can fit in a 1-fragment CONSOLE_WRITE message */ > +#define RM_CONS_WRITE_MSG_SIZE ((1 * (GH_MSGQ_MAX_MSG_SIZE - 8)) - 4) > + > +struct rm_cons_port { > + struct tty_port port; > + u16 vmid; > + bool open; Why do you care if it is open or not? > + unsigned int index; > + > + DECLARE_KFIFO(put_fifo, char, 1024); > + spinlock_t fifo_lock; > + struct work_struct put_work; > + > + struct rm_cons_data *cons_data; > +}; > + > +struct rm_cons_data { > + struct tty_driver *tty_driver; > + struct device *dev; > + > + spinlock_t ports_lock; > + struct rm_cons_port *ports[RSC_MGR_TTY_ADAPTERS]; > + > + struct notifier_block rsc_mgr_notif; > + struct console console; > +}; > + > +static void put_work_fn(struct work_struct *ws) > +{ > + char buf[RM_CONS_WRITE_MSG_SIZE]; > + int count, ret; > + struct rm_cons_port *port = container_of(ws, struct rm_cons_port, put_work); > + > + while (!kfifo_is_empty(&port->put_fifo)) { > + count = kfifo_out_spinlocked(&port->put_fifo, buf, sizeof(buf), &port->fifo_lock); > + if (count <= 0) > + continue; > + > + ret = gh_rm_console_write(port->vmid, buf, count); > + if (ret) { > + pr_warn_once("failed to send characters: %d\n", ret); What will this warning help with? > + break; If an error happens, shouldn't you keep trying to send the rest of the data? > + } > + } > +} > + > +static int rsc_mgr_console_notif(struct notifier_block *nb, unsigned long cmd, void *data) > +{ > + int count, i; > + struct rm_cons_port *rm_port = NULL; > + struct tty_port *tty_port = NULL; > + struct rm_cons_data *cons_data = container_of(nb, struct rm_cons_data, rsc_mgr_notif); > + const struct gh_rm_notification *notif = data; > + struct gh_rm_notif_vm_console_chars const * const msg = notif->buff; > + > + if (cmd != GH_RM_NOTIF_VM_CONSOLE_CHARS || > + notif->size < sizeof(*msg)) > + return NOTIFY_DONE; > + > + spin_lock(&cons_data->ports_lock); > + for (i = 0; i < RSC_MGR_TTY_ADAPTERS; i++) { > + if (!cons_data->ports[i]) > + continue; > + if (cons_data->ports[i]->vmid == msg->vmid) { > + rm_port = cons_data->ports[i]; > + break; > + } > + } > + if (rm_port) > + tty_port = tty_port_get(&rm_port->port); > + spin_unlock(&cons_data->ports_lock); > + > + if (!rm_port) > + pr_warn("Received unexpected console characters for VMID %u\n", msg->vmid); > + if (!tty_port) > + return NOTIFY_DONE; > + > + count = tty_buffer_request_room(tty_port, msg->num_bytes); > + tty_insert_flip_string(tty_port, msg->bytes, count); > + tty_flip_buffer_push(tty_port); > + > + tty_port_put(tty_port); > + return NOTIFY_OK; > +} > + > +static ssize_t vmid_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + struct rm_cons_port *rm_port = dev_get_drvdata(dev); > + > + if (rm_port->vmid == GH_VMID_SELF) > + return sysfs_emit(buf, "self\n"); > + > + return sysfs_emit(buf, "%u\n", rm_port->vmid); You didn't document this sysfs file, why not? And tty drivers should not have random sysfs files, please don't add this. > +} > + > +static DEVICE_ATTR_RO(vmid); > + > +static struct attribute *rsc_mgr_tty_dev_attrs[] = { > + &dev_attr_vmid.attr, > + NULL > +}; > + > +static const struct attribute_group rsc_mgr_tty_dev_attr_group = { > + .attrs = rsc_mgr_tty_dev_attrs, > +}; > + > +static const struct attribute_group *rsc_mgr_tty_dev_attr_groups[] = { > + &rsc_mgr_tty_dev_attr_group, > + NULL > +}; > + > +static int rsc_mgr_tty_open(struct tty_struct *tty, struct file *filp) > +{ > + int ret; > + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); > + > + if (!rm_port->open) { Why are you caring if the port is open already or not? > + ret = gh_rm_console_open(rm_port->vmid); Can't this just be called for every open()? And what happens if this changes right after it is checked? > + if (ret) { > + pr_err("Failed to open RM console for vmid %x: %d\n", rm_port->vmid, ret); dev_err() > + return ret; > + } > + rm_port->open = true; > + } > + > + return tty_port_open(&rm_port->port, tty, filp); > +} > + > +static void rsc_mgr_tty_close(struct tty_struct *tty, struct file *filp) > +{ > + int ret; > + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); > + > + if (rm_port->open) { > + if (rm_port->vmid != GH_VMID_SELF) { > + ret = gh_rm_console_close(rm_port->vmid); > + if (ret) > + pr_warn("Failed to close RM console for vmid %d: %d\n", > + rm_port->vmid, ret); > + } > + rm_port->open = false; So if you had multiple open/close this would close the console the first close call, but not the second? Are you sure you tested this out properly? > + > + tty_port_close(&rm_port->port, tty, filp); > + } > + > +} > + > +static int rsc_mgr_tty_write(struct tty_struct *tty, const unsigned char *buf, int count) > +{ > + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); > + int ret; > + > + ret = kfifo_in_spinlocked(&rm_port->put_fifo, buf, count, &rm_port->fifo_lock); > + if (ret > 0) > + schedule_work(&rm_port->put_work); Why not just do the write here? Why is a work queue needed? > + > + return ret; > +} > + > +static unsigned int rsc_mgr_mgr_tty_write_room(struct tty_struct *tty) > +{ > + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); > + > + return kfifo_avail(&rm_port->put_fifo); > +} > + > +static void rsc_mgr_console_write(struct console *co, const char *buf, unsigned count) > +{ > + struct rm_cons_port *rm_port = co->data; > + int ret; > + > + ret = kfifo_in_spinlocked(&rm_port->put_fifo, buf, count, &rm_port->fifo_lock); > + if (ret > 0) > + schedule_work(&rm_port->put_work); Same here, why not just send the data now? > +} > + > +static struct tty_driver *rsc_mgr_console_device(struct console *co, int *index) > +{ > + struct rm_cons_port *rm_port = co->data; > + > + *index = rm_port->index; > + return rm_port->port.tty->driver; Love the locking :( > +} > + > +static int rsc_mgr_console_setup(struct console *co, char *unused) > +{ > + int ret; > + struct rm_cons_port *rm_port = co->data; > + > + if (!rm_port->open) { > + ret = gh_rm_console_open(rm_port->vmid); > + if (ret) { > + pr_err("Failed to open RM console for vmid %x: %d\n", rm_port->vmid, ret); > + return ret; > + } > + rm_port->open = true; Again, don't mess with open/close. > + } > + > + return 0; > +} > + > +static int rsc_mgr_console_exit(struct console *co) > +{ > + int ret; > + struct rm_cons_port *rm_port = co->data; > + > + if (rm_port->open) { > + ret = gh_rm_console_close(rm_port->vmid); > + if (ret) { > + pr_err("Failed to close RM console for vmid %x: %d\n", rm_port->vmid, ret); > + return ret; > + } > + rm_port->open = false; > + } > + > + return 0; > +} > + > +static const struct tty_operations rsc_mgr_tty_ops = { > + .open = rsc_mgr_tty_open, > + .close = rsc_mgr_tty_close, > + .write = rsc_mgr_tty_write, > + .write_room = rsc_mgr_mgr_tty_write_room, > +}; > + > +static void rsc_mgr_port_destruct(struct tty_port *port) > +{ > + struct rm_cons_port *rm_port = container_of(port, struct rm_cons_port, port); > + struct rm_cons_data *cons_data = rm_port->cons_data; > + > + spin_lock(&cons_data->ports_lock); > + WARN_ON(cons_data->ports[rm_port->index] != rm_port); Does this mean you just crashed the system if something went wrong? How can this ever happen? > + cons_data->ports[rm_port->index] = NULL; > + spin_unlock(&cons_data->ports_lock); > + kfree(rm_port); > +} > + > +static const struct tty_port_operations rsc_mgr_port_ops = { > + .destruct = rsc_mgr_port_destruct, > +}; > + > +static struct rm_cons_port *rsc_mgr_port_create(struct rm_cons_data *cons_data, u16 vmid) > +{ > + struct rm_cons_port *rm_port; > + struct device *ttydev; > + unsigned int index; > + int ret; > + > + rm_port = kzalloc(sizeof(*rm_port), GFP_KERNEL); > + rm_port->vmid = vmid; > + INIT_KFIFO(rm_port->put_fifo); > + spin_lock_init(&rm_port->fifo_lock); > + INIT_WORK(&rm_port->put_work, put_work_fn); > + tty_port_init(&rm_port->port); > + rm_port->port.ops = &rsc_mgr_port_ops; > + > + spin_lock(&cons_data->ports_lock); > + for (index = 0; index < RSC_MGR_TTY_ADAPTERS; index++) { > + if (!cons_data->ports[index]) { > + cons_data->ports[index] = rm_port; > + rm_port->index = index; > + break; > + } > + } > + spin_unlock(&cons_data->ports_lock); > + if (index >= RSC_MGR_TTY_ADAPTERS) { > + ret = -ENOSPC; > + goto err_put_port; > + } > + > + ttydev = tty_port_register_device_attr(&rm_port->port, cons_data->tty_driver, index, > + cons_data->dev, rm_port, rsc_mgr_tty_dev_attr_groups); > + if (IS_ERR(ttydev)) { > + ret = PTR_ERR(ttydev); > + goto err_put_port; > + } > + > + return rm_port; > +err_put_port: > + tty_port_put(&rm_port->port); > + return ERR_PTR(ret); > +} > + > +static int rsc_mgr_console_probe(struct auxiliary_device *auxdev, > + const struct auxiliary_device_id *aux_dev_id) > +{ > + struct rm_cons_data *cons_data; > + struct rm_cons_port *rm_port; > + int ret; > + u16 vmid; > + > + cons_data = devm_kzalloc(&auxdev->dev, sizeof(*cons_data), GFP_KERNEL); > + if (!cons_data) > + return -ENOMEM; > + dev_set_drvdata(&auxdev->dev, cons_data); > + cons_data->dev = &auxdev->dev; > + > + cons_data->tty_driver = tty_alloc_driver(RSC_MGR_TTY_ADAPTERS, > + TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV); > + if (IS_ERR(cons_data->tty_driver)) > + return PTR_ERR(cons_data->tty_driver); > + > + cons_data->tty_driver->driver_name = "gh"; > + cons_data->tty_driver->name = "ttyGH"; Where did you pick this name from? Where is it documented? > + cons_data->tty_driver->type = TTY_DRIVER_TYPE_SYSTEM; > + cons_data->tty_driver->init_termios = tty_std_termios; > + tty_set_operations(cons_data->tty_driver, &rsc_mgr_tty_ops); > + > + ret = tty_register_driver(cons_data->tty_driver); > + if (ret) { > + dev_err(&auxdev->dev, "Could not register tty driver: %d\n", ret); > + goto err_put_tty; > + } > + > + spin_lock_init(&cons_data->ports_lock); > + > + cons_data->rsc_mgr_notif.notifier_call = rsc_mgr_console_notif; > + ret = gh_rm_register_notifier(&cons_data->rsc_mgr_notif); > + if (ret) { > + dev_err(&auxdev->dev, "Could not register for resource manager notifications: %d\n", > + ret); > + goto err_put_tty; > + } > + > + rm_port = rsc_mgr_port_create(cons_data, GH_VMID_SELF); > + if (IS_ERR(rm_port)) { > + ret = PTR_ERR(rm_port); > + dev_err(&auxdev->dev, "Could not create own console: %d\n", ret); > + goto err_unreg_notif; > + } > + > + strncpy(cons_data->console.name, "ttyGH", sizeof(cons_data->console.name)); Again, where did this name come from? thanks, greg k-h
On 28. 09. 22, 21:56, Elliot Berman wrote: > --- /dev/null > +++ b/drivers/tty/gunyah_tty.c > @@ -0,0 +1,409 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#define pr_fmt(fmt) "gh_rsc_mgr_console: " fmt > + > +#include <linux/gunyah_rsc_mgr.h> > +#include <linux/auxiliary_bus.h> > +#include <linux/workqueue.h> > +#include <linux/spinlock.h> > +#include <linux/tty_flip.h> > +#include <linux/console.h> > +#include <linux/module.h> > +#include <linux/kfifo.h> > +#include <linux/kref.h> > +#include <linux/slab.h> > +#include <linux/tty.h> Sort alphabetically, please. Not by inv. xmas tree. > +/* > + * The Linux TTY code does not support dynamic addition of tty derived devices so we need to know > + * how many tty devices we might need when space is allocated for the tty device. Since VMs might be > + * added/removed dynamically, we need to make sure we have enough allocated. > + */ > +#define RSC_MGR_TTY_ADAPTERS 16 > + > +/* # of payload bytes that can fit in a 1-fragment CONSOLE_WRITE message */ > +#define RM_CONS_WRITE_MSG_SIZE ((1 * (GH_MSGQ_MAX_MSG_SIZE - 8)) - 4) "1 *" is kind of superfluous. "- 8 - 4" -- it's too many magic constants in here. Define macros for them. > +struct rm_cons_port { > + struct tty_port port; > + u16 vmid; > + bool open; > + unsigned int index; > + > + DECLARE_KFIFO(put_fifo, char, 1024); Why is tty_port::xmit_fifo not enough? > + spinlock_t fifo_lock; > + struct work_struct put_work; > + > + struct rm_cons_data *cons_data; > +}; > + > +struct rm_cons_data { > + struct tty_driver *tty_driver; It looks weird to have a driver per console/device. > + struct device *dev; > + > + spinlock_t ports_lock; > + struct rm_cons_port *ports[RSC_MGR_TTY_ADAPTERS]; > + > + struct notifier_block rsc_mgr_notif; > + struct console console; > +}; > + > +static void put_work_fn(struct work_struct *ws) > +{ > + char buf[RM_CONS_WRITE_MSG_SIZE]; Ugh, is this 1024-12? Do not do this on stack! > + int count, ret; > + struct rm_cons_port *port = container_of(ws, struct rm_cons_port, put_work); > + > + while (!kfifo_is_empty(&port->put_fifo)) { > + count = kfifo_out_spinlocked(&port->put_fifo, buf, sizeof(buf), &port->fifo_lock); > + if (count <= 0) > + continue; This does not make much sense. 1) kfifo_is_empty() is not locked; 2) it's overly complicated. It can be, IMO: while (1) { count = kfifo_out_spinlocked(); if (!count) break; > +static int rsc_mgr_console_notif(struct notifier_block *nb, unsigned long cmd, void *data) > +{ > + int count, i; Not unsigned? > + struct rm_cons_port *rm_port = NULL; > + struct tty_port *tty_port = NULL; > + struct rm_cons_data *cons_data = container_of(nb, struct rm_cons_data, rsc_mgr_notif); > + const struct gh_rm_notification *notif = data; > + struct gh_rm_notif_vm_console_chars const * const msg = notif->buff; Interesting mix of inconsistencies. Once you start with const, once you place it after struct. Please make it consistent (start with const). ANd here, you should apply inv. xmas tree sorting. > + > + if (cmd != GH_RM_NOTIF_VM_CONSOLE_CHARS || > + notif->size < sizeof(*msg)) > + return NOTIFY_DONE; Weird indentation. notif->size should start with either 4 spaces less or one more tab. regards,
On 9/30/2022 5:17 AM, Greg Kroah-Hartman wrote: > On Wed, Sep 28, 2022 at 12:56:33PM -0700, Elliot Berman wrote: >> Gunyah provides a console for each VM using the VM console resource >> manager APIs. This driver allows console data from other >> VMs to be accessed via a TTY device and exports a console device to dump >> Linux's own logs to our console. >> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> >> --- >> MAINTAINERS | 1 + >> drivers/tty/Kconfig | 8 + >> drivers/tty/Makefile | 1 + >> drivers/tty/gunyah_tty.c | 409 +++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 419 insertions(+) >> create mode 100644 drivers/tty/gunyah_tty.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index a0cba618e5f6..e8d4a6d9491a 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -8890,6 +8890,7 @@ F: Documentation/devicetree/bindings/firmware/gunyah-hypervisor.yaml >> F: Documentation/virt/gunyah/ >> F: arch/arm64/gunyah/ >> F: drivers/mailbox/gunyah-msgq.c >> +F: drivers/tty/gunyah_tty.c >> F: drivers/virt/gunyah/ >> F: include/asm-generic/gunyah.h >> F: include/linux/gunyah*.h >> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig >> index cc30ff93e2e4..ff86e977f9ac 100644 >> --- a/drivers/tty/Kconfig >> +++ b/drivers/tty/Kconfig >> @@ -380,6 +380,14 @@ config RPMSG_TTY >> To compile this driver as a module, choose M here: the module will be >> called rpmsg_tty. >> >> +config GUNYAH_CONSOLE >> + tristate "Gunyah Consoles" >> + depends on GUNYAH >> + help >> + This enables support for console output using Gunyah's Resource Manager RPC. >> + This is normally used when a secondary VM which does not have exclusive access >> + to a real or virtualized serial device and virtio-console is unavailable. > > module name? > >> + >> endif # TTY >> >> source "drivers/tty/serdev/Kconfig" >> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile >> index 07aca5184a55..d183fbfd835b 100644 >> --- a/drivers/tty/Makefile >> +++ b/drivers/tty/Makefile >> @@ -27,5 +27,6 @@ obj-$(CONFIG_GOLDFISH_TTY) += goldfish.o >> obj-$(CONFIG_MIPS_EJTAG_FDC_TTY) += mips_ejtag_fdc.o >> obj-$(CONFIG_VCC) += vcc.o >> obj-$(CONFIG_RPMSG_TTY) += rpmsg_tty.o >> +obj-$(CONFIG_GUNYAH_CONSOLE) += gunyah_tty.o >> >> obj-y += ipwireless/ >> diff --git a/drivers/tty/gunyah_tty.c b/drivers/tty/gunyah_tty.c >> new file mode 100644 >> index 000000000000..80a20da11ad0 >> --- /dev/null >> +++ b/drivers/tty/gunyah_tty.c >> @@ -0,0 +1,409 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#define pr_fmt(fmt) "gh_rsc_mgr_console: " fmt > > You are a driver, use dev_printk() functions, no need for pr_fmt() at > all, right? > >> + >> +#include <linux/gunyah_rsc_mgr.h> >> +#include <linux/auxiliary_bus.h> >> +#include <linux/workqueue.h> >> +#include <linux/spinlock.h> >> +#include <linux/tty_flip.h> >> +#include <linux/console.h> >> +#include <linux/module.h> >> +#include <linux/kfifo.h> >> +#include <linux/kref.h> >> +#include <linux/slab.h> >> +#include <linux/tty.h> >> + >> +/* >> + * The Linux TTY code does not support dynamic addition of tty derived devices so we need to know >> + * how many tty devices we might need when space is allocated for the tty device. Since VMs might be >> + * added/removed dynamically, we need to make sure we have enough allocated. > > Wrap comments at 80 columns please. > >> + */ >> +#define RSC_MGR_TTY_ADAPTERS 16 > > We can have dynamic tty devices, so I don't understand this comment. > What really is the problem here? > Yes, I see the confusion. Dynamic device addition of tty devices is supported. As I understand, you need to know the maximum number of lines that could be added, and that is limitation I was referring to. Is this comment better? The Linux TTY code requires us to know ahead of time how many lines we might need. Each line here corresponds to a VM. 16 seems like a reasonable number of lines for systems that are running Gunyah and using the provided console interface. >> + >> +/* # of payload bytes that can fit in a 1-fragment CONSOLE_WRITE message */ >> +#define RM_CONS_WRITE_MSG_SIZE ((1 * (GH_MSGQ_MAX_MSG_SIZE - 8)) - 4) >> + >> +struct rm_cons_port { >> + struct tty_port port; >> + u16 vmid; >> + bool open; > > Why do you care if it is open or not? > I can clean it out. >> + unsigned int index; >> + >> + DECLARE_KFIFO(put_fifo, char, 1024); >> + spinlock_t fifo_lock; >> + struct work_struct put_work; >> + >> + struct rm_cons_data *cons_data; >> +}; >> + >> +struct rm_cons_data { >> + struct tty_driver *tty_driver; >> + struct device *dev; >> + >> + spinlock_t ports_lock; >> + struct rm_cons_port *ports[RSC_MGR_TTY_ADAPTERS]; >> + >> + struct notifier_block rsc_mgr_notif; >> + struct console console; >> +}; >> + >> +static void put_work_fn(struct work_struct *ws) >> +{ >> + char buf[RM_CONS_WRITE_MSG_SIZE]; >> + int count, ret; >> + struct rm_cons_port *port = container_of(ws, struct rm_cons_port, put_work); >> + >> + while (!kfifo_is_empty(&port->put_fifo)) { >> + count = kfifo_out_spinlocked(&port->put_fifo, buf, sizeof(buf), &port->fifo_lock); >> + if (count <= 0) >> + continue; >> + >> + ret = gh_rm_console_write(port->vmid, buf, count); >> + if (ret) { >> + pr_warn_once("failed to send characters: %d\n", ret); > > What will this warning help with? > >> + break; > > If an error happens, shouldn't you keep trying to send the rest of the > data? > I'll update to retry on anything but ENOMEM. >> + } >> + } >> +} >> + >> +static int rsc_mgr_console_notif(struct notifier_block *nb, unsigned long cmd, void *data) >> +{ >> + int count, i; >> + struct rm_cons_port *rm_port = NULL; >> + struct tty_port *tty_port = NULL; >> + struct rm_cons_data *cons_data = container_of(nb, struct rm_cons_data, rsc_mgr_notif); >> + const struct gh_rm_notification *notif = data; >> + struct gh_rm_notif_vm_console_chars const * const msg = notif->buff; >> + >> + if (cmd != GH_RM_NOTIF_VM_CONSOLE_CHARS || >> + notif->size < sizeof(*msg)) >> + return NOTIFY_DONE; >> + >> + spin_lock(&cons_data->ports_lock); >> + for (i = 0; i < RSC_MGR_TTY_ADAPTERS; i++) { >> + if (!cons_data->ports[i]) >> + continue; >> + if (cons_data->ports[i]->vmid == msg->vmid) { >> + rm_port = cons_data->ports[i]; >> + break; >> + } >> + } >> + if (rm_port) >> + tty_port = tty_port_get(&rm_port->port); >> + spin_unlock(&cons_data->ports_lock); >> + >> + if (!rm_port) >> + pr_warn("Received unexpected console characters for VMID %u\n", msg->vmid); >> + if (!tty_port) >> + return NOTIFY_DONE; >> + >> + count = tty_buffer_request_room(tty_port, msg->num_bytes); >> + tty_insert_flip_string(tty_port, msg->bytes, count); >> + tty_flip_buffer_push(tty_port); >> + >> + tty_port_put(tty_port); >> + return NOTIFY_OK; >> +} >> + >> +static ssize_t vmid_show(struct device *dev, struct device_attribute *attr, char *buf) >> +{ >> + struct rm_cons_port *rm_port = dev_get_drvdata(dev); >> + >> + if (rm_port->vmid == GH_VMID_SELF) >> + return sysfs_emit(buf, "self\n"); >> + >> + return sysfs_emit(buf, "%u\n", rm_port->vmid); > > You didn't document this sysfs file, why not? > > And tty drivers should not have random sysfs files, please don't add > this. > Removed >> +} >> + >> +static DEVICE_ATTR_RO(vmid); >> + >> +static struct attribute *rsc_mgr_tty_dev_attrs[] = { >> + &dev_attr_vmid.attr, >> + NULL >> +}; >> + >> +static const struct attribute_group rsc_mgr_tty_dev_attr_group = { >> + .attrs = rsc_mgr_tty_dev_attrs, >> +}; >> + >> +static const struct attribute_group *rsc_mgr_tty_dev_attr_groups[] = { >> + &rsc_mgr_tty_dev_attr_group, >> + NULL >> +}; >> + >> +static int rsc_mgr_tty_open(struct tty_struct *tty, struct file *filp) >> +{ >> + int ret; >> + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); >> + >> + if (!rm_port->open) { > > Why are you caring if the port is open already or not? > >> + ret = gh_rm_console_open(rm_port->vmid); > > Can't this just be called for every open()? > > And what happens if this changes right after it is checked? > I've moved the open/close callbacks to the activate/shutdown tty_port_operations. >> + if (ret) { >> + pr_err("Failed to open RM console for vmid %x: %d\n", rm_port->vmid, ret); > > dev_err() > >> + return ret; >> + } >> + rm_port->open = true; >> + } >> + >> + return tty_port_open(&rm_port->port, tty, filp); >> +} >> + >> +static void rsc_mgr_tty_close(struct tty_struct *tty, struct file *filp) >> +{ >> + int ret; >> + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); >> + >> + if (rm_port->open) { >> + if (rm_port->vmid != GH_VMID_SELF) { >> + ret = gh_rm_console_close(rm_port->vmid); >> + if (ret) >> + pr_warn("Failed to close RM console for vmid %d: %d\n", >> + rm_port->vmid, ret); >> + } >> + rm_port->open = false; > > So if you had multiple open/close this would close the console the first > close call, but not the second? > > Are you sure you tested this out properly? > >> + >> + tty_port_close(&rm_port->port, tty, filp); >> + } >> + >> +} >> + >> +static int rsc_mgr_tty_write(struct tty_struct *tty, const unsigned char *buf, int count) >> +{ >> + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); >> + int ret; >> + >> + ret = kfifo_in_spinlocked(&rm_port->put_fifo, buf, count, &rm_port->fifo_lock); >> + if (ret > 0) >> + schedule_work(&rm_port->put_work); > > Why not just do the write here? Why is a work queue needed? > The gh_rm_console_* calls will sleep. console_write can be called in an atomic context, so I put the characters on FIFO. I'll update so that FIFO only used for console. >> + >> + return ret; >> +} >> + >> +static unsigned int rsc_mgr_mgr_tty_write_room(struct tty_struct *tty) >> +{ >> + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); >> + >> + return kfifo_avail(&rm_port->put_fifo); >> +} >> + >> +static void rsc_mgr_console_write(struct console *co, const char *buf, unsigned count) >> +{ >> + struct rm_cons_port *rm_port = co->data; >> + int ret; >> + >> + ret = kfifo_in_spinlocked(&rm_port->put_fifo, buf, count, &rm_port->fifo_lock); >> + if (ret > 0) >> + schedule_work(&rm_port->put_work); > > Same here, why not just send the data now? > >> +} >> + >> +static struct tty_driver *rsc_mgr_console_device(struct console *co, int *index) >> +{ >> + struct rm_cons_port *rm_port = co->data; >> + >> + *index = rm_port->index; >> + return rm_port->port.tty->driver; > > Love the locking :( > >> +} >> + >> +static int rsc_mgr_console_setup(struct console *co, char *unused) >> +{ >> + int ret; >> + struct rm_cons_port *rm_port = co->data; >> + >> + if (!rm_port->open) { >> + ret = gh_rm_console_open(rm_port->vmid); >> + if (ret) { >> + pr_err("Failed to open RM console for vmid %x: %d\n", rm_port->vmid, ret); >> + return ret; >> + } >> + rm_port->open = true; > > Again, don't mess with open/close. > In general, is it acceptable to use tty_port(_set)_initialized in the console_setup/console_exit? static int rsc_mgr_console_setup(struct console *co, char *unused) { struct rm_cons_port *rm_port = co->data; int ret; if (!tty_port_get(&rm_port->port)) return -ENODEV; mutex_lock(&rm_port->port.mutex); if (!tty_port_initialized(&rm_port->port)) { ret = gh_rm_console_open(rm_port->vmid); if (ret) { dev_err(rm_port->port.tty->dev, "Failed to open %s%d: %d\n", co->name, rm_port->index, ret); goto err; } tty_port_set_initialized(&rm_port->port, true); } rm_port->port.console = true; mutex_unlock(&rm_port->port.mutex); return 0; err: mutex_unlock(&rm_port->port.mutex); tty_port_put(&rm_port->port); return ret; } static int rsc_mgr_console_exit(struct console *co) { int ret; struct rm_cons_port *rm_port = co->data; mutex_lock(&rm_port->port.mutex); rm_port->port.console = false; if (!tty_port_active(&rm_port->port)) { ret = gh_rm_console_close(rm_port->vmid); if (ret) dev_err(rm_port->port.tty->dev, "Failed to close %s%d: %d\n", co->name, rm_port->index, ret); tty_port_set_initialized(&rm_port->port, false); } mutex_unlock(&rm_port->port.mutex); tty_port_put(&rm_port->port); return 0; } >> + } >> + >> + return 0; >> +} >> + >> +static int rsc_mgr_console_exit(struct console *co) >> +{ >> + int ret; >> + struct rm_cons_port *rm_port = co->data; >> + >> + if (rm_port->open) { >> + ret = gh_rm_console_close(rm_port->vmid); >> + if (ret) { >> + pr_err("Failed to close RM console for vmid %x: %d\n", rm_port->vmid, ret); >> + return ret; >> + } >> + rm_port->open = false; >> + } >> + >> + return 0; >> +} >> + >> +static const struct tty_operations rsc_mgr_tty_ops = { >> + .open = rsc_mgr_tty_open, >> + .close = rsc_mgr_tty_close, >> + .write = rsc_mgr_tty_write, >> + .write_room = rsc_mgr_mgr_tty_write_room, >> +}; >> + >> +static void rsc_mgr_port_destruct(struct tty_port *port) >> +{ >> + struct rm_cons_port *rm_port = container_of(port, struct rm_cons_port, port); >> + struct rm_cons_data *cons_data = rm_port->cons_data; >> + >> + spin_lock(&cons_data->ports_lock); >> + WARN_ON(cons_data->ports[rm_port->index] != rm_port); > > Does this mean you just crashed the system if something went wrong? > > How can this ever happen? > > This can't happen and was added defensively. Will drop. >> + cons_data->ports[rm_port->index] = NULL; >> + spin_unlock(&cons_data->ports_lock); >> + kfree(rm_port); >> +} >> + >> +static const struct tty_port_operations rsc_mgr_port_ops = { >> + .destruct = rsc_mgr_port_destruct, >> +}; >> + >> +static struct rm_cons_port *rsc_mgr_port_create(struct rm_cons_data *cons_data, u16 vmid) >> +{ >> + struct rm_cons_port *rm_port; >> + struct device *ttydev; >> + unsigned int index; >> + int ret; >> + >> + rm_port = kzalloc(sizeof(*rm_port), GFP_KERNEL); >> + rm_port->vmid = vmid; >> + INIT_KFIFO(rm_port->put_fifo); >> + spin_lock_init(&rm_port->fifo_lock); >> + INIT_WORK(&rm_port->put_work, put_work_fn); >> + tty_port_init(&rm_port->port); >> + rm_port->port.ops = &rsc_mgr_port_ops; >> + >> + spin_lock(&cons_data->ports_lock); >> + for (index = 0; index < RSC_MGR_TTY_ADAPTERS; index++) { >> + if (!cons_data->ports[index]) { >> + cons_data->ports[index] = rm_port; >> + rm_port->index = index; >> + break; >> + } >> + } >> + spin_unlock(&cons_data->ports_lock); >> + if (index >= RSC_MGR_TTY_ADAPTERS) { >> + ret = -ENOSPC; >> + goto err_put_port; >> + } >> + >> + ttydev = tty_port_register_device_attr(&rm_port->port, cons_data->tty_driver, index, >> + cons_data->dev, rm_port, rsc_mgr_tty_dev_attr_groups); >> + if (IS_ERR(ttydev)) { >> + ret = PTR_ERR(ttydev); >> + goto err_put_port; >> + } >> + >> + return rm_port; >> +err_put_port: >> + tty_port_put(&rm_port->port); >> + return ERR_PTR(ret); >> +} >> + >> +static int rsc_mgr_console_probe(struct auxiliary_device *auxdev, >> + const struct auxiliary_device_id *aux_dev_id) >> +{ >> + struct rm_cons_data *cons_data; >> + struct rm_cons_port *rm_port; >> + int ret; >> + u16 vmid; >> + >> + cons_data = devm_kzalloc(&auxdev->dev, sizeof(*cons_data), GFP_KERNEL); >> + if (!cons_data) >> + return -ENOMEM; >> + dev_set_drvdata(&auxdev->dev, cons_data); >> + cons_data->dev = &auxdev->dev; >> + >> + cons_data->tty_driver = tty_alloc_driver(RSC_MGR_TTY_ADAPTERS, >> + TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV); >> + if (IS_ERR(cons_data->tty_driver)) >> + return PTR_ERR(cons_data->tty_driver); >> + >> + cons_data->tty_driver->driver_name = "gh"; >> + cons_data->tty_driver->name = "ttyGH"; > > Where did you pick this name from? > > Where is it documented? > "GH" is the shorthand we've been using for "Gunyah". I didn't find documentation for dynamically assigned char devices, but if it exists, I can add entry for ttyGH. >> + cons_data->tty_driver->type = TTY_DRIVER_TYPE_SYSTEM; >> + cons_data->tty_driver->init_termios = tty_std_termios; >> + tty_set_operations(cons_data->tty_driver, &rsc_mgr_tty_ops); >> + >> + ret = tty_register_driver(cons_data->tty_driver); >> + if (ret) { >> + dev_err(&auxdev->dev, "Could not register tty driver: %d\n", ret); >> + goto err_put_tty; >> + } >> + >> + spin_lock_init(&cons_data->ports_lock); >> + >> + cons_data->rsc_mgr_notif.notifier_call = rsc_mgr_console_notif; >> + ret = gh_rm_register_notifier(&cons_data->rsc_mgr_notif); >> + if (ret) { >> + dev_err(&auxdev->dev, "Could not register for resource manager notifications: %d\n", >> + ret); >> + goto err_put_tty; >> + } >> + >> + rm_port = rsc_mgr_port_create(cons_data, GH_VMID_SELF); >> + if (IS_ERR(rm_port)) { >> + ret = PTR_ERR(rm_port); >> + dev_err(&auxdev->dev, "Could not create own console: %d\n", ret); >> + goto err_unreg_notif; >> + } >> + >> + strncpy(cons_data->console.name, "ttyGH", sizeof(cons_data->console.name)); > > Again, where did this name come from? > > thanks, > > greg k-h
On Thu, Oct 06, 2022 at 10:59:51PM -0700, Elliot Berman wrote: > > > + */ > > > +#define RSC_MGR_TTY_ADAPTERS 16 > > > > We can have dynamic tty devices, so I don't understand this comment. > > What really is the problem here? > > > > Yes, I see the confusion. Dynamic device addition of tty devices is > supported. As I understand, you need to know the maximum number of lines > that could be added, and that is limitation I was referring to. What do you mean by "lines"? That's not a tty kernel term. > Is this comment better? > > The Linux TTY code requires us to know ahead of time how many lines we > might need. Each line here corresponds to a VM. 16 seems like a > reasonable number of lines for systems that are running Gunyah and using > the provided console interface. Again, line? Do you mean port? > > > + cons_data->tty_driver->driver_name = "gh"; KBUILD_MODNAME? > > > + cons_data->tty_driver->name = "ttyGH"; > > > > Where did you pick this name from? > > > > Where is it documented? > > > > "GH" is the shorthand we've been using for "Gunyah". I didn't find > documentation for dynamically assigned char devices, but if it exists, I can > add entry for ttyGH. Why use a new name at all? Why not stick with the existing tty names and device numbers? thanks, greg k-h
>> + >> +/* >> + * The Linux TTY code does not support dynamic addition of tty derived devices so we need to know >> + * how many tty devices we might need when space is allocated for the tty device. Since VMs might be >> + * added/removed dynamically, we need to make sure we have enough allocated. > > Wrap comments at 80 columns please. > checkpatch has extend LINE MAX to 100,do we still suggest wrap to 80?
On Sat, Oct 08, 2022 at 03:41:53PM +0800, Zhou Furong wrote: > > > > + > > > +/* > > > + * The Linux TTY code does not support dynamic addition of tty derived devices so we need to know > > > + * how many tty devices we might need when space is allocated for the tty device. Since VMs might be > > > + * added/removed dynamically, we need to make sure we have enough allocated. > > > > Wrap comments at 80 columns please. > > > > checkpatch has extend LINE MAX to 100,do we still suggest wrap to 80? For comment blocks, yes please.
On 10/7/2022 12:40 AM, Greg Kroah-Hartman wrote: > On Thu, Oct 06, 2022 at 10:59:51PM -0700, Elliot Berman wrote: >> >> "GH" is the shorthand we've been using for "Gunyah". I didn't find >> documentation for dynamically assigned char devices, but if it exists, I can >> add entry for ttyGH. > > Why use a new name at all? Why not stick with the existing tty names > and device numbers? > I can use hvc framework, although driver-level buffering is needed on both the get_chars/put_chars paths because: - get_chars wants to poll for characters, but Gunyah will push characters to Linux - put_chars can be called in atomic context in the printk console path. Gunyah RM calls can sleep, so we add to buffer and queue work to write the characters. I also chose to use new tty driver because the Gunyah hypervisor call to open the console (gh_rm_console_open) can only be done after starting the VM. Gunyah will only forward characters sent from the other VM to Linux after the gh_rm_console_open call is made. When launching a VM, users would want to open console before VM starts so they can get startup messages from the VM. I planned to use the carrier_raised() to hold tty_port_block_until_ready until the VM is started and the gh_rm_console_open() happens. Thanks, Elliot
On 10/3/2022 12:01 AM, Jiri Slaby wrote: > On 28. 09. 22, 21:56, Elliot Berman wrote: >> +struct rm_cons_port { >> + struct tty_port port; >> + u16 vmid; >> + bool open; >> + unsigned int index; >> + >> + DECLARE_KFIFO(put_fifo, char, 1024); > > Why is tty_port::xmit_fifo not enough? > xmit_fifo gave me some inspiration to avoid using KFIFO here and skip extra an extra memcpy into/out of the FIFO. I've also dropped out the FIFO usage for tty_operations and this buffering is only used for printk console. Thanks, Elliot
On Sun, Oct 09, 2022 at 01:59:21PM -0700, Elliot Berman wrote: > > > On 10/7/2022 12:40 AM, Greg Kroah-Hartman wrote: > > On Thu, Oct 06, 2022 at 10:59:51PM -0700, Elliot Berman wrote: > > > > > > "GH" is the shorthand we've been using for "Gunyah". I didn't find > > > documentation for dynamically assigned char devices, but if it exists, I can > > > add entry for ttyGH. > > > > Why use a new name at all? Why not stick with the existing tty names > > and device numbers? > > > > I can use hvc framework, although driver-level buffering is needed on > both the get_chars/put_chars paths because: I'm not asking about the framework (although that is a good question, you need to document why this has to be new.) I'm asking why pick a new name? You will not have a name conflict in your system with this device with any other tty name right? > - get_chars wants to poll for characters, but Gunyah will push > characters to Linux > - put_chars can be called in atomic context in the printk console path. > Gunyah RM calls can sleep, so we add to buffer and queue work to > write the characters. > > I also chose to use new tty driver because the Gunyah hypervisor call to > open the console (gh_rm_console_open) can only be done after starting the > VM. Gunyah will only forward characters sent from the other VM to Linux > after the gh_rm_console_open call is made. When launching a VM, users would > want to open console before VM starts so they can get startup messages from > the VM. I planned to use the carrier_raised() to hold > tty_port_block_until_ready until the VM is started and the > gh_rm_console_open() happens. I'm sorry, but I don't understand this. Why is this all a new api at all? What about the virtio api? Why not just use that instead? thanks, greg k-h
On 10/10/2022 1:23 PM, Greg Kroah-Hartman wrote: > On Sun, Oct 09, 2022 at 01:59:21PM -0700, Elliot Berman wrote: >> >> >> On 10/7/2022 12:40 AM, Greg Kroah-Hartman wrote: >>> On Thu, Oct 06, 2022 at 10:59:51PM -0700, Elliot Berman wrote: >>>> >>>> "GH" is the shorthand we've been using for "Gunyah". I didn't find >>>> documentation for dynamically assigned char devices, but if it exists, I can >>>> add entry for ttyGH. >>> >>> Why use a new name at all? Why not stick with the existing tty names >>> and device numbers? >>> >> >> I can use hvc framework, although driver-level buffering is needed on >> both the get_chars/put_chars paths because: > > I'm not asking about the framework (although that is a good question, > you need to document why this has to be new.) I'm asking why pick a new > name? You will not have a name conflict in your system with this device > with any other tty name right? > That's correct, I didn't see any other instances of "ttyGH" in kernel. >> - get_chars wants to poll for characters, but Gunyah will push >> characters to Linux >> - put_chars can be called in atomic context in the printk console path. >> Gunyah RM calls can sleep, so we add to buffer and queue work to >> write the characters. >> >> I also chose to use new tty driver because the Gunyah hypervisor call to >> open the console (gh_rm_console_open) can only be done after starting the >> VM. Gunyah will only forward characters sent from the other VM to Linux >> after the gh_rm_console_open call is made. When launching a VM, users would >> want to open console before VM starts so they can get startup messages from >> the VM. I planned to use the carrier_raised() to hold >> tty_port_block_until_ready until the VM is started and the >> gh_rm_console_open() happens. > > I'm sorry, but I don't understand this. > > Why is this all a new api at all? What about the virtio api? Why not > just use that instead? We want to support virtual machines and Virtual Machine Managers (the userspace component) that don't support virtio. Qualcomm has some lightweight VMs that have a Gunyah driver stack but no virtio stack. Further, implementing a simple userspace VMM to launch a Linux kernel is much easier with the Gunyah console as no device paravirtualization is needed to get some output from Linux. I don't anticipate these VMs to be commonplace, but they do exist. Thanks, Elliot
On Mon, Oct 10, 2022 at 01:58:00PM -0700, Elliot Berman wrote: > > > On 10/10/2022 1:23 PM, Greg Kroah-Hartman wrote: > > On Sun, Oct 09, 2022 at 01:59:21PM -0700, Elliot Berman wrote: > > > > > > > > > On 10/7/2022 12:40 AM, Greg Kroah-Hartman wrote: > > > > On Thu, Oct 06, 2022 at 10:59:51PM -0700, Elliot Berman wrote: > > > > > > > > > > "GH" is the shorthand we've been using for "Gunyah". I didn't find > > > > > documentation for dynamically assigned char devices, but if it exists, I can > > > > > add entry for ttyGH. > > > > > > > > Why use a new name at all? Why not stick with the existing tty names > > > > and device numbers? > > > > > > > > > > I can use hvc framework, although driver-level buffering is needed on > > > both the get_chars/put_chars paths because: > > > > I'm not asking about the framework (although that is a good question, > > you need to document why this has to be new.) I'm asking why pick a new > > name? You will not have a name conflict in your system with this device > > with any other tty name right? > > > > That's correct, I didn't see any other instances of "ttyGH" in kernel. Do you see any instances of ttyS? Any other existing name? Why pick a new name at all? And if you do pick a new name, you need to document it really really well, not bury it downn within the tty driver code. thanks, greg k-h
On 10/10/2022 11:22 PM, Greg Kroah-Hartman wrote: > On Mon, Oct 10, 2022 at 01:58:00PM -0700, Elliot Berman wrote: >> >> >> On 10/10/2022 1:23 PM, Greg Kroah-Hartman wrote: >>> On Sun, Oct 09, 2022 at 01:59:21PM -0700, Elliot Berman wrote: >>>> >>>>>>> On 10/7/2022 12:40 AM, Greg Kroah-Hartman wrote: >>>>> On Thu, Oct 06, 2022 at 10:59:51PM -0700, Elliot Berman wrote: >>>>>> >>>>>> "GH" is the shorthand we've been using for "Gunyah". I didn't find >>>>>> documentation for dynamically assigned char devices, but if it exists, I can >>>>>> add entry for ttyGH. >>>>> >>>>> Why use a new name at all? Why not stick with the existing tty names >>>>> and device numbers? >>>>> >>>> >>>> I can use hvc framework, although driver-level buffering is needed on >>>> both the get_chars/put_chars paths because: >>> >>> I'm not asking about the framework (although that is a good question, >>> you need to document why this has to be new.) I'm asking why pick a new >>> name? You will not have a name conflict in your system with this device >>> with any other tty name right? >>> >> >> That's correct, I didn't see any other instances of "ttyGH" in kernel. > > Do you see any instances of ttyS? Any other existing name? Why pick a > new name at all? > > And if you do pick a new name, you need to document it really really > well, not bury it downn within the tty driver code. > Seems other drivers are adding a comment in Kconfig help text. I can do the same.
On Tue, Oct 11, 2022 at 03:04:08PM -0700, Elliot Berman wrote: > > > On 10/10/2022 11:22 PM, Greg Kroah-Hartman wrote: > > On Mon, Oct 10, 2022 at 01:58:00PM -0700, Elliot Berman wrote: > > > > > > > > > On 10/10/2022 1:23 PM, Greg Kroah-Hartman wrote: > > > > On Sun, Oct 09, 2022 at 01:59:21PM -0700, Elliot Berman wrote: > > > > > > > > > > > > > On 10/7/2022 12:40 AM, Greg Kroah-Hartman wrote: > > > > > > On Thu, Oct 06, 2022 at 10:59:51PM -0700, Elliot Berman wrote: > > > > > > > > > > > > > > "GH" is the shorthand we've been using for "Gunyah". I didn't find > > > > > > > documentation for dynamically assigned char devices, but if it exists, I can > > > > > > > add entry for ttyGH. > > > > > > > > > > > > Why use a new name at all? Why not stick with the existing tty names > > > > > > and device numbers? > > > > > > > > > > > > > > > > I can use hvc framework, although driver-level buffering is needed on > > > > > both the get_chars/put_chars paths because: > > > > > > > > I'm not asking about the framework (although that is a good question, > > > > you need to document why this has to be new.) I'm asking why pick a new > > > > name? You will not have a name conflict in your system with this device > > > > with any other tty name right? > > > > > > > > > > That's correct, I didn't see any other instances of "ttyGH" in kernel. > > > > Do you see any instances of ttyS? Any other existing name? Why pick a > > new name at all? > > > > And if you do pick a new name, you need to document it really really > > well, not bury it downn within the tty driver code. > > > > Seems other drivers are adding a comment in Kconfig help text. I can do the > same. If you really want this, yes. But again, you have not justified a whole new name yet...
diff --git a/MAINTAINERS b/MAINTAINERS index a0cba618e5f6..e8d4a6d9491a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8890,6 +8890,7 @@ F: Documentation/devicetree/bindings/firmware/gunyah-hypervisor.yaml F: Documentation/virt/gunyah/ F: arch/arm64/gunyah/ F: drivers/mailbox/gunyah-msgq.c +F: drivers/tty/gunyah_tty.c F: drivers/virt/gunyah/ F: include/asm-generic/gunyah.h F: include/linux/gunyah*.h diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig index cc30ff93e2e4..ff86e977f9ac 100644 --- a/drivers/tty/Kconfig +++ b/drivers/tty/Kconfig @@ -380,6 +380,14 @@ config RPMSG_TTY To compile this driver as a module, choose M here: the module will be called rpmsg_tty. +config GUNYAH_CONSOLE + tristate "Gunyah Consoles" + depends on GUNYAH + help + This enables support for console output using Gunyah's Resource Manager RPC. + This is normally used when a secondary VM which does not have exclusive access + to a real or virtualized serial device and virtio-console is unavailable. + endif # TTY source "drivers/tty/serdev/Kconfig" diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile index 07aca5184a55..d183fbfd835b 100644 --- a/drivers/tty/Makefile +++ b/drivers/tty/Makefile @@ -27,5 +27,6 @@ obj-$(CONFIG_GOLDFISH_TTY) += goldfish.o obj-$(CONFIG_MIPS_EJTAG_FDC_TTY) += mips_ejtag_fdc.o obj-$(CONFIG_VCC) += vcc.o obj-$(CONFIG_RPMSG_TTY) += rpmsg_tty.o +obj-$(CONFIG_GUNYAH_CONSOLE) += gunyah_tty.o obj-y += ipwireless/ diff --git a/drivers/tty/gunyah_tty.c b/drivers/tty/gunyah_tty.c new file mode 100644 index 000000000000..80a20da11ad0 --- /dev/null +++ b/drivers/tty/gunyah_tty.c @@ -0,0 +1,409 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#define pr_fmt(fmt) "gh_rsc_mgr_console: " fmt + +#include <linux/gunyah_rsc_mgr.h> +#include <linux/auxiliary_bus.h> +#include <linux/workqueue.h> +#include <linux/spinlock.h> +#include <linux/tty_flip.h> +#include <linux/console.h> +#include <linux/module.h> +#include <linux/kfifo.h> +#include <linux/kref.h> +#include <linux/slab.h> +#include <linux/tty.h> + +/* + * The Linux TTY code does not support dynamic addition of tty derived devices so we need to know + * how many tty devices we might need when space is allocated for the tty device. Since VMs might be + * added/removed dynamically, we need to make sure we have enough allocated. + */ +#define RSC_MGR_TTY_ADAPTERS 16 + +/* # of payload bytes that can fit in a 1-fragment CONSOLE_WRITE message */ +#define RM_CONS_WRITE_MSG_SIZE ((1 * (GH_MSGQ_MAX_MSG_SIZE - 8)) - 4) + +struct rm_cons_port { + struct tty_port port; + u16 vmid; + bool open; + unsigned int index; + + DECLARE_KFIFO(put_fifo, char, 1024); + spinlock_t fifo_lock; + struct work_struct put_work; + + struct rm_cons_data *cons_data; +}; + +struct rm_cons_data { + struct tty_driver *tty_driver; + struct device *dev; + + spinlock_t ports_lock; + struct rm_cons_port *ports[RSC_MGR_TTY_ADAPTERS]; + + struct notifier_block rsc_mgr_notif; + struct console console; +}; + +static void put_work_fn(struct work_struct *ws) +{ + char buf[RM_CONS_WRITE_MSG_SIZE]; + int count, ret; + struct rm_cons_port *port = container_of(ws, struct rm_cons_port, put_work); + + while (!kfifo_is_empty(&port->put_fifo)) { + count = kfifo_out_spinlocked(&port->put_fifo, buf, sizeof(buf), &port->fifo_lock); + if (count <= 0) + continue; + + ret = gh_rm_console_write(port->vmid, buf, count); + if (ret) { + pr_warn_once("failed to send characters: %d\n", ret); + break; + } + } +} + +static int rsc_mgr_console_notif(struct notifier_block *nb, unsigned long cmd, void *data) +{ + int count, i; + struct rm_cons_port *rm_port = NULL; + struct tty_port *tty_port = NULL; + struct rm_cons_data *cons_data = container_of(nb, struct rm_cons_data, rsc_mgr_notif); + const struct gh_rm_notification *notif = data; + struct gh_rm_notif_vm_console_chars const * const msg = notif->buff; + + if (cmd != GH_RM_NOTIF_VM_CONSOLE_CHARS || + notif->size < sizeof(*msg)) + return NOTIFY_DONE; + + spin_lock(&cons_data->ports_lock); + for (i = 0; i < RSC_MGR_TTY_ADAPTERS; i++) { + if (!cons_data->ports[i]) + continue; + if (cons_data->ports[i]->vmid == msg->vmid) { + rm_port = cons_data->ports[i]; + break; + } + } + if (rm_port) + tty_port = tty_port_get(&rm_port->port); + spin_unlock(&cons_data->ports_lock); + + if (!rm_port) + pr_warn("Received unexpected console characters for VMID %u\n", msg->vmid); + if (!tty_port) + return NOTIFY_DONE; + + count = tty_buffer_request_room(tty_port, msg->num_bytes); + tty_insert_flip_string(tty_port, msg->bytes, count); + tty_flip_buffer_push(tty_port); + + tty_port_put(tty_port); + return NOTIFY_OK; +} + +static ssize_t vmid_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct rm_cons_port *rm_port = dev_get_drvdata(dev); + + if (rm_port->vmid == GH_VMID_SELF) + return sysfs_emit(buf, "self\n"); + + return sysfs_emit(buf, "%u\n", rm_port->vmid); +} + +static DEVICE_ATTR_RO(vmid); + +static struct attribute *rsc_mgr_tty_dev_attrs[] = { + &dev_attr_vmid.attr, + NULL +}; + +static const struct attribute_group rsc_mgr_tty_dev_attr_group = { + .attrs = rsc_mgr_tty_dev_attrs, +}; + +static const struct attribute_group *rsc_mgr_tty_dev_attr_groups[] = { + &rsc_mgr_tty_dev_attr_group, + NULL +}; + +static int rsc_mgr_tty_open(struct tty_struct *tty, struct file *filp) +{ + int ret; + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); + + if (!rm_port->open) { + ret = gh_rm_console_open(rm_port->vmid); + if (ret) { + pr_err("Failed to open RM console for vmid %x: %d\n", rm_port->vmid, ret); + return ret; + } + rm_port->open = true; + } + + return tty_port_open(&rm_port->port, tty, filp); +} + +static void rsc_mgr_tty_close(struct tty_struct *tty, struct file *filp) +{ + int ret; + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); + + if (rm_port->open) { + if (rm_port->vmid != GH_VMID_SELF) { + ret = gh_rm_console_close(rm_port->vmid); + if (ret) + pr_warn("Failed to close RM console for vmid %d: %d\n", + rm_port->vmid, ret); + } + rm_port->open = false; + + tty_port_close(&rm_port->port, tty, filp); + } + +} + +static int rsc_mgr_tty_write(struct tty_struct *tty, const unsigned char *buf, int count) +{ + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); + int ret; + + ret = kfifo_in_spinlocked(&rm_port->put_fifo, buf, count, &rm_port->fifo_lock); + if (ret > 0) + schedule_work(&rm_port->put_work); + + return ret; +} + +static unsigned int rsc_mgr_mgr_tty_write_room(struct tty_struct *tty) +{ + struct rm_cons_port *rm_port = dev_get_drvdata(tty->dev); + + return kfifo_avail(&rm_port->put_fifo); +} + +static void rsc_mgr_console_write(struct console *co, const char *buf, unsigned count) +{ + struct rm_cons_port *rm_port = co->data; + int ret; + + ret = kfifo_in_spinlocked(&rm_port->put_fifo, buf, count, &rm_port->fifo_lock); + if (ret > 0) + schedule_work(&rm_port->put_work); +} + +static struct tty_driver *rsc_mgr_console_device(struct console *co, int *index) +{ + struct rm_cons_port *rm_port = co->data; + + *index = rm_port->index; + return rm_port->port.tty->driver; +} + +static int rsc_mgr_console_setup(struct console *co, char *unused) +{ + int ret; + struct rm_cons_port *rm_port = co->data; + + if (!rm_port->open) { + ret = gh_rm_console_open(rm_port->vmid); + if (ret) { + pr_err("Failed to open RM console for vmid %x: %d\n", rm_port->vmid, ret); + return ret; + } + rm_port->open = true; + } + + return 0; +} + +static int rsc_mgr_console_exit(struct console *co) +{ + int ret; + struct rm_cons_port *rm_port = co->data; + + if (rm_port->open) { + ret = gh_rm_console_close(rm_port->vmid); + if (ret) { + pr_err("Failed to close RM console for vmid %x: %d\n", rm_port->vmid, ret); + return ret; + } + rm_port->open = false; + } + + return 0; +} + +static const struct tty_operations rsc_mgr_tty_ops = { + .open = rsc_mgr_tty_open, + .close = rsc_mgr_tty_close, + .write = rsc_mgr_tty_write, + .write_room = rsc_mgr_mgr_tty_write_room, +}; + +static void rsc_mgr_port_destruct(struct tty_port *port) +{ + struct rm_cons_port *rm_port = container_of(port, struct rm_cons_port, port); + struct rm_cons_data *cons_data = rm_port->cons_data; + + spin_lock(&cons_data->ports_lock); + WARN_ON(cons_data->ports[rm_port->index] != rm_port); + cons_data->ports[rm_port->index] = NULL; + spin_unlock(&cons_data->ports_lock); + kfree(rm_port); +} + +static const struct tty_port_operations rsc_mgr_port_ops = { + .destruct = rsc_mgr_port_destruct, +}; + +static struct rm_cons_port *rsc_mgr_port_create(struct rm_cons_data *cons_data, u16 vmid) +{ + struct rm_cons_port *rm_port; + struct device *ttydev; + unsigned int index; + int ret; + + rm_port = kzalloc(sizeof(*rm_port), GFP_KERNEL); + rm_port->vmid = vmid; + INIT_KFIFO(rm_port->put_fifo); + spin_lock_init(&rm_port->fifo_lock); + INIT_WORK(&rm_port->put_work, put_work_fn); + tty_port_init(&rm_port->port); + rm_port->port.ops = &rsc_mgr_port_ops; + + spin_lock(&cons_data->ports_lock); + for (index = 0; index < RSC_MGR_TTY_ADAPTERS; index++) { + if (!cons_data->ports[index]) { + cons_data->ports[index] = rm_port; + rm_port->index = index; + break; + } + } + spin_unlock(&cons_data->ports_lock); + if (index >= RSC_MGR_TTY_ADAPTERS) { + ret = -ENOSPC; + goto err_put_port; + } + + ttydev = tty_port_register_device_attr(&rm_port->port, cons_data->tty_driver, index, + cons_data->dev, rm_port, rsc_mgr_tty_dev_attr_groups); + if (IS_ERR(ttydev)) { + ret = PTR_ERR(ttydev); + goto err_put_port; + } + + return rm_port; +err_put_port: + tty_port_put(&rm_port->port); + return ERR_PTR(ret); +} + +static int rsc_mgr_console_probe(struct auxiliary_device *auxdev, + const struct auxiliary_device_id *aux_dev_id) +{ + struct rm_cons_data *cons_data; + struct rm_cons_port *rm_port; + int ret; + u16 vmid; + + cons_data = devm_kzalloc(&auxdev->dev, sizeof(*cons_data), GFP_KERNEL); + if (!cons_data) + return -ENOMEM; + dev_set_drvdata(&auxdev->dev, cons_data); + cons_data->dev = &auxdev->dev; + + cons_data->tty_driver = tty_alloc_driver(RSC_MGR_TTY_ADAPTERS, + TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV); + if (IS_ERR(cons_data->tty_driver)) + return PTR_ERR(cons_data->tty_driver); + + cons_data->tty_driver->driver_name = "gh"; + cons_data->tty_driver->name = "ttyGH"; + cons_data->tty_driver->type = TTY_DRIVER_TYPE_SYSTEM; + cons_data->tty_driver->init_termios = tty_std_termios; + tty_set_operations(cons_data->tty_driver, &rsc_mgr_tty_ops); + + ret = tty_register_driver(cons_data->tty_driver); + if (ret) { + dev_err(&auxdev->dev, "Could not register tty driver: %d\n", ret); + goto err_put_tty; + } + + spin_lock_init(&cons_data->ports_lock); + + cons_data->rsc_mgr_notif.notifier_call = rsc_mgr_console_notif; + ret = gh_rm_register_notifier(&cons_data->rsc_mgr_notif); + if (ret) { + dev_err(&auxdev->dev, "Could not register for resource manager notifications: %d\n", + ret); + goto err_put_tty; + } + + rm_port = rsc_mgr_port_create(cons_data, GH_VMID_SELF); + if (IS_ERR(rm_port)) { + ret = PTR_ERR(rm_port); + dev_err(&auxdev->dev, "Could not create own console: %d\n", ret); + goto err_unreg_notif; + } + + strncpy(cons_data->console.name, "ttyGH", sizeof(cons_data->console.name)); + cons_data->console.write = rsc_mgr_console_write; + cons_data->console.device = rsc_mgr_console_device; + cons_data->console.setup = rsc_mgr_console_setup; + cons_data->console.exit = rsc_mgr_console_exit; + cons_data->console.index = rm_port->index; + cons_data->console.data = rm_port; + register_console(&cons_data->console); + + ret = gh_rm_get_vmid(&vmid); + if (!ret) { + rm_port = rsc_mgr_port_create(cons_data, vmid); + if (IS_ERR(rm_port)) + dev_warn(&auxdev->dev, "Could not create loop-back console: %ld\n", + PTR_ERR(rm_port)); + } else { + dev_warn(&auxdev->dev, "Failed to get this VM's VMID: %d. Not creating loop-back console\n", + ret); + } + + return 0; +err_unreg_notif: + gh_rm_unregister_notifier(&cons_data->rsc_mgr_notif); +err_put_tty: + tty_driver_kref_put(cons_data->tty_driver); + return ret; +} + +static void rsc_mgr_console_remove(struct auxiliary_device *auxdev) +{ + struct rm_cons_data *cons_data = dev_get_drvdata(&auxdev->dev); + + unregister_console(&cons_data->console); + gh_rm_unregister_notifier(&cons_data->rsc_mgr_notif); + tty_driver_kref_put(cons_data->tty_driver); +} + +static struct auxiliary_device_id rsc_mgr_console_ids[] = { + { .name = "gunyah_rsc_mgr.console" }, + {} +}; +MODULE_DEVICE_TABLE(auxiliary, rsc_mgr_console_ids); + +static struct auxiliary_driver rsc_mgr_console_drv = { + .probe = rsc_mgr_console_probe, + .remove = rsc_mgr_console_remove, + .id_table = rsc_mgr_console_ids, +}; +module_auxiliary_driver(rsc_mgr_console_drv); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Gunyah Console");
Gunyah provides a console for each VM using the VM console resource manager APIs. This driver allows console data from other VMs to be accessed via a TTY device and exports a console device to dump Linux's own logs to our console. Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> --- MAINTAINERS | 1 + drivers/tty/Kconfig | 8 + drivers/tty/Makefile | 1 + drivers/tty/gunyah_tty.c | 409 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 419 insertions(+) create mode 100644 drivers/tty/gunyah_tty.c