Re: [Qemu-devel] [RFC v6-based v1 0/5] refine mdev framework

[Kirti Wankhede]

On 8/18/2016 11:55 PM, Alex Williamson wrote:
> On Thu, 18 Aug 2016 16:42:14 +0800
> Dong Jia <address@hidden> wrote:
> 
>> On Wed, 17 Aug 2016 03:09:10 -0700
>> Neo Jia <address@hidden> wrote:
>>
>>> On Wed, Aug 17, 2016 at 04:58:14PM +0800, Dong Jia wrote:  
>>>> On Tue, 16 Aug 2016 16:14:12 +0800
>>>> Jike Song <address@hidden> wrote:
>>>>   
>>>>>
>>>>> This patchset is based on NVidia's "Add Mediated device support" series, 
>>>>> version 6:
>>>>>
>>>>>   http://www.spinics.net/lists/kvm/msg136472.html
>>>>>
>>>>>
>>>>> Background:
>>>>>
>>>>>   The patchset from NVidia introduced the Mediated Device support to
>>>>>   Linux/VFIO. With that series, one can create virtual devices (supporting
>>>>>   by underlying physical device and vendor driver), and assign them to
>>>>>   userspace like QEMU/KVM, in the same way as device assignment via VFIO.
>>>>>
>>>>>   Based on that, NVidia and Intel implemented their vGPU solutions, IBM
>>>>>   implemented its CCW pass-through.  However, there are limitations
>>>>>   imposed by current (v6 in particular) mdev framework: the mdev must be
>>>>>   represented as a PCI device, several vfio capabilities such as
>>>>>   sparse mmap are not possible, and so forth.
>>>>>
>>>>>   This series aims to address above limitations and simplify the 
>>>>> implementation.
>>>>>
>>>>>
>>>>> Key Changes:
>>>>>
>>>>>   - An independent "struct device" was introduced to parent_device, thus
>>>>>     a hierarchy in driver core is formed with physical device, parent 
>>>>> device
>>>>>     and mdev device;
>>>>>
>>>>>   - Leveraging the mechanism and APIs provided by Linux driver core, it
>>>>>     is now safe to remove all refcnts and locks;
>>>>>
>>>>>   - vfio_mpci (later renamed to vfio_mdev) was made BUS-agnostic: all
>>>>>     PCI-specific logic was removed, accesses from userspace are now
>>>>>     passed to vendor driver directly, thus guaranteed that full VFIO
>>>>>     capabilities provided: e.g. dynamic regions, sparse mmap, etc.;
>>>>>
>>>>>     With vfio_mdev being BUS-agnostic, it is enough to have only one
>>>>>     driver for all mdev devices;  
>>>>
>>>> Hi Jike:
>>>>
>>>> I don't know what happened, but finding out which direction this will
>>>> likely go seems my first priority now...  
>>>
>>> Hi Dong,
>>>
>>> Just want to let you know that we are preparing the v7 patches to 
>>> incorporate
>>> the latest review comments from Intel folks and Alex, for some changes in 
>>> this
>>> patch set also mentioned in the recent review are already queued up in the 
>>> new
>>> version.  
>> Hi Neo,
>>
>> Good to know this. :>
>>
>>>   
>>>>
>>>> I'd say, either with only the original mdev v6, or patched this series,
>>>> vfio-ccw could live. But this series saves my work of mimicing the
>>>> vfio-mpci code in my vfio-mccw driver. I like this incremental patches.  
>>>
>>> Thanks for sharing your progress and good to know our current v6 solution 
>>> works 
>>> for you. We are still evaluating the vfio_mdev changes here as I still 
>>> prefer to
>>> share general VFIO pci handling inside a common VFIO PCI driver, and the
>>> modularization will reduce the impact of future changes and potential 
>>> regressions
>>> cross architectures - between PCI and CCW.  
>> If this is something that Alex and the Intel folks are fine with, I have
>> no problem with this too. Thanks,
> 
> Overall, I like this a lot.  Creating a proper device hierarchy and
> letting the driver core manage the references makes a lot of sense and
> the reduction in code volume and complexity speaks for itself.  

We are evaluating on this proposed solution. But the proposed patches
are not tested, those have bugs.

+#define dev_to_parent_dev(_dev) container_of((_dev),   \
+                                            struct parent_device, dev)

This macro itself is not correct and causes kernel crash. This macro
doesn't really takes to 'struct parent_device' as it aimed to.

We are evaluating on how this will change sysfs entries and what will be
the impact of fixing all these bugs or will that really going to help.

> I like
> how the PCI mdev layer goes away, we're not imposing arbitrary
> restrictions on the vendor driver in an attempt to insert a common
> layer.

We were trying to make it more and more configurable for vendor driver
while keeping common code at common place so that code is not replicated
in each vendor driver. We believe with the new version of v7, it will
become a common module instead of pci module.


>  We can add helpers for things that do end up being common as we
> go.  Using devices rather than uuids for functions is a big
> improvement.


This is agreed on reviews of v6 version of my patches. Now we are
introducing ‘online’ instead of start()/stop() and all are in agreement
with that, right?

Thanks,
Kirti

> I hope that Neo and Kirti will incorporate many of these
> changes in their next revision.  Thanks for stepping in with this,
> 
> Alex
>