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

dt_parse_int() is for safe and convenient parsing into an int variable.
This includes range checking.

Other integer types need their own parsing methods.  To be created as
needed.

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

Okay.

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

Only if people who see different aspects of the whole picture neglect to
contribute.

Attempting to create a flawless diamond from the start tends to deliver
expensive solutions to slightly wrong problems late.

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

I went for the stupidest solution that could possibly work.  That
solution happens to compress multiple identical CPUs into a single node.
Dropping that should be easy enough.

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

There are problems with using OF *now*, namely the ones I've mentioned
repeatedly:

1. Whether OF is a good match for configuring QEMU remains to be seen.

2. I can only juggle so many balls at the same time.  The reason I was
able to deliver a workable starting point was that I picked the smallest
number of balls that still promised to be interesting.  The 1275 ball
isn't among them, sorry.  That does not mean I refuse to touch that
ball.  It just means that if I add it now, I'll likely drop all the
balls.  Which won't help anybody.

The OF ball can be added later.  Until then we can certainly keep it in
mind, and avoid decisions that make its later addition harder
needlessly.

After the initial merge, my personal opinions and limitations become
less relevant.  You could replace my configuration data encoding
wholesale by OF then, without being held back by my ignorance about it.