Re: [PATCH v6 13/21] gunyah: vm_mgr: Introduce basic VM Manager

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Oct 26, 2022, at 20:58, Elliot Berman wrote:

> +static const struct file_operations gh_vm_fops = {
> +	.unlocked_ioctl = gh_vm_ioctl,
> +	.release = gh_vm_release,
> +	.llseek = noop_llseek,
> +};

There should be a .compat_ioctl entry here, otherwise it is
impossible to use from 32-bit tasks. If all commands have
arguments passed through a pointer to a properly defined
structure, you can just set it to compat_ptr_ioctl.

> +static long gh_dev_ioctl_create_vm(unsigned long arg)
> +{
> +	struct gunyah_vm *ghvm;
> +	struct file *file;
> +	int fd, err;
> +
> +	/* arg reserved for future use. */
> +	if (arg)
> +		return -EINVAL;

Do you have something specific in mind here? If 'create'
is the only command you support, and it has no arguments,
it would be easier to do it implicitly during open() and
have each fd opened from /dev/gunyah represent a new VM.

> +	ghvm = gunyah_vm_alloc();
> +	if (IS_ERR_OR_NULL(ghvm))
> +		return PTR_ERR(ghvm) ? : -ENOMEM;

If you find yourself using IS_ERR_OR_NULL(), you have
usually made a mistake. In this case, the gunyah_vm_alloc()
function is badly defined and should just return -ENOMEM
for an allocation failure.

> +static struct gunyah_rsc_mgr_device_id vm_mgr_ids[] = {
> +	{ .name = GH_RM_DEVICE_VM_MGR },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(gunyah_rsc_mgr, vm_mgr_ids);
> +
> +static struct gh_rm_driver vm_mgr_drv = {
> +	.drv = {
> +		.name = KBUILD_MODNAME,
> +		.probe = vm_mgr_probe,
> +		.remove = vm_mgr_remove,
> +	},
> +	.id_table = vm_mgr_ids,
> +};
> +module_gh_rm_driver(vm_mgr_drv);

It looks like the gunyah_rsc_mgr_device_id in this case is
purely internal to the kernel, so you are adding abstraction
layers to something that does not need to be abstracted
because the host side has no corresponding concept of
devices.

I'm correct, you can just turn the entire bus/device/driver
structure within your code into simple function calls, where
the main code calls vm_mgr_probe() as an exported function
instead of creating a device.

      Arnd