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

[Eric Auger <eric.auger@linaro.org>]

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?

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

Best Regards

Eric
> 
> 
> Alex
>