Hi - Thanks for the ACPI work. It's a nasty beast. 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 > 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. 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 Does anything have a table? Is that going to be the raw pointer to Atable->tbl, or its dump or something? / $ ls \#acpi APIC FACP HPET SSDT pretty raw table Is the APIC the MADT? > 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. > 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. > 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. > +}; > + > +#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). > +/* > + * 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. > 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. > +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. > 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. > + 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). > -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. 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). > + 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. > + */ > + > + 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? > 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. > + 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. > + default: > + error(EINVAL, "WHAT? %d\n", q); I guess that's a better errstr than NULL. =) > 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>. > +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? Thanks, Barret -- 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.
