From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50345) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKzdO-0005DU-UJ for qemu-devel@nongnu.org; Tue, 04 Mar 2014 19:24:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKzdE-0003us-Te for qemu-devel@nongnu.org; Tue, 04 Mar 2014 19:24:30 -0500 Received: from mail-oa0-f53.google.com ([209.85.219.53]:44572) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKzdE-0003uo-OE for qemu-devel@nongnu.org; Tue, 04 Mar 2014 19:24:20 -0500 Received: by mail-oa0-f53.google.com with SMTP id j17so316518oag.12 for ; Tue, 04 Mar 2014 16:24:19 -0800 (PST) Date: Tue, 4 Mar 2014 18:24:18 -0600 From: Kim Phillips Message-Id: <20140304182418.7840d09f8254faaac0d36e65@linaro.org> In-Reply-To: <1393610598.26901.21.camel@ul30vt.home> References: <1393382272-29021-1-git-send-email-kim.phillips@linaro.org> <1393382272-29021-3-git-send-email-kim.phillips@linaro.org> <1393610598.26901.21.camel@ul30vt.home> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC 2/2] hw/misc/vfio: add vfio-platform support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: alex.williamson@redhat.com Cc: peter.maydell@linaro.org, kim.phillips@freescale.com, eric.auger@linaro.org, stuart.yoder@freescale.com, qemu-devel@nongnu.org, agraf@suse.de, a.motakis@virtualopensystems.com, kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org On Fri, 28 Feb 2014 11:03:18 -0700 Alex Williamson wrote: > On Tue, 2014-02-25 at 20:37 -0600, Kim Phillips wrote: > > - support allocating a variable number of regions > > - VFIODevice's bars[] become dynamically allocated *regions > > - VFIOBAR's device fd replaced with parent VFIODevice ptr, > > to facilitate BAR r/w ops calling vfio_eoi() > > On one hand, I had assumed we'd create a hw/misc/vfio/ directory and > perhaps split things into pci, common, vga, and platform. We already > need the pci vs vga split as there's lots of irrelevant and complicated > vga quirks that most people don't want to see. On the other hand, I'm > surprised how much of the PCI code you can re-use. Still, I think it > would be cleaner to create a VFIOPCIDevice and a VFIOPlatformDevice. sounds good, had started down that path but reverted it because I too realized how much code was indeed being reused. I'm ok with VFIOPCIDevice and a separate VFIOPlatformDevice for now, thanks. > > static void vfio_map_bars(VFIODevice *vdev) > > { > > int i; > > > > + if (!vdev->config_size) { > > + /* platform device */ > > + for (i = 0; i < vdev->nr_regions; i++) { > > + vfio_map_region(vdev, i); > > + } > > + return; > > + } > > I don't understand this, vfio_map_region() calls vfio_mmap_bar(), but > vfio_mmap_bar() has been gutted so that it only initializes the mmap > subregion, but never adds it. vfio_map_bar() does both the > initialization of the slow, read/write, memory region as well as adding > the mmap sub-region. I don't see where platform devices get a memory > region inserted anywhere into the guest address space. it's being done by the board code via the first RFC/patch ("hw/arm/virt: add a Calxeda XGMAC device"), and vfio_mmap_bar()'s call to memory_region_init_ram_ptr(). Yeah, the slow, r/w ops region path isn't used, and I'm not sure how to make it work without addressing the problems the first RFC presents. > > - if (dev_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) { > > + if (dev_info.num_regions > PCI_NUM_REGIONS) || > > + ((dev_info.flags & VFIO_DEVICE_FLAGS_PCI) && > > + (dev_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1)) { > > Now we start to have platform devices error out if they have more > regions than PCI knows about... That doesn't make much sense. right, I should have made it more clear that the multiple-regions code is sketchy in the "support allocating a variable number of regions" blurb in the commit text of this RFC. > > +static void register_vfio_platform_dev_type(void) > > +{ > > + type_register_static(&vfio_platform_dev_info); > > +} > > + > > +type_init(register_vfio_platform_dev_type) > > This all looks reasonable, but I suspect it would be cleaner if > vfio_find_get_group() was in a common file along with basic mmap and > read/write access functions. Thanks, so rename existing hw/misc/vfio.c to its original name vfio-pci.c, and load all common functions back into a vfio.c? Kim