Re: [PATCH v11 10/26] gunyah: vm_mgr: Introduce basic VM Manager

[Elliot Berman]

On 3/31/2023 7:25 AM, Alex Elder wrote:
> On 3/3/23 7:06 PM, Elliot Berman wrote:
> > @@ -129,6 +131,7 @@ struct gh_rm_connection {
> >    * @cache: cache for allocating Tx messages
> >    * @send_lock: synchronization to allow only one request to be sent 
> > at a time
> >    * @nh: notifier chain for clients interested in RM notification 
> > messages
> > + * @miscdev: /dev/gunyah
> >    */
> >   struct gh_rm {
> >       struct device *dev;
> > @@ -145,6 +148,8 @@ struct gh_rm {
> >       struct kmem_cache *cache;
> >       struct mutex send_lock;
> >       struct blocking_notifier_head nh;
> > +
> > +    struct miscdevice miscdev;
> >   };
> >   /**
> > @@ -593,6 +598,21 @@ void gh_rm_put(struct gh_rm *rm)
> >   }
> >   EXPORT_SYMBOL_GPL(gh_rm_put);
>
> I feel like /dev/gunyah code would more appropriately be found
> in "vm_mgr.c".  All gh_dev_ioctl() does is call the function
> defined there, and it's therefore a VM-oriented rather than
> resource-oriented device.

I'd like to keep the gh_dev_ioctl where it is because it keeps the 
struct gh_rm explicitly private to rsc_mgr.c and thinking this helps 
keep the design cleaner long term by preventing new members from 
sneaking into struct gh_rm.

> > +
> > +static long gh_dev_ioctl_create_vm(struct gh_rm *rm, unsigned long arg)
> > +{
> > +    struct gh_vm *ghvm;
> > +    struct file *file;
> > +    int fd, err;
> > +
> > +    /* arg reserved for future use. */
>
> Do you have a clear idea of how this might be used in the future?

Not yet. I have some vague ideas to use it as a enumeration of "special" 
VM types. We might have special number for VMs which use "protected VM 
firmware"  for the Android boot flow, another number for the "Trusted UI 
VM", another for "OEM VM", etc. Passing 0 would always be the 
unauthenticated VM which we are creating today.

We're considering bumping the info to a separate ioctl since additional 
info needs to be passed from userspace to configure the VM. Userspace 
would do GH_CREATE_VM(). Another ioctl like GH_VM_SET_PVMFW_ADDRESS() 
would imply that the VM uses the protected VM firmware for the Android 
boot flow. Another ioctl call would be used to imply the "Trusted UI 
VM". In any case, we're still in early design phase.

> I was thinking you could silently ignore the argument value, but
> I suppose if it *does* get used in the future, you want the caller
> to know it's being ignored.  (Is that right?)

That's right.

Thanks,
Elliot