From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LaDf9-0003YS-7b for qemu-devel@nongnu.org; Thu, 19 Feb 2009 13:30:19 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LaDf7-0003Xq-Km for qemu-devel@nongnu.org; Thu, 19 Feb 2009 13:30:18 -0500 Received: from [199.232.76.173] (port=37414 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LaDf7-0003Xe-Cn for qemu-devel@nongnu.org; Thu, 19 Feb 2009 13:30:17 -0500 Received: from mx2.redhat.com ([66.187.237.31]:34927) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LaDf6-0004Nq-Mh for qemu-devel@nongnu.org; Thu, 19 Feb 2009 13:30:17 -0500 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n1JIUCWe028604 for ; Thu, 19 Feb 2009 13:30:14 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n1JIUCU4006656 for ; Thu, 19 Feb 2009 13:30:13 -0500 Received: from pike.pond.sub.org (vpn-10-52.str.redhat.com [10.32.10.52]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n1JIUAll011314 for ; Thu, 19 Feb 2009 13:30:11 -0500 Subject: Re: [Qemu-devel] Machine description as data prototype, take 3 References: <87iqnh6kyv.fsf@pike.pond.sub.org> <871vtuafdr.fsf@pike.pond.sub.org> From: Markus Armbruster Date: Thu, 19 Feb 2009 19:30:10 +0100 In-Reply-To: (Blue Swirl's message of "Thu\, 19 Feb 2009 18\:40\:10 +0200") Message-ID: <87bpsy1dql.fsf@pike.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Blue Swirl writes: > On 2/19/09, Markus Armbruster 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.