Re: [PATCH v6 1/4] vfio: Mediated device Core driver

[Alex Williamson <alex.williamson@redhat.com>]

On Sat, 13 Aug 2016 00:14:39 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 8/10/2016 12:30 AM, Alex Williamson wrote:
> > On Thu, 4 Aug 2016 00:33:51 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> > 
> > This is used later by mdev_device_start() and mdev_device_stop() to get
> > the parent_device so it can call the start and stop ops callbacks
> > respectively.  That seems to imply that all of instances for a given
> > uuid come from the same parent_device.  Where is that enforced?  I'm
> > still having a hard time buying into the uuid+instance plan when it
> > seems like each mdev_device should have an actual unique uuid.
> > Userspace tools can figure out which uuids to start for a given user, I
> > don't see much value in collecting them to instances within a uuid.
> >   
> 
> Initially we started discussion with VM_UUID+instance suggestion, where
> instance was introduced to support multiple devices in a VM.

The instance number was never required in order to support multiple
devices in a VM, IIRC this UUID+instance scheme was to appease NVIDIA
management tools which wanted to re-use the VM UUID by creating vGPU
devices with that same UUID and therefore associate udev events to a
given VM.  Only then does an instance number become necessary since the
UUID needs to be static for a vGPUs within a VM.  This has always felt
like a very dodgy solution when we should probably just be querying
libvirt to give us a device to VM association.

> 'mdev_create' creates device and 'mdev_start' is to commit resources of
> all instances of similar devices assigned to VM.
> 
> For example, to create 2 devices:
> # echo "$UUID:0:params" > /sys/devices/../mdev_create
> # echo "$UUID:1:params" > /sys/devices/../mdev_create
> 
> "$UUID-0" and "$UUID-1" devices are created.
> 
> Commit resources for above devices with single 'mdev_start':
> # echo "$UUID" > /sys/class/mdev/mdev_start
> 
> Considering $UUID to be a unique UUID of a device, we don't need
> 'instance', so 'mdev_create' would look like:
> 
> # echo "$UUID1:params" > /sys/devices/../mdev_create
> # echo "$UUID2:params" > /sys/devices/../mdev_create
> 
> where $UUID1 and $UUID2 would be mdev device's unique UUID and 'params'
> would be vendor specific parameters.
> 
> Device nodes would be created as "$UUID1" and "$UUID"
> 
> Then 'mdev_start' would be:
> # echo "$UUID1, $UUID2" > /sys/class/mdev/mdev_start
> 
> Similarly 'mdev_stop' and 'mdev_destroy' would be:
> 
> # echo "$UUID1, $UUID2" > /sys/class/mdev/mdev_stop

I'm not sure a comma separated list makes sense here, for both
simplicity in the kernel and more fine grained error reporting, we
probably want to start/stop them individually.  Actually, why is it
that we can't use the mediated device being opened and released to
automatically signal to the backend vendor driver to commit and release
resources? I don't fully understand why userspace needs this interface.

> and
> 
> # echo "$UUID1" > /sys/devices/../mdev_destroy
> # echo "$UUID2" > /sys/devices/../mdev_destroy
> 
> Does this seems reasonable?

I've been hoping we could drop the instance numbers and create actual
unique UUIDs per mediated device for a while ;)  Thanks,

Alex