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
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
