From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40026) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X06Pt-0007Pm-Tc for qemu-devel@nongnu.org; Thu, 26 Jun 2014 05:56:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X06Po-0003Ou-Bg for qemu-devel@nongnu.org; Thu, 26 Jun 2014 05:56:29 -0400 Received: from cantor2.suse.de ([195.135.220.15]:38311 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X06Po-0003Nu-1E for qemu-devel@nongnu.org; Thu, 26 Jun 2014 05:56:24 -0400 Message-ID: <53ABEE45.8010204@suse.de> Date: Thu, 26 Jun 2014 11:56:21 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1401695374-4287-1-git-send-email-eric.auger@linaro.org> <1401695374-4287-4-git-send-email-eric.auger@linaro.org> <53AB3D55.2000802@suse.de> <53ABD010.3060705@linaro.org> In-Reply-To: <53ABD010.3060705@linaro.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v3 03/10] vfio: add vfio-platform support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Auger , eric.auger@st.com, christoffer.dall@linaro.org, qemu-devel@nongnu.org, kim.phillips@freescale.com, a.rigo@virtualopensystems.com Cc: peter.maydell@linaro.org, patches@linaro.org, Kim Phillips , stuart.yoder@freescale.com, alex.williamson@redhat.com, christophe.barnichon@st.com, a.motakis@virtualopensystems.com, kvmarm@lists.cs.columbia.edu 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 >>> >>> 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 >>> Signed-off-by: Eric Auger >>> --- >>> 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