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

  Gunyah VM manager is a kernel moduel which exposes an interface to
Gunyah userspace to load, run, and interact with other Gunyah virtual
machines. The interface is a character device at /dev/gunyah.

Add a basic VM manager driver. Upcoming patches will add more ioctls
into this driver.

Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 drivers/virt/gunyah/Kconfig                   |   8 +
 drivers/virt/gunyah/Makefile                  |   3 +
 drivers/virt/gunyah/vm_mgr.c                  | 152 ++++++++++++++++++
 drivers/virt/gunyah/vm_mgr.h                  |  17 ++
 include/uapi/linux/gunyah.h                   |  23 +++
 6 files changed, 204 insertions(+)
 create mode 100644 drivers/virt/gunyah/vm_mgr.c
 create mode 100644 drivers/virt/gunyah/vm_mgr.h
 create mode 100644 include/uapi/linux/gunyah.h
  

On Wed, Oct 26, 2022 at 11:58:38AM -0700, Elliot Berman wrote:
> +#define GH_CREATE_VM			_IO(GH_IOCTL_TYPE, 0x40) /* Returns a Gunyah VM fd */

Why 0x40?  Why not just use the same KVM ioctl numbers and names as you
are doing the same thing as them, right?

Normally your first ioctl is "0x01", not "0x40", so this feels really
odd.

thanks,

greg k-h
  

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
  

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@linaro.org/

Thanks,
Elliot
  

+Michael

On 11/1/2022 10:14 PM, Greg Kroah-Hartman wrote:
> On Wed, Oct 26, 2022 at 11:58:38AM -0700, Elliot Berman wrote:
>> +#define GH_CREATE_VM			_IO(GH_IOCTL_TYPE, 0x40) /* Returns a Gunyah VM fd */
> 
> Why 0x40?  Why not just use the same KVM ioctl numbers and names as you
> are doing the same thing as them, right?

We've designed so that there are a few ioctls that will feel similar to 
KVM ioctls since we know this design has been successful, but we don't 
intend to support KVM ioctls 1:1. Gunyah has different semantics for 
many of the name-identical ioctls. It seems odd to mix some re-used KVM 
ioctls with novel Gunyah ioctls?

> 
> Normally your first ioctl is "0x01", not "0x40", so this feels really
> odd.
> 

Documentation/userspace-api/ioctl/iocl-number.rst advises to pick an 
unused block. We picked ioctl code 'G' and unused sequence numbers under 
that code. I'm ok to move the block around.

Thanks,
Elliot
  

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.

thanks,

greg k-h
  

On Wed, Nov 02, 2022 at 11:45:12AM -0700, Elliot Berman wrote:
> +Michael
> 
> On 11/1/2022 10:14 PM, Greg Kroah-Hartman wrote:
> > On Wed, Oct 26, 2022 at 11:58:38AM -0700, Elliot Berman wrote:
> > > +#define GH_CREATE_VM			_IO(GH_IOCTL_TYPE, 0x40) /* Returns a Gunyah VM fd */
> > 
> > Why 0x40?  Why not just use the same KVM ioctl numbers and names as you
> > are doing the same thing as them, right?
> 
> We've designed so that there are a few ioctls that will feel similar to KVM
> ioctls since we know this design has been successful, but we don't intend to
> support KVM ioctls 1:1. Gunyah has different semantics for many of the
> name-identical ioctls. It seems odd to mix some re-used KVM ioctls with
> novel Gunyah ioctls?

Even if you don't support it 1:1, at least for the ones that are the
same thing, pick the same numbers as that's a nicer thing to do, right?

> > Normally your first ioctl is "0x01", not "0x40", so this feels really
> > odd.
> > 
> 
> Documentation/userspace-api/ioctl/iocl-number.rst advises to pick an unused
> block. We picked ioctl code 'G' and unused sequence numbers under that code.
> I'm ok to move the block around.

How do you know you picked an unused block?  It wasn't obvious where you
got these values from at all, and unfortunatly, no one really ever
updates that documentation file.  Luckily it never really matters.

thanks,

greg k-h
  

On Wed, Nov 2, 2022, at 19:44, Elliot Berman wrote:
> On 11/2/2022 12:31 AM, Arnd Bergmann wrote:
>>> +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).

Ok, thanks for the background. Having different types of virtual
machines does mean that you may need some complexity, but I would
still lean towards using the simpler context management of opening
the /dev/gunyah device node to get a new context, and then using
ioctls on each fd to manage that context, instead of going through
the extra indirection of having a secondary 'open context' command
that always requires opening two file descriptors.

>> 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@linaro.org/

I see you concern, but I wasn't suggesting having everything
in one module either. There are three common ways of splitting
things into separate modules:

- I suggested having the vm_mgr module as a library block that
  exports a few symbols which get used by the core module. The
  module doesn't do anything on its own, but loading the core
  module forces loading the vm_mgr.

- Alternatively one can do the opposite, and have symbols
  exported by the core module, with the vm_mgr module using
  it. This would make sense if you commonly have the core
  module loaded on virtual machines that do not need to manage
  other VMs.

- The method you have is to have a lower "bus" level that
  abstracts device providers from consumers, with both sides
  hooking into the bus. This makes sense for physical buses
  like PCI or USB where both the host driver and the function
  driver are unaware of implementation details of the other,
  but in your case it does not seem like a good fit.

        Arnd
  

On 11/3/2022 2:39 AM, Arnd Bergmann wrote:
> On Wed, Nov 2, 2022, at 19:44, Elliot Berman wrote:
>> On 11/2/2022 12:31 AM, Arnd Bergmann wrote:
>>>> +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).
> 
> Ok, thanks for the background. Having different types of virtual
> machines does mean that you may need some complexity, but I would
> still lean towards using the simpler context management of opening
> the /dev/gunyah device node to get a new context, and then using
> ioctls on each fd to manage that context, instead of going through
> the extra indirection of having a secondary 'open context' command
> that always requires opening two file descriptors.
> 
>>> 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@linaro.org/
> 
> I see you concern, but I wasn't suggesting having everything
> in one module either. There are three common ways of splitting
> things into separate modules:
> 
> - I suggested having the vm_mgr module as a library block that
>    exports a few symbols which get used by the core module. The
>    module doesn't do anything on its own, but loading the core
>    module forces loading the vm_mgr.
> 

Got the idea, I'll do this.

- Elliot

> - Alternatively one can do the opposite, and have symbols
>    exported by the core module, with the vm_mgr module using
>    it. This would make sense if you commonly have the core
>    module loaded on virtual machines that do not need to manage
>    other VMs.
> 
> - The method you have is to have a lower "bus" level that
>    abstracts device providers from consumers, with both sides
>    hooking into the bus. This makes sense for physical buses
>    like PCI or USB where both the host driver and the function
>    driver are unaware of implementation details of the other,
>    but in your case it does not seem like a good fit.
> 
>          Arnd
  

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
  

On 11/2/2022 5:24 PM, Greg Kroah-Hartman wrote:
> On Wed, Nov 02, 2022 at 11:45:12AM -0700, Elliot Berman wrote:
>> +Michael
>>
>> On 11/1/2022 10:14 PM, Greg Kroah-Hartman wrote:
>>> On Wed, Oct 26, 2022 at 11:58:38AM -0700, Elliot Berman wrote:
>>>> +#define GH_CREATE_VM			_IO(GH_IOCTL_TYPE, 0x40) /* Returns a Gunyah VM fd */
>>>
>>> Why 0x40?  Why not just use the same KVM ioctl numbers and names as you
>>> are doing the same thing as them, right?
>>
>> We've designed so that there are a few ioctls that will feel similar to KVM
>> ioctls since we know this design has been successful, but we don't intend to
>> support KVM ioctls 1:1. Gunyah has different semantics for many of the
>> name-identical ioctls. It seems odd to mix some re-used KVM ioctls with
>> novel Gunyah ioctls?
> 
> Even if you don't support it 1:1, at least for the ones that are the
> same thing, pick the same numbers as that's a nicer thing to do, right?
> 

Does same thing == interpretation of arguments is the same? For
instance, GH_CREATE_VM and KVM_CREATE_VM interpret the arguments
differently. Same for KVM_SET_USERSPACE_MEMORY. The high level 
functionality should be similar for most all hypervisors since they will 
all support creating a VM and probably sharing memory with that VM. The 
arguments for that will necessarily look similar, but they will probably 
be subtly different because the hypervisors support different features.

I don't think userspace that supports both KVM and Gunyah will benefit 
much from re-using the same numbers since those re-used ioctl calls 
still need to sit within the context of a Gunyah VM.

Thanks,
Elliot
  

On Fri, Nov 4, 2022, at 01:11, Elliot Berman wrote:
> On 11/2/2022 5:24 PM, Greg Kroah-Hartman wrote:
>> On Wed, Nov 02, 2022 at 11:45:12AM -0700, Elliot Berman wrote:
>>
>> Even if you don't support it 1:1, at least for the ones that are the
>> same thing, pick the same numbers as that's a nicer thing to do, right?
>> 
>
> Does same thing == interpretation of arguments is the same? For
> instance, GH_CREATE_VM and KVM_CREATE_VM interpret the arguments
> differently. Same for KVM_SET_USERSPACE_MEMORY. The high level 
> functionality should be similar for most all hypervisors since they will 
> all support creating a VM and probably sharing memory with that VM. The 
> arguments for that will necessarily look similar, but they will probably 
> be subtly different because the hypervisors support different features.

I think in the ideal case, you should make the arguments and the
command codes the same for any command where that is possible. If
you come across a command that is shared with KVM but just needs
another flag, that would involve coordinating with the KVM maintainers
about sharing the definition so the same flag does not get reused
in an incompatible way.

For commands that cannot fit into the existing definition, there
should be a different command code, using your own namespace and
not the 0xAE block that KVM has. It still makes sense to follow
the argument structure roughly here, unless there is a technical
reason for making it different.

> I don't think userspace that supports both KVM and Gunyah will benefit 
> much from re-using the same numbers since those re-used ioctl calls 
> still need to sit within the context of a Gunyah VM.

One immediate benefit is for tools that work on running processes,
such as strace, gdb or qemu-user. If they encounter a known command,
they can correctly display the arguments etc.

Another benefit is for sharing portions of the VMM that live in
outside processes like vhost-user based device emulation that
interacts with irqfd, memfd etc. The more similar the command
interface is, the easier it gets to keep these tools portable.

      Arnd
  

On 11/4/2022 1:10 AM, Arnd Bergmann wrote:
> On Fri, Nov 4, 2022, at 01:11, Elliot Berman wrote:
>> On 11/2/2022 5:24 PM, Greg Kroah-Hartman wrote:
>>> On Wed, Nov 02, 2022 at 11:45:12AM -0700, Elliot Berman wrote:
>>>
>>> Even if you don't support it 1:1, at least for the ones that are the
>>> same thing, pick the same numbers as that's a nicer thing to do, right?
>>>
>>
>> Does same thing == interpretation of arguments is the same? For
>> instance, GH_CREATE_VM and KVM_CREATE_VM interpret the arguments
>> differently. Same for KVM_SET_USERSPACE_MEMORY. The high level
>> functionality should be similar for most all hypervisors since they will
>> all support creating a VM and probably sharing memory with that VM. The
>> arguments for that will necessarily look similar, but they will probably
>> be subtly different because the hypervisors support different features.
> 
> I think in the ideal case, you should make the arguments and the
> command codes the same for any command where that is possible. If
> you come across a command that is shared with KVM but just needs
> another flag, that would involve coordinating with the KVM maintainers
> about sharing the definition so the same flag does not get reused
> in an incompatible way.
> 

I think the converse also needs to be true; KVM would need to check that
new flags don't get used in some incompatible way with Gunyah, even if
one of us is just -EINVAL'ing. I don't think Gunyah and KVM should be
reliant on the other reviewing shared ioctls.

The problem is a bit worse because "machine type" is architecture-
dependent whereas the planned Gunyah flags are architecture-independent.
KVM within itself re-uses flags between architectures so Gunyah would
need to reserve some flags from all architectures that KVM supports.

> For commands that cannot fit into the existing definition, there
> should be a different command code, using your own namespace and
> not the 0xAE block that KVM has. It still makes sense to follow
> the argument structure roughly here, unless there is a technical
> reason for making it different.
> 
>> I don't think userspace that supports both KVM and Gunyah will benefit
>> much from re-using the same numbers since those re-used ioctl calls
>> still need to sit within the context of a Gunyah VM.
> 
> One immediate benefit is for tools that work on running processes,
> such as strace, gdb or qemu-user. If they encounter a known command,
> they can correctly display the arguments etc.
> 

We can update these tools and anyway there will be different ioctls to
get started. There are important ioctls that wouldn't be correctly
displayed off the bat anyway; work would need to be done to support the
Gunyah ioctls either way. Whereas tooling update is temporary, the
coupling of KVM and Gunyah ioctls would be permanent.

> Another benefit is for sharing portions of the VMM that live in
> outside processes like vhost-user based device emulation that
> interacts with irqfd, memfd etc. The more similar the command
> interface is, the easier it gets to keep these tools portable.
> 

Hypervisor interfaces already have different ioctls for equivalent
functionality [1], so VMMs that want to scale to multiple hypervisors
already abstract out ioctl-level interfaces so there wouldn't be any
code-reuse even if Gunyah and KVM shared the same ioctl number. Between
hypervisors, the best case is there is design similarity for userspace,
which makes it easier to add new hypervisor support for VMMs and that's
what we are aiming for.

[1]: e.g. compare KVM, acrn, xen for implementing virtual interrupts.
KVM and acrn use independently implemented irqfd interfaces, xen has
totally different implementation called event channels.

Thanks,
Elliot
  

On 11/4/2022 3:38 PM, Elliot Berman wrote:
> 
> 
> On 11/4/2022 1:10 AM, Arnd Bergmann wrote:
>> On Fri, Nov 4, 2022, at 01:11, Elliot Berman wrote:
>>> On 11/2/2022 5:24 PM, Greg Kroah-Hartman wrote:
>>>> On Wed, Nov 02, 2022 at 11:45:12AM -0700, Elliot Berman wrote:
>>>>
>>>> Even if you don't support it 1:1, at least for the ones that are the
>>>> same thing, pick the same numbers as that's a nicer thing to do, right?
>>>>
>>>
>>> Does same thing == interpretation of arguments is the same? For
>>> instance, GH_CREATE_VM and KVM_CREATE_VM interpret the arguments
>>> differently. Same for KVM_SET_USERSPACE_MEMORY. The high level
>>> functionality should be similar for most all hypervisors since they will
>>> all support creating a VM and probably sharing memory with that VM. The
>>> arguments for that will necessarily look similar, but they will probably
>>> be subtly different because the hypervisors support different features.
>>
>> I think in the ideal case, you should make the arguments and the
>> command codes the same for any command where that is possible. If
>> you come across a command that is shared with KVM but just needs
>> another flag, that would involve coordinating with the KVM maintainers
>> about sharing the definition so the same flag does not get reused
>> in an incompatible way.
>>
> 
> I think the converse also needs to be true; KVM would need to check that
> new flags don't get used in some incompatible way with Gunyah, even if
> one of us is just -EINVAL'ing. I don't think Gunyah and KVM should be
> reliant on the other reviewing shared ioctls.
> 
> The problem is a bit worse because "machine type" is architecture-
> dependent whereas the planned Gunyah flags are architecture-independent.
> KVM within itself re-uses flags between architectures so Gunyah would
> need to reserve some flags from all architectures that KVM supports.

I agree w/ Elliot. We would like to keep Gunyah independent and not rely 
on the existing KVM ioctls space. We should allow new hypervisor drivers 
interfaces addition in Linux kernel without them relying on KVM.

> 
>> For commands that cannot fit into the existing definition, there
>> should be a different command code, using your own namespace and
>> not the 0xAE block that KVM has. It still makes sense to follow
>> the argument structure roughly here, unless there is a technical
>> reason for making it different.
>>
>>> I don't think userspace that supports both KVM and Gunyah will benefit
>>> much from re-using the same numbers since those re-used ioctl calls
>>> still need to sit within the context of a Gunyah VM.
>>
>> One immediate benefit is for tools that work on running processes,
>> such as strace, gdb or qemu-user. If they encounter a known command,
>> they can correctly display the arguments etc.
>>
> 
> We can update these tools and anyway there will be different ioctls to
> get started. There are important ioctls that wouldn't be correctly
> displayed off the bat anyway; work would need to be done to support the
> Gunyah ioctls either way. Whereas tooling update is temporary, the
> coupling of KVM and Gunyah ioctls would be permanent.

Agree, tools can be updated and that is the easy part as we grow the s/w 
stack around Gunyah in userspace, like we already do w/ CrosVM (Virtual 
Machine Manager) and QEMU will be next followed by rust-vmm. All of them 
can be done without Gunyah ioctls relying anything on the KVM ioctls. 
Elliot has also explained very well that we don't to go to KVM 
maintainers for any of our additions and we also don't want them to come 
to us, since there is no interoperability testing. It is best that both 
Hypervisors and their Linux interfaces evolve independently.

---Trilok Soni
  

Hi Arnd, Greg,

On 11/4/2022 9:19 PM, Trilok Soni wrote:
> On 11/4/2022 3:38 PM, Elliot Berman wrote:
>>
>>
>> On 11/4/2022 1:10 AM, Arnd Bergmann wrote:
>>> On Fri, Nov 4, 2022, at 01:11, Elliot Berman wrote:
>>>> On 11/2/2022 5:24 PM, Greg Kroah-Hartman wrote:
>>>>> On Wed, Nov 02, 2022 at 11:45:12AM -0700, Elliot Berman wrote:
>>>>>
>>>>> Even if you don't support it 1:1, at least for the ones that are the
>>>>> same thing, pick the same numbers as that's a nicer thing to do, 
>>>>> right?
>>>>>
>>>>
>>>> Does same thing == interpretation of arguments is the same? For
>>>> instance, GH_CREATE_VM and KVM_CREATE_VM interpret the arguments
>>>> differently. Same for KVM_SET_USERSPACE_MEMORY. The high level
>>>> functionality should be similar for most all hypervisors since they 
>>>> will
>>>> all support creating a VM and probably sharing memory with that VM. The
>>>> arguments for that will necessarily look similar, but they will 
>>>> probably
>>>> be subtly different because the hypervisors support different features.
>>>
>>> I think in the ideal case, you should make the arguments and the
>>> command codes the same for any command where that is possible. If
>>> you come across a command that is shared with KVM but just needs
>>> another flag, that would involve coordinating with the KVM maintainers
>>> about sharing the definition so the same flag does not get reused
>>> in an incompatible way.
>>>
>>
>> I think the converse also needs to be true; KVM would need to check that
>> new flags don't get used in some incompatible way with Gunyah, even if
>> one of us is just -EINVAL'ing. I don't think Gunyah and KVM should be
>> reliant on the other reviewing shared ioctls.
>>
>> The problem is a bit worse because "machine type" is architecture-
>> dependent whereas the planned Gunyah flags are architecture-independent.
>> KVM within itself re-uses flags between architectures so Gunyah would
>> need to reserve some flags from all architectures that KVM supports.
> 
> I agree w/ Elliot. We would like to keep Gunyah independent and not rely 
> on the existing KVM ioctls space. We should allow new hypervisor drivers 
> interfaces addition in Linux kernel without them relying on KVM.
> 
>>
>>> For commands that cannot fit into the existing definition, there
>>> should be a different command code, using your own namespace and
>>> not the 0xAE block that KVM has. It still makes sense to follow
>>> the argument structure roughly here, unless there is a technical
>>> reason for making it different.
>>>
>>>> I don't think userspace that supports both KVM and Gunyah will benefit
>>>> much from re-using the same numbers since those re-used ioctl calls
>>>> still need to sit within the context of a Gunyah VM.
>>>
>>> One immediate benefit is for tools that work on running processes,
>>> such as strace, gdb or qemu-user. If they encounter a known command,
>>> they can correctly display the arguments etc.
>>>
>>
>> We can update these tools and anyway there will be different ioctls to
>> get started. There are important ioctls that wouldn't be correctly
>> displayed off the bat anyway; work would need to be done to support the
>> Gunyah ioctls either way. Whereas tooling update is temporary, the
>> coupling of KVM and Gunyah ioctls would be permanent.
> 
> Agree, tools can be updated and that is the easy part as we grow the s/w 
> stack around Gunyah in userspace, like we already do w/ CrosVM (Virtual 
> Machine Manager) and QEMU will be next followed by rust-vmm. All of them 
> can be done without Gunyah ioctls relying anything on the KVM ioctls. 
> Elliot has also explained very well that we don't to go to KVM 
> maintainers for any of our additions and we also don't want them to come 
> to us, since there is no interoperability testing. It is best that both 
> Hypervisors and their Linux interfaces evolve independently.

Are above explanations reasonable to not re-use KVM ioctl numbers?

Thanks,
Elliot
  

On Thu, Nov 10, 2022 at 04:03:10PM -0800, Elliot Berman wrote:
> > Agree, tools can be updated and that is the easy part as we grow the s/w
> > stack around Gunyah in userspace, like we already do w/ CrosVM (Virtual
> > Machine Manager) and QEMU will be next followed by rust-vmm. All of them
> > can be done without Gunyah ioctls relying anything on the KVM ioctls.
> > Elliot has also explained very well that we don't to go to KVM
> > maintainers for any of our additions and we also don't want them to come
> > to us, since there is no interoperability testing. It is best that both
> > Hypervisors and their Linux interfaces evolve independently.
> 
> Are above explanations reasonable to not re-use KVM ioctl numbers?

Try getting close at least, where possible please.  As your ioctl
numbers didn't even start at 0, it's a bit odd...

thanks,

greg k-h
  

On 11/10/2022 10:24 PM, Greg Kroah-Hartman wrote:
> On Thu, Nov 10, 2022 at 04:03:10PM -0800, Elliot Berman wrote:
>>> Agree, tools can be updated and that is the easy part as we grow the s/w
>>> stack around Gunyah in userspace, like we already do w/ CrosVM (Virtual
>>> Machine Manager) and QEMU will be next followed by rust-vmm. All of them
>>> can be done without Gunyah ioctls relying anything on the KVM ioctls.
>>> Elliot has also explained very well that we don't to go to KVM
>>> maintainers for any of our additions and we also don't want them to come
>>> to us, since there is no interoperability testing. It is best that both
>>> Hypervisors and their Linux interfaces evolve independently.
>>
>> Are above explanations reasonable to not re-use KVM ioctl numbers?
> 
> Try getting close at least, where possible please.  As your ioctl
> numbers didn't even start at 0, it's a bit odd...

Ack, will do.

Thanks,
Elliot