Re: [PATCH RFC v2 08/15] vfio/nvgrace-egm: Expose EGM region as char device

[Alex Williamson <alex@shazbot.org>]

On Mon, 23 Feb 2026 15:55:07 +0000
<ankita@nvidia.com> wrote:

> From: Ankit Agrawal <ankita@nvidia.com>
> 
> The EGM module expose the various EGM regions as a char device. A
> usermode app such as Qemu may mmap to the region and use as VM sysmem.
> Each EGM region is represented with a unique char device /dev/egmX
> bearing a distinct minor number.
> 
> EGM module implements the mmap file_ops to manage the usermode app's
> VMA mapping to the EGM region. The appropriate region is determined
> from the minor number.
> 
> Note that the EGM memory region is invisible to the host kernel as it
> is not present in the host EFI map. The host Linux MM thus cannot manage
> the memory, even though it is accessible on the host SPA. The EGM module
> thus use remap_pfn_range() to perform the VMA mapping to the EGM region.
> 
> Suggested-by: Aniket Agashe <aniketa@nvidia.com>
> Signed-off-by: Ankit Agrawal <ankita@nvidia.com>
> ---
>  drivers/vfio/pci/nvgrace-gpu/egm.c | 99 ++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/drivers/vfio/pci/nvgrace-gpu/egm.c b/drivers/vfio/pci/nvgrace-gpu/egm.c
> index 6fd6302a004a..d7e4f61a241c 100644
> --- a/drivers/vfio/pci/nvgrace-gpu/egm.c
> +++ b/drivers/vfio/pci/nvgrace-gpu/egm.c
> @@ -10,15 +10,114 @@
>  
>  static dev_t dev;
>  static struct class *class;
> +static DEFINE_XARRAY(egm_chardevs);
> +
> +struct chardev {
> +	struct device device;
> +	struct cdev cdev;
> +};
> +
> +static int nvgrace_egm_open(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}
> +
> +static int nvgrace_egm_release(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}
> +
> +static int nvgrace_egm_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	return 0;

At this point it seems none of these stubs should return success.

> +}
> +
> +static const struct file_operations file_ops = {
> +	.owner = THIS_MODULE,
> +	.open = nvgrace_egm_open,
> +	.release = nvgrace_egm_release,
> +	.mmap = nvgrace_egm_mmap,
> +};
> +
> +static void egm_chardev_release(struct device *dev)
> +{
> +	struct chardev *egm_chardev = container_of(dev, struct chardev, device);
> +
> +	kfree(egm_chardev);
> +}
> +
> +static struct chardev *
> +setup_egm_chardev(struct nvgrace_egm_dev *egm_dev)
> +{
> +	struct chardev *egm_chardev;
> +	int ret;
> +
> +	egm_chardev = kzalloc(sizeof(*egm_chardev), GFP_KERNEL);
> +	if (!egm_chardev)
> +		goto create_err;

return ERR_PTR(-ENOMEM);  Same for remaining returns.

> +
> +	device_initialize(&egm_chardev->device);
> +
> +	/*
> +	 * Use the proximity domain number as the device minor
> +	 * number. So the EGM corresponding to node X would be
> +	 * /dev/egmX.
> +	 */
> +	egm_chardev->device.devt = MKDEV(MAJOR(dev), egm_dev->egmpxm);

As in previous comment, we have no guarantee that the PXM value is in
the range 0-3 of the reserved minor numbers.

> +	egm_chardev->device.class = class;
> +	egm_chardev->device.release = egm_chardev_release;
> +	egm_chardev->device.parent = &egm_dev->aux_dev.dev;
> +	cdev_init(&egm_chardev->cdev, &file_ops);
> +	egm_chardev->cdev.owner = THIS_MODULE;
> +
> +	ret = dev_set_name(&egm_chardev->device, "egm%lld", egm_dev->egmpxm);
> +	if (ret)
> +		goto error_exit;
> +
> +	ret = cdev_device_add(&egm_chardev->cdev, &egm_chardev->device);
> +	if (ret)
> +		goto error_exit;
> +
> +	return egm_chardev;
> +
> +error_exit:
> +	put_device(&egm_chardev->device);
> +create_err:
> +	return NULL;
> +}
> +
> +static void del_egm_chardev(struct chardev *egm_chardev)
> +{
> +	cdev_device_del(&egm_chardev->cdev, &egm_chardev->device);
> +	put_device(&egm_chardev->device);
> +}
>  
>  static int egm_driver_probe(struct auxiliary_device *aux_dev,
>  			    const struct auxiliary_device_id *id)
>  {
> +	struct nvgrace_egm_dev *egm_dev =
> +		container_of(aux_dev, struct nvgrace_egm_dev, aux_dev);
> +	struct chardev *egm_chardev;
> +
> +	egm_chardev = setup_egm_chardev(egm_dev);
> +	if (!egm_chardev)
> +		return -EINVAL;

Use IS_ERR() and don't clobber the return value.

> +
> +	xa_store(&egm_chardevs, egm_dev->egmpxm, egm_chardev, GFP_KERNEL);

Return value unchecked.  Isn't this xarray just replacing stuffing this
in drvdata?  Why?

> +
>  	return 0;
>  }
>  
>  static void egm_driver_remove(struct auxiliary_device *aux_dev)
>  {
> +	struct nvgrace_egm_dev *egm_dev =
> +		container_of(aux_dev, struct nvgrace_egm_dev, aux_dev);
> +	struct chardev *egm_chardev = xa_erase(&egm_chardevs, egm_dev->egmpxm);
> +
> +	if (!egm_chardev)
> +		return;
> +
> +	del_egm_chardev(egm_chardev);

No evidence yet of lifecycle management if there's an outstanding
opened chardev.  Thanks,

Alex

>  }
>  
>  static const struct auxiliary_device_id egm_id_table[] = {