Re: [Qemu-devel] [RFC v3 03/10] vfio: add vfio-platform support

[Alexander Graf <agraf@suse.de>]

On 26.06.14 09:47, Eric Auger wrote:
> On 06/25/2014 11:21 PM, Alexander Graf wrote:
>> On 02.06.14 09:49, Eric Auger wrote:
>>> From: Kim Phillips <kim.phillips@linaro.org>
>>>
>>> Functions for which PCI and platform device support share are moved
>>> into common.c.  The common vfio_{get,put}_group() get an additional
>>> argument, a pointer to a vfio_reset_handler(), for which to pass on
>>> to qemu_register_reset, but only if it exists (the platform device
>>> code currently passes a NULL as its reset_handler).
>>>
>>> For the platform device code, we basically use SysBusDevice
>>> instead of PCIDevice.  Since realize() returns void, unlike
>>> PCIDevice's initfn, error codes are moved into the
>>> error message text with %m.
>>>
>>> Currently only MMIO access is supported at this time.
>>>
>>> The perceived path for future QEMU development is:
>>>
>>> - add support for interrupts
>>> - verify and test platform dev unmap path
>>> - test existing PCI path for any regressions
>>> - add support for creating platform devices on the qemu command line
>>>     - currently device address specification is hardcoded for test
>>>       development on Calxeda Midway's fff51000.ethernet device
>>> - reset is not supported and registration of reset functions is
>>>     bypassed for platform devices.
>>>     - there is no standard means of resetting a platform device,
>>>       unsure if it suffices to be handled at device--VFIO binding time
>>>
>>> [1] http://www.spinics.net/lists/kvm-arm/msg08195.html
>>>
>>> Changes (v2 -> v3):
>>> [work done by Eric Auger]
>>>
>>> This new version introduces 2 separate VFIO Device objects:
>>> - VFIOPCIDevice
>>> - VFIOPlatformDevice
>>>
>>> Both objects share a VFIODevice struct.
>>>
>>> Also a VFIORegion shared struct was created. It is embedded in
>>> VFIOBAR struct. VFIOPlatformDevice uses VFIORegion directly.
>>>
>>> Introducing those base classes induced quite a lot of tiny
>>> changes in the PCI code. Theoretically PCI and platform
>>> devices can be supported simultaneously. PCI modifications
>>> currently are not tested.
>>>
>>> The VFIODevice is not a QOM object due to the single inheritance
>>> model limitation.
>>>
>>> The VFIODevice struct embeds an ops structure which is
>>> specialized in each VFIO leaf device. This makes possible to call
>>> device specific functions in common parts, hence achieving better
>>> factorization.
>>>
>>> Reset handling typically is handled that way where a unique
>>> generic ResetHandler (in common.c) is used for both derived
>>> classes. It calls device specific methods.
>>>
>>> As in the original contribution, only MMIO is supported in that
>>> patch file (in mmap mode). IRQ support comes in a subsequent patch.
>>>
>>> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>> ---
>>>    hw/vfio/Makefile.objs      |    2 +
>>>    hw/vfio/common.c           |  849 ++++++++++++++++++++++++++++
>>>    hw/vfio/pci.c              | 1316
>>> ++++++++++----------------------------------
>>>    hw/vfio/platform.c         |  267 +++++++++
>>>    hw/vfio/vfio-common.h      |  143 +++++
>>>    linux-headers/linux/vfio.h |    1 +
>>>    6 files changed, 1565 insertions(+), 1013 deletions(-)
>> This patch is impossible to review. Please split it out into a part (or
>> maybe even multiple patches) that separate pci specific VFIO support
>> into its own file and then another set of patches implementing platform
>> support.
> Hi Alex,
>
> OK I will reorganize the patch as you suggest.
>
> Alex (both of you), do you think the general split direction looks
> reasonable, ie. having this VFIODevice struct and VFIORegion struct? or
> do you think it induces too many  (and possibly cumbersome) changes in
> the PCI code?

I don't think we'll be able to tell before the implications are visible 
- and that basically means splitting the patch :(.

> wrt VFIOPlatformDevice I changed the code to inherit from PlatformDevice
> instead of SysBusDevice - maybe too early but future will say ... - .

I'll check out which way to go forward there today. But either way, the 
two aren't incredibly different.


Alex