* [9front] libaml IndexField
@ 2021-02-21 2:12 Michael Forney
2021-02-21 18:14 ` cinap_lenrek
0 siblings, 1 reply; 3+ messages in thread
From: Michael Forney @ 2021-02-21 2:12 UTC (permalink / raw)
To: 9front
[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]
In /dev/kmesg on my T14, I saw a message
amlmapio: [0xffffff18-0x100000018] overlaps usable memory
amlmapio: mapping \_SB.FRTP failed
Here is the relevant snippet from my DSDT:
Scope (_SB)
{
...
OperationRegion (ECMC, SystemIO, 0x72, 0x02)
Field (ECMC, AnyAcc, NoLock, Preserve)
{
ECMI, 8,
ECMD, 8
}
IndexField (ECMI, ECMD, ByteAcc, NoLock, Preserve)
{
Offset (0x08),
FRTB, 32
}
OperationRegion (FRTP, SystemMemory, FRTB, 0x0100)
Field (FRTP, AnyAcc, NoLock, Preserve)
{
...
}
}
With some debugging output:
amlmapio(\_SB.ECMC): Io 72 - 74
rwreg(\_SB.ECMC): Io [72+0]/1 <- 8
rwreg(\_SB.ECMC): Io [72+1]/1 -> 18
amlmapio(\_SB.FRTP): Mem ffffff18 - 100000018
amlmapio: [0xffffff18-0x100000018) overlaps usable memory
amlmapio: mapping \_SB.FRTP failed
It seems that libaml does not handle IndexField correctly and just did
a single read from ECMD after setting ECMI to 8, causing the FRTP
region to be evaluated as 0xffffff18-0x100000018. Instead, it should
be reading 4 bytes [18 c0 22 cc], evaluating it as
0xcc22c018-0xcc22118:
amlmapio(\_SB.ECMC): Io 72 - 74
rwreg(\_SB.ECMC): Io [72+0]/1 <- 8
rwreg(\_SB.ECMC): Io [72+1]/1 -> 18
rwreg(\_SB.ECMC): Io [72+0]/1 <- 9
rwreg(\_SB.ECMC): Io [72+1]/1 -> c0
rwreg(\_SB.ECMC): Io [72+0]/1 <- a
rwreg(\_SB.ECMC): Io [72+1]/1 -> 22
rwreg(\_SB.ECMC): Io [72+0]/1 <- b
rwreg(\_SB.ECMC): Io [72+1]/1 -> cc
amlmapio(\_SB.FRTP): Mem cc22c018 - cc22c118
I wrote a patch (attached) to fix this, and it seems to work. Though,
it's not clear to me when things should be dereferenced. Previously,
the data field was dereferenced at evalfield, but the region and index
field were not until rwfield. After the patch, the index field is
also dereferenced in evalfield.
For BankField, the index *is* dereferenced in evalfield. I'm pretty
sure that this means that BankField does not work currently, since
store() just returns nil for 'f' objects. The bank selector will
never get set.
Anyway, I don't know if this solves any real problems; it's just
something I noticed and thought I'd try to fix.
[-- Attachment #2: 0001-libaml-fix-IndexField-access-when-multiple-reads-writes-are-needed.patch --]
[-- Type: text/plain, Size: 3966 bytes --]
From 1e2c2c5c5f27c027f76cbce3a151d30e48f22006
From: Michael Forney <mforney@mforney.org>
Date: Sun, 21 Feb 2021 00:24:41 +0000
Subject: libaml: fix IndexField access when multiple reads/writes are needed
Previously, accessing an IndexField would set the index field to a
fixed value, and then read from the region associated with the data
field (incorporating its offset). However if the read/write length is
larger than the data field, it would just read past the end. Instead,
the index needs to be updated for each access.
To fix this, introduce a function rwfieldunit, which does one access
of the region/index-data/bank, and use this in rwfield instead of
rwreg directly.
diff e7f36397ace2aaaf16a643080dcc439c5b061613 1e2c2c5c5f27c027f76cbce3a151d30e48f22006
--- a/sys/src/libaml/aml.c Thu Feb 18 00:58:35 2021
+++ b/sys/src/libaml/aml.c Sat Feb 20 16:24:41 2021
@@ -72,6 +72,7 @@
struct Field {
void *reg; /* Buffer or Region */
Field *index;
+ Field *data;
void *indexv;
int flags;
int bitoff;
@@ -219,6 +220,7 @@
gcmark(f->reg);
gcmark(f->index);
gcmark(f->indexv);
+ gcmark(f->data);
break;
}
}
@@ -608,17 +610,41 @@
}
}
+static void *rwfield(Field *f, void *v, int write);
+
+static uvlong
+rwfieldunit(Field *f, int off, int len, uvlong v, int write)
+{
+ void *b, *reg;
+
+ if(f->reg){
+ if((reg = deref(f->reg)) == nil)
+ return 0;
+ v = rwreg(reg, off, len, v, write);
+ }else if(f->index && f->data){
+ rwfield(f->index, mki(off), 1);
+ if(write){
+ b = mk('b', len);
+ putle(b, len, v);
+ rwfield(f->data, b, 1);
+ }else{
+ b = rwfield(f->data, nil, 0);
+ v = getle(b, len);
+ }
+ }
+ return v;
+}
+
static void*
rwfield(Field *f, void *v, int write)
{
int boff, blen, wo, ws, wl, wa, wd, i;
uvlong w, m;
- void *reg;
uchar *b;
- if(f == nil || (reg = deref(f->reg)) == nil)
+ if(f == nil)
return nil;
- if(f->index)
+ if(f->indexv)
store(f->indexv, f->index);
blen = f->bitlen;
if(write){
@@ -649,11 +675,11 @@
w <<= ws;
if(wl != wd){
m = ((1ULL<<wl)-1) << ws;
- w |= rwreg(reg, wo*wa, wa, 0, 0) & ~m;
+ w |= rwfieldunit(f, wo*wa, wa, 0, 0) & ~m;
}
- rwreg(reg, wo*wa, wa, w, 1);
+ rwfieldunit(f, wo*wa, wa, w, 1);
} else {
- w = rwreg(reg, wo*wa, wa, 0, 0) >> ws;
+ w = rwfieldunit(f, wo*wa, wa, 0, 0) >> ws;
for(i = 0; i < wl; i++, boff++){
b[boff/8] |= (w&1)<<(boff%8);
w >>= 1;
@@ -937,9 +963,12 @@
/* no break */
case 'f':
l = p;
- if(l->index)
+ if(l->data)
return fmtprint(f, "IndexField(%x, %x, %x) @ %V[%V]",
- l->flags, l->bitoff, l->bitlen, l->index, l->indexv);
+ l->flags, l->bitoff, l->bitlen, l->data, l->index);
+ else if(l->indexv)
+ return fmtprint(f, "BankField(%x, %x, %x, %V=%V) @ %V",
+ l->flags, l->bitoff, l->bitlen, l->index, l->indexv, l->reg);
return fmtprint(f, "Field(%x, %x, %x) @ %V",
l->flags, l->bitoff, l->bitlen, l->reg);
default:
@@ -1374,11 +1403,12 @@
static void*
evalfield(void)
{
- int flags, bitoff, wa, n;
- Field *f, *df;
+ int flags, bitoff, n;
+ Field *f, *xf, *df;
Name *d;
uchar *p;
+ xf = nil;
df = nil;
flags = 0;
bitoff = 0;
@@ -1387,6 +1417,9 @@
flags = ival(FP->arg[1]);
break;
case Oxfld:
+ xf = deref(FP->arg[0]);
+ if(xf == nil || TAG(xf) != 'f')
+ goto Out;
df = deref(FP->arg[1]);
if(df == nil || TAG(df) != 'f')
goto Out;
@@ -1425,21 +1458,17 @@
f = mk('f', sizeof(Field));
f->flags = flags;
f->bitlen = n;
+ f->bitoff = bitoff;
switch(FP->op - optab){
case Ofld:
f->reg = FP->arg[0];
- f->bitoff = bitoff;
break;
case Oxfld:
- wa = fieldalign(df->flags);
- f->reg = df->reg;
- f->bitoff = df->bitoff + (bitoff % (wa*8));
- f->indexv = mki((bitoff/(wa*8))*wa);
- f->index = FP->arg[0];
+ f->data = df;
+ f->index = xf;
break;
case Obfld:
f->reg = FP->arg[0];
- f->bitoff = bitoff;
f->indexv = FP->arg[2];
f->index = df;
break;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [9front] libaml IndexField
2021-02-21 2:12 [9front] libaml IndexField Michael Forney
@ 2021-02-21 18:14 ` cinap_lenrek
2021-02-22 7:21 ` Michael Forney
0 siblings, 1 reply; 3+ messages in thread
From: cinap_lenrek @ 2021-02-21 18:14 UTC (permalink / raw)
To: 9front
Wow, good catch!
Yeah, the IndexField was completely wrong. And you'r also right
that banking is broken due to the store needing a Name* target,
not a Field*.
Theres also another issue that we compare the Field.indexv with
nil, which is wrong, as nil pointer also means a integer of 0.
I'd also like to avoid adding a data pointer to the Field struct,
and instead, reuse the reg pointer to point to the data Field.
We can ditinguish IndexField from BankField by the reg tag.
Also renamed indexv to bank as this is what it is used for now
after clearing up the confusion.
I also got rid of the indirection on Field.reg, so now it will
never point to a Name* object and there is no need for the
deref() at rwfield() time. I think i only added that to get
prettier names during debugging.
--- a/sys/src/libaml/aml.c Sat Feb 20 21:02:07 2021 -0800
+++ b/sys/src/libaml/aml.c Sun Feb 21 19:02:10 2021 +0100
@@ -70,9 +70,9 @@
};
struct Field {
- void *reg; /* Buffer or Region */
- Field *index;
- void *indexv;
+ void *reg; /* Buffer or Region or data Field */
+ void *bank; /* bank value */
+ Field *index; /* bank or index Field */
int flags;
int bitoff;
int bitlen;
@@ -217,8 +217,8 @@
case 'u':
f = p;
gcmark(f->reg);
+ gcmark(f->bank);
gcmark(f->index);
- gcmark(f->indexv);
break;
}
}
@@ -608,18 +608,46 @@
}
}
+static void *rwfield(Field *f, void *v, int write);
+
+static uvlong
+rwfieldunit(Field *f, int off, int len, uvlong v, int write)
+{
+ if(f->index){
+ if(TAG(f->reg) == 'f'){
+ void *b;
+
+ /* set index field */
+ rwfield(f->index, mki(off), 1);
+
+ /* set data field */
+ f = f->reg;
+ if(write){
+ b = mk('b', len);
+ putle(b, len, v);
+ rwfield(f, b, 1);
+ }else{
+ b = rwfield(f, nil, 0);
+ v = getle(b, len);
+ }
+ return v;
+ }
+
+ /* set bank field */
+ rwfield(f->index, f->bank, 1);
+ }
+ return rwreg(f->reg, off, len, v, write);
+}
+
static void*
rwfield(Field *f, void *v, int write)
{
int boff, blen, wo, ws, wl, wa, wd, i;
uvlong w, m;
- void *reg;
uchar *b;
- if(f == nil || (reg = deref(f->reg)) == nil)
+ if(f == nil)
return nil;
- if(f->index)
- store(f->indexv, f->index);
blen = f->bitlen;
if(write){
if(v && TAG(v) == 'b'){
@@ -649,11 +677,11 @@
w <<= ws;
if(wl != wd){
m = ((1ULL<<wl)-1) << ws;
- w |= rwreg(reg, wo*wa, wa, 0, 0) & ~m;
+ w |= rwfieldunit(f, wo*wa, wa, 0, 0) & ~m;
}
- rwreg(reg, wo*wa, wa, w, 1);
+ rwfieldunit(f, wo*wa, wa, w, 1);
} else {
- w = rwreg(reg, wo*wa, wa, 0, 0) >> ws;
+ w = rwfieldunit(f, wo*wa, wa, 0, 0) >> ws;
for(i = 0; i < wl; i++, boff++){
b[boff/8] |= (w&1)<<(boff%8);
w >>= 1;
@@ -937,9 +965,14 @@
/* no break */
case 'f':
l = p;
- if(l->index)
- return fmtprint(f, "IndexField(%x, %x, %x) @ %V[%V]",
- l->flags, l->bitoff, l->bitlen, l->index, l->indexv);
+ if(l->index){
+ if(TAG(l->reg) == 'f')
+ return fmtprint(f, "IndexField(%x, %x, %x) @ %V[%V]",
+ l->flags, l->bitoff, l->bitlen, l->reg, l->index);
+ else
+ return fmtprint(f, "BankField(%x, %x, %x, %V=%V) @ %V",
+ l->flags, l->bitoff, l->bitlen, l->index, l->bank, l->reg);
+ }
return fmtprint(f, "Field(%x, %x, %x) @ %V",
l->flags, l->bitoff, l->bitlen, l->reg);
default:
@@ -1374,28 +1407,36 @@
static void*
evalfield(void)
{
- int flags, bitoff, wa, n;
- Field *f, *df;
+ int flags, bitoff, n;
+ Field *f, *index;
+ void *reg, *bank;
Name *d;
uchar *p;
- df = nil;
- flags = 0;
+ bank = nil;
+ index = nil;
bitoff = 0;
switch(FP->op - optab){
+ default:
+ goto Out;
case Ofld:
+ if((reg = deref(FP->arg[0])) == nil || TAG(reg) != 'r')
+ goto Out;
flags = ival(FP->arg[1]);
break;
case Oxfld:
- df = deref(FP->arg[1]);
- if(df == nil || TAG(df) != 'f')
+ if((index = deref(FP->arg[0])) == nil || TAG(index) != 'f')
+ goto Out;
+ if((reg = deref(FP->arg[1])) == nil || TAG(reg) != 'f') /* data field */
goto Out;
flags = ival(FP->arg[2]);
break;
case Obfld:
- df = deref(FP->arg[1]);
- if(df == nil || TAG(df) != 'f')
+ if((reg = deref(FP->arg[0])) == nil || TAG(reg) != 'r')
goto Out;
+ if((index = deref(FP->arg[1])) == nil || TAG(index) != 'f')
+ goto Out;
+ bank = FP->arg[2];
flags = ival(FP->arg[3]);
break;
}
@@ -1425,25 +1466,10 @@
f = mk('f', sizeof(Field));
f->flags = flags;
f->bitlen = n;
- switch(FP->op - optab){
- case Ofld:
- f->reg = FP->arg[0];
- f->bitoff = bitoff;
- break;
- case Oxfld:
- wa = fieldalign(df->flags);
- f->reg = df->reg;
- f->bitoff = df->bitoff + (bitoff % (wa*8));
- f->indexv = mki((bitoff/(wa*8))*wa);
- f->index = FP->arg[0];
- break;
- case Obfld:
- f->reg = FP->arg[0];
- f->bitoff = bitoff;
- f->indexv = FP->arg[2];
- f->index = df;
- break;
- }
+ f->bitoff = bitoff;
+ f->reg = reg;
+ f->bank = bank;
+ f->index = index;
bitoff += n;
d->v = f;
}
--
cinap
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [9front] libaml IndexField
2021-02-21 18:14 ` cinap_lenrek
@ 2021-02-22 7:21 ` Michael Forney
0 siblings, 0 replies; 3+ messages in thread
From: Michael Forney @ 2021-02-22 7:21 UTC (permalink / raw)
To: 9front
Thanks for taking the extra step with my patch, your new version looks
good to me. I tested it and it seems to be working correctly.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-22 7:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-21 2:12 [9front] libaml IndexField Michael Forney
2021-02-21 18:14 ` cinap_lenrek
2021-02-22 7:21 ` Michael Forney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).