Re: [Qemu-devel] Machine description as data prototype, take 3

[Markus Armbruster]

Blue Swirl <address@hidden> writes:

> On 2/19/09, Markus Armbruster <address@hidden> 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

The first parameter is void * because this is a dt_prop_spec parse
method.

This particular method parses int, not uint64_t.

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

Not a good idea, I fear.  I use the type returned by strtoul(), because
that ensures there's no truncation in the assignment.  The conversion to
ram_addr_t happens later, in the part you snipped, and is carefully
checked for truncation.

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

Makes sense if we want to support ram_addr_t wider than long.  Do we?

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

To be honest, I read just enough on 1275 to

1. develop doubts on whether it is a good match for the problem
(discussed elsewhere in this thread), and

2. more importantly, realize that if I set out to master 1275 before
touching code, I'd certainly get bogged down in details before I could
accomplish anything useful, and/or get too bored to continue.

So I decided to once again exercise the three principal virtues
(Laziness, Impatience, and Hubris) and just go ahead and create some
working code, so we can have the kind of productive discussion we're
having now.

Let me stress: so far my work has *not* been about bringing 1275 or any
other configuration data structure to QEMU.  It's been chiefly exploring
how to configure and build a virtual machine, driven by configuration
data, talking to device code through an abstract device interface.  I
feel that details of configuration data encoding, like whether something
is encoded in the node name or a property, are entirely tangential to
that effort.  How exactly you decorate those trees doesn't affect the
abstract device interface at all.  It affects the machine builder, but I
doubt it affects it structurally.

It goes without saying that I'm fully prepared to change my
configuration data encoding.  However, I'd like to tackle the
restructuring Anthony recommended first.  Once I got that done, I'll be
happy to revisit your recommendations on config data encoding.

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

Thanks, that's encouraging.