9front - general discussion about 9front
 help / color / mirror / Atom feed
* [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

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