Bug 604 - ISACaller simulator needs RADIX MMU support
Summary: ISACaller simulator needs RADIX MMU support
Status: CONFIRMED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- enhancement
Assignee: Luke Kenneth Casson Leighton
URL:
Depends on:
Blocks: 241
  Show dependency treegraph
 
Reported: 2021-02-21 13:24 GMT by Luke Kenneth Casson Leighton
Modified: 2021-05-01 22:40 BST (History)
2 users (show)

See Also:
NLnet milestone: ---
total budget (EUR) for completion of task and all subtasks: 0
budget (EUR) for this task, excluding subtasks' budget: 0
parent task for budget allocation:
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Tobias Platen 2021-02-28 11:15:24 GMT
I could do this after fixing all bugs.
Comment 2 Luke Kenneth Casson Leighton 2021-02-28 12:05:58 GMT
(In reply to Tobias Platen from comment #1)
> I could do this after fixing all bugs.

from long experience i have found that
simultaneous co-developing, particularly when there is an existing working implementation (microwatt in this case) is infinitely quicker and easier.

in other words ISACaller can be used *to* fix all bugs.
Comment 3 Luke Kenneth Casson Leighton 2021-03-02 18:14:42 GMT
design discussed here

http://lists.libre-soc.org/pipermail/libre-soc-dev/2021-March/002061.html

RADIX Mem class to be added as an option that has a Mem class as a member instance

and treats that as "Physical" memory.

exact same API.

class RADIXMem:

       self.mem = Mem()

    def ld(.....)

    def st(.....)
Comment 4 Luke Kenneth Casson Leighton 2021-03-04 17:27:57 GMT
i went over the various implementations
https://bugs.libre-soc.org/show_bug.cgi?id=604#c0

honestly i found it a bit of a nuisance :)

the most obvious-looking (short once comments are removed) was gem5.

one downside: we are NOT doing hypervisor... yet.

so we need full compatibility with *microwatt* but microwatt is the least obvious because it is a FSM that communicates via handshakes with dcache / icache.

bleh.

also microwatt uses PIDR or something which is nonstandard, temporary until hypervisor is properly implemented.

my feeling is, then, perhaps, create some ancillary functions such as a "permission checker" which uses microwatt rules.

segment check for example:

https://github.com/antonblanchard/microwatt/blob/6523acc74344b95e7cceb83611fb8cb2a030c1a3/mmu.vhdl#L338

and permission_check

https://github.com/antonblanchard/microwatt/blob/6523acc74344b95e7cceb83611fb8cb2a030c1a3/mmu.vhdl#L362

i will just cut/paste stubs in niw, 1 sec
Comment 5 Luke Kenneth Casson Leighton 2021-03-04 17:48:10 GMT
done, and also added pgtbl0/3.  needs converting.
Comment 6 Luke Kenneth Casson Leighton 2021-03-04 18:17:20 GMT
whoops i just noticed that microwatt already allocates SPR 720 to PRTBL
(a temporary non-official allocation), we were going to use that for SVSRR0
so i have moved SVSRR0 to 721.

https://git.libre-soc.org/?p=libreriscv.git;a=commitdiff;h=0863c2918838890e88ef7d9f2ace66a8476fd3f2

tobias remember to do a git submodule update.
Comment 7 Luke Kenneth Casson Leighton 2021-03-05 14:25:51 GMT
tobias i am just putting in some leaf-node functions which i am taking from microwatt, like this:

+    def _decode_prte(self, data):
+        """PRTE0 Layout
+           -----------------------------------------------
+           |/|RTS1|/|     RPDB          | RTS2 |  RPDS   |
+           -----------------------------------------------
+            0 1  2 3 4                55 56  58 59      63
+        """
+        zero = SelectableInt(0, 1)
+        rts = selectconcat(data[5:8],      # [56-58] - RTS2
+                           data[61:63],    # [1-2]   - RTS1
+ ....

the idea is to put these functions together with meaningful names
then work out how to join them up.
Comment 8 Luke Kenneth Casson Leighton 2021-03-05 16:31:23 GMT
quick test done of _decode_prte and _segment_check:

commit 60a226ae541d99e1c168984fa6ca8f4a4ed08e48 (HEAD -> master, origin/master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Fri Mar 5 16:16:41 2021 +0000

    add segment_check function, plus quick test.
    also fix order because SelectableInt deals in BE


tobias if you do the permissions check i will do a simple function which
does prtable_addr pgtable_addr and pte from microwatt mmu.vhdl
Comment 9 Luke Kenneth Casson Leighton 2021-03-09 12:42:45 GMT
tobias i moved the ISACaller RADIX class to a separate module, it's cleaner
need to remove some imports now.

i'm assuming you're keeping regularly up-to-date and have all commits pushed
immediately at the time that they're confirmed working.
Comment 10 Luke Kenneth Casson Leighton 2021-03-09 13:10:45 GMT
added the perm check function.

commit e440bd82392c798fe611abd89b3f25a331af2cca (HEAD -> master, origin/master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Tue Mar 9 13:09:35 2021 +0000

    create first check_perms RADIX ISACaller function
Comment 11 Luke Kenneth Casson Leighton 2021-03-09 18:11:22 GMT
commit d4809949497559f69c576838812b2282fe001d78 (HEAD -> master, origin/master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Tue Mar 9 18:09:53 2021 +0000

    debug radix mmu ISACaller

i managed to get the radix unit test pass, by adding __call__ as a duplicate of Mem.__call__
Comment 12 Luke Kenneth Casson Leighton 2021-03-09 19:38:03 GMT
i'm going to try this:
@@ -189,7 +189,9 @@ class RADIX:
     def ld(self, address, width=8, swap=True, check_in_mem=False):
         print("RADIX: ld from addr 0x%x width %d" % (address, width))
-        shift = SelectableInt(0, 32)
+        (shift, mbits, pgbase) = self._decode_prte(addr)
+        #shift = SelectableInt(0, 32)
+

you can see at line 235 and 315 in mmu.vhdl that pts is assigned to shift
variable
https://github.com/antonblanchard/microwatt/blob/6523acc74344b95e7cceb83611fb8cb2a030c1a3/mmu.vhdl#L235
Comment 13 Luke Kenneth Casson Leighton 2021-03-09 20:21:30 GMT
from radixwalk.cc, walkTree needs to be:

std::pair<Addr, Fault>
RadixWalk::walkTree(Addr vaddr ,
    uint64_t curBase,
    BaseTLB::Mode mode,
    uint64_t curSize ,
    uint64_t usefulBits)

mode will be:

* execute (instructions)
* load or
* store

curSize and usefulBits are shift and mask

curBase starts at the root page and works up from there.
Comment 14 Luke Kenneth Casson Leighton 2021-03-11 11:36:43 GMT
continued from here:
http://lists.libre-soc.org/pipermail/libre-soc-dev/2021-March/002121.html


turns out i did think about this:
https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/decoder/isa/mem.py;h=723fca0980d2d35d2dd12f9fc1f0d8bd325f7569;hb=5963eef6679f6833b6b8f854868d90480e3753b2#l36

that's a dictionary, not a list.

so it should be good to use the example from here:

https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/fu/mmu/test/test_issuer_mmu_rom.py;h=2552b14b03bb5ac08c6a14f86bac3694f1220500;hb=5963eef6679f6833b6b8f854868d90480e3753b2#l15

if you take a copy of that dictionary *make sure to document that fact* in both
files.  better to move it to a separate file (on its own) and import it from
both unit tests.
Comment 15 Luke Kenneth Casson Leighton 2021-03-11 18:20:48 GMT
commit 8e4b1c04b2e7fd655f9a35e796028a41a756cf1b (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Thu Mar 11 18:19:43 2021 +0000

    whoops PIDR is defined as 32-bits in SPRs.csv (and spec)


tobias i'd assumed PIDR was 64 bit, it's actually 32.

            effpid = pid #self.pid # TODO, check on this
Comment 16 Tobias Platen 2021-03-11 18:58:35 GMT
> if you take a copy of that dictionary *make sure to document that fact* in both
> files.  better to move it to a separate file (on its own) and import it from
> both unit tests.

I'm going to do that. 
But for any reason I do not receive emails from the mailing list.
Comment 17 Luke Kenneth Casson Leighton 2021-03-11 19:16:16 GMT
(In reply to Tobias Platen from comment #16)

> But for any reason I do not receive emails from the mailing list.

your server's currently rejecting messages (server configuration problem, 451)
Comment 18 Tobias Platen 2021-03-15 17:38:17 GMT
email now working correctly
Comment 19 Luke Kenneth Casson Leighton 2021-03-17 12:35:47 GMT
tobias i just added an extra argument to Mem and RADIXMMU classes,
"instr_fetch".  this is crucial to detecting "Execute" in the permission
check function at the minimum.
Comment 20 Luke Kenneth Casson Leighton 2021-03-18 18:53:11 GMT
just put in read of MSR.PR bit for "priv".  i've no idea which way
round it should be set :)

i have a thought that it may be a good idea to create and raise
actual exceptions (in python) then catch them back in ISACaller
and call ISACaller.trap() with the right arguments (right exception
address/type).
Comment 22 Luke Kenneth Casson Leighton 2021-03-24 10:20:10 GMT
(In reply to Luke Kenneth Casson Leighton from comment #21)

> that should be "nonzero != 0"

done.
Comment 23 Luke Kenneth Casson Leighton 2021-03-29 22:40:15 BST
to see what segment_check is for, it works out the new shift value, also it sets badtree.

it should be possible to check the exception that is called in execute1.vhdl when that happens.  grep "badtree" *.vhdl
Comment 24 Luke Kenneth Casson Leighton 2021-03-30 16:23:01 BST
Tobias i believe i may have RADIXMMU returning correct addresses,
after going through this:
https://github.com/power-gem5/gem5/blob/gem5-experimental/src/arch/power/radix_walk_example.txt#L65


https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=569ec82cc22073e92871f1f458d65dc74059108b
Comment 25 Tobias Platen 2021-03-30 18:35:40 BST
this is defitively useful information
Comment 26 Luke Kenneth Casson Leighton 2021-03-30 20:20:19 BST
Tobias i do not believe there is a need for self._prtable_lookup().

the code already exists here, at the beginning of walk_tree:

        # get address of root entry
        shift = selectconcat(SelectableInt(0,1), prtbl[58:63]) # TODO verify
        addr_next = self._get_prtable_addr(shift, prtbl, addr, pidr)

the rest of it is not relevant except potentially the pieces which it looks
like the Microwatt team have used for some "communication" such as by setting
RPDS=0 to indicate "disable radix".

                    elsif mbits = 0 then
                        -- Use RPDS = 0 to disable radix tree walks

i do not believe it is necessary to code up a complex _prtable_lookup function.
Comment 27 Tobias Platen 2021-03-30 20:38:19 BST
I wrote this code, so that I could get an understanding how the address of the first page table gets calculated. It just look longer for me to understand. If it is redunant, it should be removed. I was unaware of that when I began.
Comment 28 Luke Kenneth Casson Leighton 2021-04-06 10:17:31 BST
tobias i wrote this to openpower-hdl-cores
http://lists.mailinglist.openpowerfoundation.org/pipermail/openpower-hdl-cores/2021-April/000257.html

however we should not stop and wait for a response.  do keep investigating.
perhaps working out a state diagram of microwatt MMU would help?
Comment 29 Tobias Platen 2021-04-06 11:21:30 BST
I fully agree.
Comment 30 Luke Kenneth Casson Leighton 2021-04-15 09:13:54 BST
tobias the simulator and TestIssuer memory access is split between instruction and data.  these paths DO NOT MEET at a common point (common memory) they are COMPLETELY SEPARATE.

all unit tests involving memory will read instructions starting from 0x0000.

all such tests if writing to 0x0000 *DO NOT* overwrite the instructions because the PATHS ARE COMPLETELY SEPARATE.

this has greatly simplified the test infrastructure.

to test mmu-enabled data load/store it will be necessary to DISABLE virtual memory on instruction load, in both ISACaller and TestIssuer.
Comment 31 Luke Kenneth Casson Leighton 2021-04-15 09:49:19 BST
done.  lwz no longer attempted to be read from an illegal memory location.


commit a0cd3943c6768c4676d39e821f58269d15e76596 (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Thu Apr 15 09:48:37 2021 +0100

    add icachemmu option to ISACaller
Comment 32 Tobias Platen 2021-04-15 16:08:27 BST
The latest changes that I see are from 19 hours ago.
Comment 33 Luke Kenneth Casson Leighton 2021-04-15 23:58:24 BST
 440         # mbits := unsigned('0' & data(4 downto 0));
 441         mbits = selectconcat(SelectableInt(0,1), data[58:63])
 442         assert(mbits.bits==6) #variable mbits : unsigned(5 downto 0);

this is incorrect.  the conversion is as follows:

* x downto y converts to 63-x downto 63-y
* 4 downto 0 is therefore 59 downto 63

however python lists are to the end plus 1

therefore 4 downto 0 is data[59:64]

NOT

58:63

selectconcat of one extra zero makes this 6 bits long.

confirmed by examining line 366

366         // [59:63] = NLS = Next Level Size                |
 367         //            |    NLS >= 5
Comment 34 Luke Kenneth Casson Leighton 2021-04-16 00:51:45 BST
tobias: never use "type" in python.  always use "isinstance"

        if isinstance(addr, str):
            return addr
        if isinstance(shift, str):
            return shift
Comment 35 Luke Kenneth Casson Leighton 2021-04-16 11:57:05 BST
i did a lot of code-cleanup yesterday, including fixing a number of code-fragments that had been altered from their original extraction from microwatt.

tobias it is critical to keep EXACTLY to microwatt until such time as we have deeply comprehensive unit tests in place.

only then is it safe to perform code-morphs such as removing mbits from segment_check.

i also found several pieces of code that were duplicates of functions that had already been written, but inserted inline.

the functions were correct but the inline duplicates had completely wrong offsets as part of the conversion from MSB0 to LSB0-python.

despite that it is actually functional and the next step is to pile on the unit tests .

i will track down the microwatt mmu tests, they will be necessary.
Comment 36 Luke Kenneth Casson Leighton 2021-04-16 12:04:21 BST
https://github.com/antonblanchard/microwatt/tree/master/tests/mmu

nope.

this looks much too sophisticated for ISACaller to attempt to run, it looks like it would be appropriate to run under litex sim.py once things are much further along.

we need to progress further with smaller unit tests, first.
Comment 37 Tobias Platen 2021-04-16 15:18:27 BST
The test is definively too complex. Simple tests are needed first, after this more complex tests can run on real ECP5 Hardware.
Comment 38 Luke Kenneth Casson Leighton 2021-04-16 18:50:38 BST
Tobias Platen <libre-soc@platen-software.de>
	
6:36 PM (12 minutes ago)
	
to libre-soc-dev
On Fri, 2021-04-16 at 18:21 +0200, Tobias Platen wrote:
> today: first carefully reading the cleaned up radixmmu.py, then more
> testing

Found a bug in:
def _new_lookup(self, data, shift):
        """
        mbits := unsigned('0' & data(4 downto 0));
        if mbits < 5 or mbits > 16 or mbits > r.shift then
            v.state := RADIX_FINISH;
            v.badtree := '1'; -- throw error
        else
            v.shift := v.shift - mbits;
            v.mask_size := mbits(4 downto 0);
            v.pgbase := data(55 downto 8) & x"00"; NLB?
            v.state := RADIX_LOOKUP; --> next level
        end if;
        """
This needs to be
def _new_lookup(self, data, r_shift, v_shift):
assuming that r_shift and v_shift are not identical.
I'll investigate that by reading the microwatt source code.
Comment 39 Luke Kenneth Casson Leighton 2021-04-16 19:13:54 BST
(In reply to Luke Kenneth Casson Leighton from comment #38)
> Tobias Platen <libre-soc@platen-software.de>

> This needs to be
> def _new_lookup(self, data, r_shift, v_shift):
> assuming that r_shift and v_shift are not identical.
> I'll investigate that by reading the microwatt source code.

the way that VHDL works, variables get used/created/substituted.  walking
it through:

    begin
        v := r;

that makes a copy of r into v.  r is the "old" shift amount.


            -- set v.shift to rts so that we can use finalmask for the segment check
            v.shift := rts;

this is now the new shift amount, set to rts, however the FSM then goes
round another loop and that value goes into r.shift.

yes, agreed: both segment_check and _new_lookup need the "old" value of shift
as an extra argument.
Comment 40 Tobias Platen 2021-04-16 19:53:42 BST
I was reading the microwatt source code indepenently and came to the same conclusion.
Comment 41 Luke Kenneth Casson Leighton 2021-04-16 23:30:33 BST
(In reply to Tobias Platen from comment #40)
> I was reading the microwatt source code indepenently and came to the same
> conclusion.

segment_check needs the same fix, i believe
Comment 42 Luke Kenneth Casson Leighton 2021-04-17 14:59:16 BST
https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/decoder/isa/radixmmu.py;h=d7035031e6d9fb86f0fb06a0a52708018e84d29a;hb=cc3b648ccb01f03711e16a80959efb8ee09f171e#l700

tobias you altered the size of pgbase from microwatt's 56 bits, extending it to 64 bits.

this is 100% guaranteed to cause damage (data corruption)

MSB0 is a bloody nuisance: note in the line above how the bits are selected? this works *****ONLY***** if the length of pgbase is exactly 56.

it is critically important not to alter the size of ANY variable, here.
Comment 43 Tobias Platen 2021-04-17 16:14:29 BST
ack, I'm going to fix my mistake now. At the time I wrote that code, I tried to understand what that microwatt code does. I came to a wrong conclusion.
Comment 44 Luke Kenneth Casson Leighton 2021-04-17 17:37:11 BST
(In reply to Tobias Platen from comment #43)
> ack, I'm going to fix my mistake now. At the time I wrote that code, I tried
> to understand what that microwatt code does. I came to a wrong conclusion.

it should now be limited to _get_pgbase.
Comment 45 Tobias Platen 2021-04-17 17:50:54 BST
I had overseen the x in x"00" & r.pgbase(55 downto 19), so I tried to solve a problem that does not exist.
Comment 46 Tobias Platen 2021-04-17 18:23:12 BST
fixed
Comment 47 Luke Kenneth Casson Leighton 2021-04-17 19:28:59 BST
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=f6eabfc48b4060248bbd0ffcefac4bcba2078f2b

i don't recognise those offsets (from memory), 0:37.

did you alter them originally when introducing the mistake that extended pgbase to 64 bits?

please check against the git revision history and make sure that the original offsets are exactly as i translated them from microwatt.  they do not look right.
Comment 48 Luke Kenneth Casson Leighton 2021-04-17 19:34:41 BST
double-check: 

       pgtable_addr := x"00" & r.pgbase(55 downto 19) &
                        ((r.pgbase(18 downto 3) and not mask) or (addrsh and mask)) &
                        "000";

*  55 downto 19 in LSB0 order
*  55-55 to 55-19 in MSB0 order
*  0 to 36 in MSB0
*  0 to 37 in python "end+1"

*  18 downto 3 in LSB0 order
*  55-18 downto 55-3 in MSB0 
*  37 to 52 in MSB0
*  37 to 53 in python "end+1"
Comment 49 Luke Kenneth Casson Leighton 2021-04-17 20:30:50 BST
i cleaned up the unit test and added a ld/st example as well as trying
to do a new lookup with an alternative PID.  i have no idea what i'm doing,
it is just experimentation.

an actual 2nd example would help, except this one's 2nd example is
actually a hypervisor kernel, not userspace
https://github.com/power-gem5/gem5/blob/gem5-experimental/src/arch/power/radix_walk_example.txt
Comment 50 Tobias Platen 2021-04-18 08:05:57 BST
I went back through the git revision history.

Those numbers had been wrong from the beginning, but at that time I did not realize that those were wrong. It would be correct if pgbase were 64 bits.

commit 11753fd5bfa447d53ed3b8c81243d9063a20da4b
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Tue Mar 9 12:38:08 2021 +0000

    move ISACaller RADIX MMU class to separate module

commit 91b3f9682737d5c9adf8a6bfcd2be15ad8bf4465
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Tue Mar 9 12:30:02 2021 +0000

    add pgtable and pte calculation to RADIX ISACaller

def _get_pgtable_addr(self, mask_size, pgbase, addrsh):
        """
        x"00" & r.pgbase(55 downto 19) &
        ((r.pgbase(18 downto 3) and not mask) or (addrsh and mask)) &
        "000";
        """
        mask16 = genmask(mask_size+5, 16)
        zero8 = SelectableInt(0, 8)
        zero3 = SelectableInt(0, 3)
        res = selectconcat(zero8,
                           pgbase[8:45],              #
                           (prtbl[45:61] & ~mask16) | #
                           (addrsh       & mask16),   #
                           zero3
                           )
        return res
Comment 51 Luke Kenneth Casson Leighton 2021-04-18 10:18:50 BST
(In reply to Tobias Platen from comment #50)
> I went back through the git revision history.
> 
> Those numbers had been wrong from the beginning, but at that time I did not
> realize that those were wrong.

me neither

> It would be correct if pgbase were 64 bits.

yeyyeh

> def _get_pgtable_addr(self, mask_size, pgbase, addrsh):
>         """
>         x"00" & r.pgbase(55 downto 19) &
>         ((r.pgbase(18 downto 3) and not mask) or (addrsh and mask)) &
>         "000";
>         """
>                            pgbase[8:45],              #
>                            (prtbl[45:61] & ~mask16) | #
                              ^^^^^
                         this should be pgbase

>                            (addrsh       & mask16),   #
>                            zero3
>                            )
>         return res
Comment 52 Luke Kenneth Casson Leighton 2021-04-18 10:20:00 BST
yep it is.  looks good

        res = selectconcat(zero8,
                           pgbase[0:37],
                           (pgbase[37:53] & ~mask16) |
                           (addrsh       & mask16),
                           zero3
                           )