]> git.lizzy.rs Git - plan9front.git/commit
libaml: fix IndexField and BankField implementations (thanks Michael Forney)
authorcinap_lenrek <cinap_lenrek@felloff.net>
Mon, 22 Feb 2021 18:27:49 +0000 (19:27 +0100)
committercinap_lenrek <cinap_lenrek@felloff.net>
Mon, 22 Feb 2021 18:27:49 +0000 (19:27 +0100)
commite77fa31516bc5dc834f1caf1f4ed3d888a9aa810
treef3a67d0979f0077ebb952cc723e71895f4016df4
parent472958e3e7313cbe0cdb8d482045c2be87a23659
libaml: fix IndexField and BankField implementations (thanks Michael Forney)

IndexField is supposed to increment the index value when an
access is done with a bigger size than the data field.
The index value is always a byte offset.

Now that we always calculate the offset for each field unit
access for IndexField, rename the indexv to bank (the bank
value), as it is only used for that. Also, do not compare
it with nil, as it is a integer constant which can be
encoded as nil to mean zero.

For BankField, the banking field was written using store(),
which does nothing when the destination is a Field*.
Use rwfield() to fix it in the new rwfieldunit().

Resolve all the Name*'s when IndexField, BankField and
Field are created. Now, Field.reg points to eigther
Buffer object, Region or Field (data Field of an IndexField).

PS: initial bug report by Michael Forney follows below:

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.
sys/src/libaml/aml.c