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

[Elliot Berman]

On 11/2/2022 5:20 PM, Greg Kroah-Hartman wrote:
> On Wed, Nov 02, 2022 at 11:44:51AM -0700, Elliot Berman wrote:
> > 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.
>
> Please don't add code that does not actually do something now, as that
> makes it impossible to review properly, _AND_ no one knows what is going
> to happen in the future.  In the future, you can just add a new ioctl
> and all is good, no need to break working userspace by suddenly looking
> at the arg value and doing something with it.

I think the argument does something today and it's documented to need to 
be 0. If a userspace from the future provides non-zero value, Linux will 
correctly reject it because it doesn't know how to interpret the 
different VM types.

I can document it more clearly as the VM type field and support only the 
one VM type today.

Creating new ioctl for each VM type feels to me like I didn't design 
CREATE_VM ioctl right in first place.

Thanks,
Elliot