diff mbox series

[v4,14/14] tty: gunyah: Add tty console driver for RM Console Services

Message ID 20220928195633.2348848-15-quic_eberman@quicinc.com
State New
Headers show
Series Drivers for gunyah hypervisor | expand

Commit Message

Elliot Berman Sept. 28, 2022, 7:56 p.m. UTC
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

Comments

Elliot Berman Oct. 7, 2022, 5:59 a.m. UTC | #1
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
Greg KH Oct. 7, 2022, 7:40 a.m. UTC | #2
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
Zhou Furong Oct. 8, 2022, 7:41 a.m. UTC | #3
>> +
>> +/*
>> + * 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?
Greg KH Oct. 8, 2022, 9:52 a.m. UTC | #4
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.
Greg KH Oct. 10, 2022, 8:23 p.m. UTC | #5
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
Elliot Berman Oct. 10, 2022, 8:58 p.m. UTC | #6
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
diff mbox series

Patch

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");