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

[Blue Swirl]

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

But we want to support 64 bit stuff as well, with this change it's easy.

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

No, I don't think so. I was again thinking of 64 bit memory addresses,
but "long" should still be 64 bits in that case.

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

That approach may produce something that works but it may be something
that is not compatible with the whole picture, or creates unnecessary
shuffling elsewhere.

In this case, there are machines using OF (Sparc32, Sparc64 and PPC),
so the machine config design should be compatible with the OF
structures.

Here's a concrete example: You proposed /cpus/num, whereas the OF way
is adding a number of CPU nodes. It is possible to convert between the
two (if all CPU properties were identical), but it's just unnecessary
work.

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

Well, then there should be no problem using the OF model as much as possible?

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