Re: [Qemu-devel] RFC [v2]: vfio / device assignment -- layout of device fd files

[Alex Williamson]

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
>