Re: [Qemu-devel] Machine description as data prototype, take 3 (was: [RFC] Machine description as data)

[Blue Swirl <blauwirbel@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4467 bytes --]

On 2/19/09, Markus Armbruster <armbru@redhat.com> wrote:
> Third iteration of the prototype.
>
>  What about an early merge?  If your answer to that is "yes, but", what
>  exactly do you want changed?

Not until the device tree discussion is finished and Qemu release is
out. This isn't something we want to rush in. There is still Paul's
development and even Fabrice's original proposal which both have
relative merits.

>  +static int
>  +dt_parse_int(void *dst, const char *src, dt_prop_spec *spec)

dst should be uint64_t *.

>  +{
>  +    char *ep;
>  +    long val;

uint64_t val

>  +
>  +    assert(spec->size == sizeof(int));
>  +    errno = 0;
>  +    val = strtol(src, &ep, 0);

strtoull

>  +    if (*ep || ep == src || errno || (int)val != val)
>  +        return -1;
>  +    *(int *)dst = val;
>  +    return 0;
>  +}
>  +
>  +static int
>  +dt_parse_ram_addr_t(void *dst, const char *src, dt_prop_spec *spec)

ram_addr_t *dst

>  +{
>  +    char *ep;
>  +    unsigned long val;

ram_addr_t val

>  +
>  +    assert(spec->size == sizeof(ram_addr_t));
>  +    errno = 0;
>  +    val = strtoul(src, &ep, 0);

strtoull

>  +typedef struct dt_device_cpus {
>  +    const char *model;
>  +    int num;
>  +} dt_device_cpus;
>  +
>  +static dt_prop_spec dt_cpus_props[] = {
>  +    DT_PROP_SPEC_INIT("model", dt_device_cpus, model, string),
>  +    DT_PROP_SPEC_INIT("num", dt_device_cpus, num, int),
>  +};

There should be one node for each cpu, not "num". Each node is named
after the CPU model, like /SUNW,UltraSPARC-IIi.

>  +static dt_prop_spec dt_memory_props[] = {
>  +    DT_PROP_SPEC_INIT("ram", dt_device_memory, ram_size, ram_addr_t),
>  +};

Memory node should be name "/memory". It has properties "available"
and "reg", in this case we only want "reg". "reg" property consists of
several phys_addr, size pairs.

>  +static dt_prop_spec dt_pc_misc_props[] = {
>  +    DT_PROP_SPEC_INIT("boot-device", dt_device_pc_misc, boot_device,
>  +                      string),
>  +};

This property is quite standard, the correct place is under "/options".

>  +static dt_prop_spec dt_vga_props[] = {
>  +    DT_PROP_SPEC_INIT("model", dt_device_vga, model, string),
>  +    DT_PROP_SPEC_INIT("ram", dt_device_vga, ram_size, ram_addr_t),

Again, there is no "model" property, but the node name specifies the model.

"ram" is not correct, this should be under "reg" property.

>  +static dt_prop_spec dt_nic_props[] = {
>  +    DT_PROP_SPEC_INIT("model", dt_device_nic, nd.model, string),
>  +    DT_PROP_SPEC_INIT("mac", dt_device_nic, nd.macaddr, macaddr),
>  +    DT_PROP_SPEC_INIT("name", dt_device_nic, nd.name, string),
>  +};

"name" is the node name, you can't use it to anything else.

Again, node name should specify the model.

>  +    root = tree_new_kid(NULL, "", NULL);
>  +    leaf = tree_new_kid(root, "cpus", NULL);
>  +    tree_put_propf(leaf, "model", "%s", "qemu32");
>  +    leaf = tree_new_kid(root, "memory", NULL);
>  +    leaf = tree_new_kid(root, "pc-misc", NULL);

Remove pc-misc.

>  +    pci = tree_new_kid(root, "pci", NULL);
>  +    leaf = tree_new_kid(pci, "piix3", NULL);

"piix3" is equal to "pci". In this case, there will not be any "piix3"
node, "pci" takes it's place. Any known PCI devices use either their
class (like "pci" for PCI bridges) or model specific name, like
"ebus".

>  +    node = tree_node_by_name(pci, "piix3");
>  +    for(i = 0; i < MAX_IDE_BUS * MAX_IDE_DEVS; i++) {
>  +        index = drive_get_index(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
>  +        if (index != -1)
>  +            dt_attach_drive(host, index, node, drives_table[index].bdrv);
>  +    }

For the PIIX IDE controller (under "/pci" node) the correct name is "ide".

>  +    /* Floppy */
>  +    node = tree_node_by_name(conf, "/pc-misc");
>  +    for(i = 0; i < MAX_FD; i++) {
>  +        index = drive_get_index(IF_FLOPPY, 0, i);
>  +        if (index != -1)
>  +            dt_attach_drive(host, index, node, drives_table[index].bdrv);
>  +    }

ISA devices should be put either under a special "/isa" node, or if
there is an PCI-to-ISA bridge, "/pci/isa" or whatever the connection
is.

I have a troubling feeling that you have not read the 1275 standard or
looked how real OpenFirmware machines name things. I've attached a
Sparc64 tree as an example, please also read the OF standards at:

http://playground.sun.com/pub/p1275/

I'd still like to thank you for your efforts so far, this is a
workable starting point.

[-- Attachment #2: fire-t200-tree.bz2 --]
[-- Type: application/x-bzip2, Size: 5823 bytes --]