From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49435) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R5l49-0002lk-RE for qemu-devel@nongnu.org; Mon, 19 Sep 2011 17:07:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R5l48-0006gh-HK for qemu-devel@nongnu.org; Mon, 19 Sep 2011 17:07:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64725) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R5l48-0006gT-8R for qemu-devel@nongnu.org; Mon, 19 Sep 2011 17:07:48 -0400 From: Alex Williamson Date: Mon, 19 Sep 2011 15:07:43 -0600 In-Reply-To: <4E7799DE.2070406@freescale.com> References: <1316445361.4443.29.camel@bling.home> <4E7799DE.2070406@freescale.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Message-ID: <1316466464.4443.66.camel@bling.home> Mime-Version: 1.0 Subject: Re: [Qemu-devel] RFC [v2]: vfio / device assignment -- layout of device fd files List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Scott Wood Cc: kvm@vger.kernel.org, Stuart Yoder , agraf@suse.de, qemu-devel@nongnu.org, avi@redhat.com On Mon, 2011-09-19 at 14:37 -0500, Scott Wood wrote: > On 09/19/2011 10:16 AM, Alex Williamson wrote: > > On Fri, 2011-09-09 at 08:11 -0500, Stuart Yoder wrote: > >> 2. Header > >> > >> The header is located at offset 0x0 in the device fd > >> and has the following format: > >> > >> struct devfd_header { > >> __u32 magic; > >> __u32 version; > >> __u32 flags; > >> }; > >> > >> The 'magic' field contains a magic value that will > >> identify the type bus the device is on. Valid values > >> are: > >> > >> 0x70636900 // "pci" - PCI device > >> 0x64740000 // "dt" - device tree (system bus) > > These look somewhat conflict-prone (even more so than "vfio") -- maybe > not ambiguous in context, but if we're going to use magic numbers we > might as well make them as unique as we can. Can't we just generate a > couple random numbers? > > Also, is the magic number specifically 0x70636900 in native endian, or > "pci" however it would be encoded on the platform? Are there any > platforms in Linux where multiple endians are supported at once in > userspace (on a per-process basis)? A GUID would be fine w/ me. > >> 3. Region > >> > >> A REGION record an addressable address region for the device. > >> > >> struct devfd_region { > >> __u32 type; // must be 0x1 > >> __u32 record_len; > >> __u32 flags; > >> __u64 offset; // seek offset to region from beginning > >> // of file > >> __u64 len ; // length of the region > >> }; > >> > >> The 'flags' field supports one flag: > >> > >> IS_MMAPABLE > >> > >> 4. Device Tree Path (DTPATH) > >> > >> A DTPATH record is a sub-record of a REGION and describes > >> the path to a device tree node for the region > > > > Can we better distinguish sub-records from records? I assume we're > > trying to be as versatile as possible by having a single "type" address > > space, but is this going to lead to implementation problems? > > What kind of problems? vvv Those kind. > > A DTPATH as a record, an INTERRUPT as a sub-record, etc. > > Same as any other unrecognized (sub)record type, you ignore it -- but > the kernel should not be generating this. I'm trying to express that I think the spec is unclear here. It's easy to hand wave that the code will do the right thing, but if the next person comes along and doesn't understand that a DTPATH is only a sub-type and a DTINDEX needs to be paired with a DTPATH, then we've failed. > > Should we instead have > > a "subtype" address space per "type" and per device type? For a "dt" > > device, it looks like we really have: > > > > * REGION (type 0) > > * DTPATH (subtype 0) > > * DTINDEX (subtype 1) > > * PHYS_ADDR (subtype 2) > > * INTERRUPT (type 1) > > * DTPATH (subtype 0) > > * DTINDEX (subtype 1) > > > > While "pci" is: > > > > * REGION (type 0) > > * PCI_CONFIG_SPACE (subtype 0) > > * PCI_BAR_INDEX (subtype 1) > > * INTERRUPT (type 1) > > I'd prefer to keep one numberspace, with documented restrictions on what > types/subtypes are allowed in each context. Makes it easier if we end > up in a situation where a (sub)record type is applicable to multiple > contexts, and easier to detect when an error has been made. Couldn't we accomplish the same with separate type and subtype number spaces? enum types { REGION, INTERRUPT, }; enum subtypes { DTPATH, DTINDEX, PHYS_ADDR, PCI_CONFIG_SPACE, PCI_BAR_INDEX, }; I just find it confusing that we intermix them when defining them. Thanks, Alex > >> 8. PCI Bar Index (PCI_BAR_INDEX) > >> > >> A PCI_BAR_INDEX record is a sub-record of a REGION record > >> and identifies the PCI BAR index for the region. > >> > >> struct devfd_barindex { > >> __u32 type; // must be 0x6 > >> __u32 record_len; > >> __u32 flags; > >> __u32 bar_index; > >> } > > > > I suppose we're more concerned with easy parsing and alignment than > > compactness, so a u32 to differentiate 6 BARS + 1 ROM is probably ok. > > Right. > > -Scott >