I could do this after fixing all bugs.

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

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

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

done, and also added pgtbl0/3.  needs converting.

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.

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.

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

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.

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

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__

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

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.

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.

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

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

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

email now working correctly

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.

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

https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/decoder/isa/radixmmu.py;h=10b95ff084e51e04229bac6d1c7d21c24394427b;hb=23d5a669f0e7193359a262e3e34a44b10e007e23#l489

that should be "nonzero != 0"

(In reply to Luke Kenneth Casson Leighton from comment #21)

> that should be "nonzero != 0"

done.

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

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

this is defitively useful information

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.

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.

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?

I fully agree.

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.

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

The latest changes that I see are from 19 hours ago.

 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

tobias: never use "type" in python.  always use "isinstance"

        if isinstance(addr, str):
            return addr
        if isinstance(shift, str):
            return shift

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.

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.

The test is definively too complex. Simple tests are needed first, after this more complex tests can run on real ECP5 Hardware.

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.

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

I was reading the microwatt source code indepenently and came to the same conclusion.

(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

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.

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.

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

I had overseen the x in x"00" & r.pgbase(55 downto 19), so I tried to solve a problem that does not exist.

fixed

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.

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"

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

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

(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

yep it is.  looks good

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