Message-ID: <20251225-nippy-umber-inchworm-ced095@quoll>
Date: Thu, 25 Dec 2025 10:02:15 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: joaopeixoto@...x.tech
Cc: linux-kernel@...r.kernel.org, ajd@...ux.ibm.com, alex@...ti.fr, 
	aou@...s.berkeley.edu, bagasdotme@...il.com, catalin.marinas@....com, 
	conor+dt@...nel.org, corbet@....net, dan.j.williams@...el.com, 
	davidmcerdeira@...x.tech, devicetree@...r.kernel.org, dev@...l-k.io, 
	gregkh@...uxfoundation.org, haren@...ux.ibm.com, heiko@...ech.de, jose@...x.tech, 
	kever.yang@...k-chips.com, krzk+dt@...nel.org, linux-arm-kernel@...ts.infradead.org, 
	linux@...linux.org.uk, linux-doc@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	maddy@...ux.ibm.com, mani@...nel.org, nathan@...nel.org, neil.armstrong@...aro.org, 
	palmer@...belt.com, pjw@...nel.org, prabhakar.mahadev-lad.rj@...renesas.com, 
	robh@...nel.org, will@...nel.org
Subject: Re: [PATCH 2/5] virt: add Bao IPC shared memory driver

On Wed, Dec 24, 2025 at 01:52:14PM +0000, joaopeixoto@...x.tech wrote:
> From: João Peixoto <joaopeixoto@...x.tech>
> 
> Add a new driver providing an interface for communication with guests
> hosted by the Bao hypervisor using shared-memory channels. The driver
> exposes read/write regions defined in device tree and notifies the
> hypervisor via an architecture-specific hypercall (SMC/HVC on ARM and
> SBI ecall on RISC-V).
> 
> The patch introduces:
>   - drivers/bao/ with the initial Bao IPC shared-memory implementation
>   - Kconfig entry enabling BAO_SHMEM
>   - Makefile integration for building the driver
>   - A character device interface supporting mmap(), read(), and write()
>   - Platform driver support using DT properties for channel layout
> 
> Each device instance maps its assigned shared-memory region, validates
> read/write channel configuration, and exposes a /dev/baoipc<N> node
> used by user space to exchange data with Bao guests.
> 
> Signed-off-by: João Peixoto <joaopeixoto@...x.tech>
> ---
>  drivers/virt/Kconfig                 |   2 +
>  drivers/virt/Makefile                |   1 +
>  drivers/virt/bao/Kconfig             |   3 +
>  drivers/virt/bao/Makefile            |   3 +
>  drivers/virt/bao/ipcshmem/Kconfig    |   9 +
>  drivers/virt/bao/ipcshmem/Makefile   |   3 +
>  drivers/virt/bao/ipcshmem/ipcshmem.c | 539 +++++++++++++++++++++++++++
>  7 files changed, 560 insertions(+)
>  create mode 100644 drivers/virt/bao/Kconfig
>  create mode 100644 drivers/virt/bao/Makefile
>  create mode 100644 drivers/virt/bao/ipcshmem/Kconfig
>  create mode 100644 drivers/virt/bao/ipcshmem/Makefile
>  create mode 100644 drivers/virt/bao/ipcshmem/ipcshmem.c
> 
> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> index 52eb7e4ba71f..cb98c4c52fd1 100644
> --- a/drivers/virt/Kconfig
> +++ b/drivers/virt/Kconfig
> @@ -47,6 +47,8 @@ source "drivers/virt/nitro_enclaves/Kconfig"
>  
>  source "drivers/virt/acrn/Kconfig"
>  
> +source "drivers/virt/bao/Kconfig"
> +
>  endif
>  
>  source "drivers/virt/coco/Kconfig"
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index f29901bd7820..623a671f8711 100644
> --- a/drivers/virt/Makefile
> +++ b/drivers/virt/Makefile
> @@ -10,3 +10,4 @@ obj-y				+= vboxguest/
>  obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
>  obj-$(CONFIG_ACRN_HSM)		+= acrn/
>  obj-y				+= coco/
> +obj-$(CONFIG_BAO_SHMEM)		+= bao/
> diff --git a/drivers/virt/bao/Kconfig b/drivers/virt/bao/Kconfig
> new file mode 100644
> index 000000000000..4f7929d57475
> --- /dev/null
> +++ b/drivers/virt/bao/Kconfig
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +source "drivers/virt/bao/ipcshmem/Kconfig"
> diff --git a/drivers/virt/bao/Makefile b/drivers/virt/bao/Makefile
> new file mode 100644
> index 000000000000..68f5d3f282c4
> --- /dev/null
> +++ b/drivers/virt/bao/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_BAO_SHMEM) += ipcshmem/
> diff --git a/drivers/virt/bao/ipcshmem/Kconfig b/drivers/virt/bao/ipcshmem/Kconfig
> new file mode 100644
> index 000000000000..42690073e819
> --- /dev/null
> +++ b/drivers/virt/bao/ipcshmem/Kconfig
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +config BAO_SHMEM
> +	tristate "Bao hypervisor shared memory support"
> +
> +	help
> +	This enables support for Bao shared memory communication.
> +	It allows the kernel to interface with guests running under
> +	the Bao hypervisor, providing a character device interface
> +	for exchanging data through dedicated shared-memory regions.
> diff --git a/drivers/virt/bao/ipcshmem/Makefile b/drivers/virt/bao/ipcshmem/Makefile
> new file mode 100644
> index 000000000000..e027dcdb06aa
> --- /dev/null
> +++ b/drivers/virt/bao/ipcshmem/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_BAO_SHMEM) += bao.o
> +bao-objs += ipcshmem.o
> diff --git a/drivers/virt/bao/ipcshmem/ipcshmem.c b/drivers/virt/bao/ipcshmem/ipcshmem.c
> new file mode 100644
> index 000000000000..cadb79bfca6e
> --- /dev/null
> +++ b/drivers/virt/bao/ipcshmem/ipcshmem.c
> @@ -0,0 +1,539 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Bao Hypervisor IPC Through Shared-memory Driver
> + *
> + * Copyright (c) Bao Project and Contributors. All rights reserved.
> + *
> + * Authors:
> + *	David Cerdeira <davidmcerdeira@...x.tech>
> + *	José Martins <jose@...x.tech>
> + *	João Peixoto <joaopeixoto@...x.tech>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/io.h>
> +#include <linux/mutex.h>
> +#include <linux/poll.h>
> +#include <linux/platform_device.h>
> +#include <linux/ioctl.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/wait.h>
> +#include <linux/mm.h>
> +
> +#if defined(CONFIG_ARM64) || defined(CONFIG_ARM)
> +#include <linux/arm-smccc.h>
> +#include <asm/memory.h>
> +#elif CONFIG_RISCV
> +#include <asm/sbi.h>
> +#endif
> +
> +#define DEV_NAME "baoipc"
> +#define MAX_DEVICES 16
> +#define NAME_LEN 32
> +
> +static dev_t bao_ipcshmem_devt;
> +struct class *cl;
> +
> +/**
> + * struct bao_ipcshmem - Bao IPC shared memory device
> + * @cdev: Character device interface
> + * @dev: Device structure
> + * @id: Device instance ID
> + * @label: Name/label of the device
> + * @read_base: Base address of the read channel
> + * @read_size: Size of the read channel
> + * @write_base: Base address of the write channel
> + * @write_size: Size of the write channel
> + * @physical_base: Physical memory base address
> + * @shmem_size: Total size of the shared memory region
> + */
> +struct bao_ipcshmem {
> +	struct cdev cdev;
> +	struct device *dev;
> +	int id;
> +	char label[NAME_LEN];
> +	void *read_base;
> +	size_t read_size;
> +	void *write_base;
> +	size_t write_size;
> +	phys_addr_t *physical_base;
> +	size_t shmem_size;
> +};
> +
> +#ifdef CONFIG_ARM64

No ifdefs. Read Linux coding style first.

> +/**
> + * bao_ipcshmem_notify - Notify the Bao hypervisor of an IPC shared memory event (ARM64)
> + * @dev: IPC shared memory device
> + *
> + * Executes a fast SMC hypercall to notify the hypervisor of an event
> + * associated with the given IPC shared memory device.
> + *
> + * Return: Hypercall return value.
> + */
> +static uint64_t bao_ipcshmem_notify(struct bao_ipcshmem *dev)
> +{
> +	register uint64_t x0 asm("x0") =
> +		ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64,
> +				   ARM_SMCCC_OWNER_VENDOR_HYP, 1);
> +	register uint64_t x1 asm("x1") = dev->id;
> +	register uint64_t x2 asm("x2") = 0;
> +
> +	asm volatile("hvc 0\t\n" : "=r"(x0) : "r"(x0), "r"(x1), "r"(x2));
> +
> +	return x0;
> +}
> +#elif CONFIG_ARM
> +/**
> + * bao_ipcshmem_notify - Notify the Bao hypervisor of an IPC shared memory event (ARM)
> + * @dev: IPC shared memory device
> + *
> + * Executes a fast SMC hypercall to notify the hypervisor of an event
> + * associated with the given IPC shared memory device.
> + *
> + * Return: Hypercall return value.
> + */
> +static uint32_t bao_ipcshmem_notify(struct bao_ipcshmem *dev)
> +{
> +	register uint32_t r0 asm("r0") =
> +		ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32,
> +				   ARM_SMCCC_OWNER_VENDOR_HYP, 1);
> +	register uint32_t r1 asm("r1") = dev->id;
> +	register uint32_t r2 asm("r2") = 0;
> +
> +	asm volatile("hvc #0\t\n" : "=r"(r0) : "r"(r0), "r"(r1), "r"(r2));
> +
> +	return r0;
> +}
> +#elif CONFIG_RISCV
> +/**
> + * bao_ipcshmem_notify - Notify the Bao hypervisor of an IPC shared memory event (RISC-V)
> + * @dev: IPC shared memory device
> + *
> + * Executes an SBI call to notify the Bao hypervisor of an IPC shared memory event.
> + *
> + * Return: SBI call error code.
> + */
> +static uint64_t bao_ipcshmem_notify(struct bao_ipcshmem *dev)
> +{
> +	struct sbiret ret = sbi_ecall(0x08000ba0, 1, dev->id, 0, 0, 0, 0, 0);
> +
> +	return ret.error;
> +}
> +#endif
> +
> +/**
> + * bao_ipcshmem_mmap_fops - mmap handler for IPC shared memory
> + * @filp: File pointer
> + * @vma: Virtual memory area
> + *
> + * Maps the physical shared memory of the Bao IPC device into
> + * userspace using remap_pfn_range.
> + *
> + * Return: 0 on success, -EFAULT on failure.
> + */
> +static int bao_ipcshmem_mmap_fops(struct file *filp, struct vm_area_struct *vma)
> +{
> +	struct bao_ipcshmem *bao = filp->private_data;
> +	unsigned long vsize = vma->vm_end - vma->vm_start;
> +	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
> +	phys_addr_t paddr;
> +
> +	if (WARN_ON_ONCE(!bao))
> +		return -ENODEV;
> +
> +	if (!vsize)
> +		return -EINVAL;
> +
> +	if (offset >= bao->shmem_size)
> +		return -EINVAL;
> +
> +	if (vsize > bao->shmem_size - offset)
> +		return -EINVAL;
> +
> +	paddr = (phys_addr_t)bao->physical_base + offset;
> +
> +	if (!PAGE_ALIGNED(paddr))
> +		return -EINVAL;
> +
> +	if (remap_pfn_range(vma, vma->vm_start,
> +			    paddr >> PAGE_SHIFT,
> +			    vsize, vma->vm_page_prot))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +/**
> + * bao_ipcshmem_read_fops - read handler for IPC shared memory
> + * @filp: File pointer
> + * @buf: Userspace buffer
> + * @count: Number of bytes to read
> + * @ppos: File offset
> + *
> + * Copies data from the Bao IPC read buffer to userspace.
> + *
> + * Return: Number of bytes read, or 0 if EOF.
> + */
> +static ssize_t bao_ipcshmem_read_fops(struct file *filp, char __user *buf,
> +				      size_t count, loff_t *ppos)
> +{
> +	struct bao_ipcshmem *bao = filp->private_data;
> +	size_t available;
> +
> +	if (WARN_ON_ONCE(!bao || !buf || !ppos))
> +		return -EINVAL;
> +
> +	if (*ppos >= bao->read_size)
> +		return 0;
> +
> +	available = bao->read_size - *ppos;
> +	if (count > available)
> +		count = available;
> +
> +	if (copy_to_user(buf, bao->read_base + *ppos, count))
> +		return -EFAULT;
> +
> +	*ppos += count;
> +	return count;
> +}
> +
> +/**
> + * bao_ipcshmem_write_fops - write handler for IPC shared memory
> + * @filp: File pointer
> + * @buf: Userspace buffer
> + * @count: Number of bytes to write
> + * @ppos: File offset
> + *
> + * Copies data from userspace to the Bao IPC write buffer and
> + * notifies the hypervisor of the update.
> + *
> + * Return: Number of bytes written.
> + */
> +static ssize_t bao_ipcshmem_write_fops(struct file *filp, const char __user *buf,
> +				       size_t count, loff_t *ppos)
> +{
> +	struct bao_ipcshmem *bao = filp->private_data;
> +	size_t avail;
> +
> +	if (WARN_ON_ONCE(!bao || !buf || !ppos))
> +		return -EINVAL;
> +
> +	if (*ppos >= bao->write_size)
> +		return 0;
> +
> +	avail = bao->write_size - *ppos;
> +	if (count > avail)
> +		count = avail;
> +
> +	if (copy_from_user(bao->write_base + *ppos, buf, count))
> +		return -EFAULT;
> +
> +	*ppos += count;
> +
> +	/* Notify any listeners that new data is available */
> +	bao_ipcshmem_notify(bao);
> +
> +	return count;
> +}
> +
> +/**
> + * bao_ipcshmem_open_fops - open handler for IPC shared memory
> + * @inode: Inode pointer
> + * @filp: File pointer
> + *
> + * Associates the file with the Bao IPC device and increments
> + * the kobject reference.
> + *
> + * Return: 0 on success.
> + */
> +static int bao_ipcshmem_open_fops(struct inode *inode, struct file *filp)
> +{
> +	struct bao_ipcshmem *bao;
> +
> +	if (WARN_ON_ONCE(!inode || !filp))
> +		return -EINVAL;
> +
> +	bao = container_of(inode->i_cdev, struct bao_ipcshmem, cdev);
> +	filp->private_data = bao;
> +
> +	kobject_get(&bao->dev->kobj);
> +
> +	return 0;
> +}
> +
> +/**
> + * bao_ipcshmem_release_fops - release handler for IPC shared memory
> + * @inode: Inode pointer
> + * @filp: File pointer
> + *
> + * Disassociates the file from the Bao IPC device and decrements
> + * the kobject reference.
> + *
> + * Return: 0 on success.
> + */
> +static int bao_ipcshmem_release_fops(struct inode *inode, struct file *filp)
> +{
> +	struct bao_ipcshmem *bao;
> +
> +	if (WARN_ON_ONCE(!inode || !filp))
> +		return -EINVAL;
> +
> +	bao = container_of(inode->i_cdev, struct bao_ipcshmem, cdev);
> +	filp->private_data = NULL;
> +
> +	kobject_put(&bao->dev->kobj);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations bao_ipcshmem_fops = {
> +	.owner = THIS_MODULE,
> +	.read = bao_ipcshmem_read_fops,
> +	.write = bao_ipcshmem_write_fops,
> +	.mmap = bao_ipcshmem_mmap_fops,
> +	.open = bao_ipcshmem_open_fops,
> +	.release = bao_ipcshmem_release_fops
> +};
> +
> +/**
> + * bao_ipcshmem_register - Register a Bao IPC shared memory device
> + * @pdev: Platform device
> + *
> + * Maps the shared memory region, validates channel layout, initializes
> + * the read/write buffers, registers the character device, and creates
> + * the sysfs device entry.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static int bao_ipcshmem_register(struct platform_device *pdev)

NAK, where did you get it from? Probe functions are ABSOLUTELY NEVER
called register.

> +{
> +	int ret = 0, id = -1;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct module *owner = THIS_MODULE;
> +	struct resource *r;
> +	dev_t devt;

No, read Linux coding style.

> +	resource_size_t shmem_size;
> +	u32 write_offset, read_offset, write_size, read_size;
> +	bool rd_in_range, wr_in_range, disjoint;
> +	void *shmem_base_addr = NULL;
> +	struct bao_ipcshmem *bao;

This is really poor coding style, barely readable and maintainable. You
need to rewtite this driver completely to match what we expect in Linux
kernel, for example base your work on last, reviewed code.

I am not even looking at rest of this - please prove that you value our
time by sending something following Linux kernel style.

Best regards,
Krzysztof

Powered by blists - more mailing lists