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

[Elliot Berman]

On 11/2/2022 12:31 AM, Arnd Bergmann wrote:
> 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.

Ack.

> > +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.

I'd like the argument here to support different types of virtual 
machines. I want to leave open what "different types" can be in case 
something new comes up in the future, but immediately "different type" 
would correspond to a few different authentication mechanisms for 
virtual machines that Gunyah supports.

In this series, I'm only supporting unauthenticated virtual machines 
because they are the simplest to get up and running from a Linux 
userspace. When I introduce the other authentication mechanisms, I'll 
expand much more on how they work, but I'll give quick overview here. 
Other authentication mechanisms that are currently supported by Gunyah 
are "protected VM" and, on Qualcomm platforms, "PIL/carveout VMs". 
Protected VMs are *loosely* similar to the protected firmware design for 
KVM and intended to support Android virtualization use cases. 
PIL/carevout VMs are special virtual machines that can run on Qualcomm 
firmware which authenticate in a way similar to remoteproc firmware (MDT 
loader).

> > +	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.

Ack

> > +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.

Ack. I can do this, although I am nervous about this snowballing into a 
situation where I have a mega-module.

> Please stop beating everything in a single module.

https://lore.kernel.org/all/250945d2-3940-9830-63e5-beec5f44010b@xxxxxxxxxx/

Thanks,
Elliot