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

[Scott Wood]

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)?

>> 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?

> 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.

> 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.

>> 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