On Tue, Feb 9, 2016 at 6:08 PM, Barret Rhoden <[email protected]> wrote:
> Hi - > > Thanks for the ACPI work. It's a nasty beast. Darn skippy. :-/ Comments below. > > On 2016-02-05 at 15:40 Dan Cross <[email protected]> wrote: > > The following changes since commit > > d0610298a0e1ed7f04b44f96a474d3ee424adfab: > > > > Bind #random to /dev during ifconfig (2016-02-05 10:29:34 -0500) > > > > are available in the git repository at: > > > > [email protected]:dancrossnyc/akaros.git > > Obligatory checkpatch output: > > ../patches/0001-ACPI-changes-for-DMAR-and-new-directory-hierarchy.patch > WARNING: externs should be avoided in .c files > #92: FILE: kern/arch/x86/mpacpi.c:25: > +extern struct Atable *apics; > > WARNING: Missing a blank line after declarations > #156: FILE: kern/arch/x86/topology.c:87: > + struct Srat *temp = srat->children[i]->tbl; > + if (temp != NULL && temp->type == SRlapic) { > > WARNING: Missing a blank line after declarations > #176: FILE: kern/arch/x86/topology.c:104: > + struct Apicst *temp = apics->children[i]->tbl; > + if (temp != NULL && temp->type == ASlapic) > > WARNING: Missing a blank line after declarations > #193: FILE: kern/arch/x86/topology.c:116: > + struct Srat *st = tail[i]->tbl; > + if (st->type == SRlapic) > > WARNING: Missing a blank line after declarations > #215: FILE: kern/arch/x86/topology.c:134: > + struct Srat *temp = srat->children[i]->tbl; > + if (temp != NULL && temp->type == SRlapic) > > total: 0 errors, 5 warnings, 2360 lines checked > I believe I've addressed all these. > From 45b70011982485e744619bab25f0ff43d1930889 Mon Sep 17 00:00:00 2001 > > From: Dan Cross <[email protected]> > > Date: Fri, 5 Feb 2016 15:36:13 -0500 > > Subject: ACPI changes for DMAR and new directory hierarchy. > > > > Add the DMAR parser, and rationalize the ACPI directory > > hierarchy to make it traversable from the shell. There > > is additional work to do here on the latter, but that is > > not critical path and this gets Gan the DMAR code he > > needs for virtualization. > > > > For future reference, this is a little difficult to review since it's > all in one huge change. There's a lot going on. > Very fair; it's a tad difficult in this particular change for the reasons we've discussed face to face, and also because the changes are (I think) pretty tightly coupled to one another. Changing the struct Atable sort of forces a lot of it; the DMAR changes could have been factored out into a separate commit though. What sorts of things are broken? Here's something that probably didn't > work right: > > / $ cat \#acpi/APIC/0/pretty > (spits out stuff about the FADT, and in general looks like the same > output as #acpi/pretty). > > / $ cat \#acpi/APIC/table > no tables > That's about the long and short of it: All `pretty` files emit the same data, table is empty. Does anything have a table? Is that going to be the raw pointer to > Atable->tbl, or its dump or something? > It should be a dump; right now it does nothing. / $ ls \#acpi > APIC FACP HPET SSDT pretty raw table > > Is the APIC the MADT? > It is. Logical, isn't it? That's inherited from Plan 9. > diff --git a/kern/arch/x86/devarch.c b/kern/arch/x86/devarch.c > > > @@ -65,6 +65,9 @@ enum { > > Qrealmem, > > Qmsr, > > Qperf, > > + Qapic, > > + Qioapic, > > + Qbase, > > > > Qmax, > > }; > > @@ -84,6 +87,9 @@ static struct dirtab archdir[Qmax] = { > > {"realmem", {Qrealmem, 0}, 0, 0444}, > > {"msr", {Qmsr, 0}, 0, 0666}, > > {"perf", {Qperf, 0}, 0, 0666}, > > + {"apic", {Qapic, 0}, 0, 0444}, > > + {"ioapic", {Qioapic, 0}, 0, 0444}, > > + {"realmodemem", {Qrealmem, 0}, 0, 0664}, > > Are all of these changes left over from some other commit or WIP? I > don't see them being used here. If it's just about removing them from > #acpi, we can just remove them completely. > Strange; detritus of something. It's odd because it's completely unrelated. An integration gone wrong. > diff --git a/kern/arch/x86/mpacpi.c b/kern/arch/x86/mpacpi.c > > @@ -22,7 +22,7 @@ > > #include <arch/ioapic.h> > > #include <arch/topology.h> > > > > -extern struct Madt *apics; > > +extern struct Atable *apics; > > Can probably remove this, and #include acpi.h. > Done. > diff --git a/kern/drivers/dev/acpi.c b/kern/drivers/dev/acpi.c > > > + // The type is the qid.path mod NQtypes. > > + Qdir = 0, > > + Qpretty, > > + Qraw, > > + Qtbl, > > + NQtypes, > > + > > + QIndexShift = 8, > > + QIndexMask = (1<<QIndexShift)-1, > > Minor code style thing: can you put spaces around your operators? > Checkpatch doesn't (always?) complain about those for some reason. > Sure! Done. > +}; > > + > > +#define ATABLEBUFSZ ROUNDUP(sizeof(struct Atable), 16) > > Is this based on the assumption that anything a caller of mkatable wants > to add to Atable->tbl will need to be at most 16 bytes aligned? (since > mkatable() does the allocation for its caller for t->tbl). > Yes. I believe that's the guarantee malloc() makes in the C standard (don't quite me on that). > +/* > > + * Lists to store RAM that we copy ACPI tables into. When we map a new > > + * ACPI list into the kernel, we copy it into a specifically RAM buffer > > + * (to make sure it's not coming from e.g. slow device memory). We store > > + * pointers to those buffers on these lists. > > + */ > > +struct Acpilist { > > + struct Acpilist *next; > > + size_t size; > > + int8_t raw[]; > > }; > > FYI, the old ACPI stuff (like much of Plan 9) was out of sync with our > style guide, especially regarding struct, function, and variable > names. We follow the Linux convention of not using uppercase names for > and using underscores. We never got around to renaming things like > Atable to acpi_tbl (or whatever). I'm mentioning this now since you're > adding a new struct in the old plan 9 naming style. > > I'm fine with waiting til after we get this merged, and we can do an > spatch to rename all of the stuff or otherwise do an ACPI-wide cleanup. > Fair enough. Let's clean that up after this goes in. > static void loaddsdt(uintptr_t pa) > > { > > - int n; > > + size_t n; > > uint8_t *dsdtp; > > > > dsdtp = sdtmap(pa, &n, 1); > > - if (dsdtp == NULL) { > > + if (dsdtp == NULL) > > return; > > - } > > } > > Was there supposed to be some sort of error handling here? It looks > like we just return, regardless of whether or not dsdtp == NULL. I > realize that the code was messed up before this commit. > It certainly looks that way. I added a print to at least acknowledge it. If I had to hazard a guess, I'd imagine that this is an acceptable condition on some hardware and so Nemo simply deleted the print statement. > +static struct Atable *parsedmar(struct Atable *parent, > > + char *name, uint8_t *raw, size_t > rawsize) > > +{ > > + struct Atable *t, *tt; > > + int i; > > + int baselen = rawsize > 38 ? 38 : rawsize; > > We have MIN() and MAX() available in the kernel. > Done. > int acpiinit(void) > > { > > - /* this smicmd test implements 'run once' for now. */ > > - if (fadt.smicmd == 0) { > > - //fmtinstall('G', Gfmt); > > - acpirsdptr(); > > - if (fadt.smicmd == 0) { > > + /* This implements 'run once' for now. */ > > We actually have run_once(), k/i/r/common.h. > Cool. Changed accordingly. > + if (root == NULL) { > > + parsersdptr(); > > + if (root == NULL) > > return -1; > > - } > > + printk("ACPI initialized\n"); > > } > > - printk("ACPI initialized\n"); > > return 0; > > } > > > static struct chan *acpiattach(char *spec) > > > - > > - /* > > +#ifdef CONFIG_WE_ARE_NOT_WORTHY > > I gather that's not a real CONFIG var. =) Though if we ever turn it > on, the code protected by it won't compile (which probably is why > intrenable() was commented out). Changed to be a little more descriptive. > -static int acpistat(struct chan *c, uint8_t * dp, int n) > > +static int acpistat(struct chan *c, uint8_t *dp, int n) > > { > > - return devstat(c, dp, n, acpidir, ARRAY_SIZE(acpidir), acpigen); > > + struct Atable *a = genatable(c); > > + > > + if (a == NULL) > > + return -1; > > Based on the asserts in genatable (that a != NULL), this test should > never happen. > Done. j > Is the genatable invariant that #acpi code will never have a chan that > doesn't point to a valid Atable? (If so, it might be worth a comment). > That is correct. Added some words. > > + if (c->qid.type == QTDIR) > > + a = a->parent; > > + assert(a != NULL); > > What's the deal with grabbing the parent's table? > > > > + /* > > + * Note that devstat hard-codes a test against the location of > 'devgen', > > + * so we pretty much have to pass it here. > > devwalk() is the one with the test, not devstat. Unless I missed it. > You are correct. Comments changed to reflect. > + */ > > + > > + return devstat(c, dp, n, a->cdirs, a->nchildren + NQtypes, devgen); > > I was expecting just to call devstat with acpigen. Is this all related > to the devgen check? > Indeed it is. For whatever odd reason, it doesn't work if you pass acpigen here. Added a TODO to eventually track that down. > static long acpiread(struct chan *c, void *a, long n, int64_t off) > > { > > long q; > > @@ -1571,58 +1918,53 @@ static long acpiread(struct chan *c, void *a, > long n, int64_t off) > > } > > > + case Qtbl: > > + s = ttext; > > + e = ttext + tlen; > > + strlcpy(s, "no tables\n", tlen); > > + for (t = tfirst; t != NULL; t = t->next) { > > When does tfirst (and tlast) get set (looks like it is just 0 from > being in the BSS)? Also, this shit is horrendously racy with ttext > being a global, instead of being alloc'd just for this acpiread call. > This code is ALL going away once we get all of this set up. Note that Qtbl is pretty much unused right now. > + case Qpretty: > > + s = ttext; > > + e = ttext + tlen; > > + s = dumpfadt(s, e, fadt); > > + s = dumpmadt(s, e, apics); > > + s = dumpslit(s, e, slit); > > + s = dumpsrat(s, e, srat); > > + s = dumpdmar(s, e, dmar); > > + dumpmsct(s, e, mscttbl); > > + return readstr(off, a, n, ttext); > > Well, here's why Qpretty just prints everything regardless of where we > are in the hierarchy. It just does the same thing every time. > Yeah. That's the brokenness of it presently. > + default: > > + error(EINVAL, "WHAT? %d\n", q); > > I guess that's a better errstr than NULL. =) > RONNNNNN!!!! > > diff --git a/kern/include/acpi.h b/kern/include/acpi.h > > > +struct Atable *mkatable(struct Atable *parent, > > + int type, char *name, uint8_t *raw, > > + size_t rawsize, size_t addsize); > > +struct slice; > > Can probably #include <slice.h>. > Sure. > +struct Atable *finatable(struct Atable *t, struct slice *slice); > > +struct Atable *finatable_nochildren(struct Atable *t); > > Can we look at an Atable and see if it has children or not, so the > caller doesn't need to decide? > The Atable doesn't have children until it's been finalized. :/ -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
