Hi,
Thanks for your MMU code review !
Answers inline.
Le 03/05/12 23:23, Sebastien Bourdeauducq a écrit :
> Hi,
>
> I had a look at the current MMU design. Looks like a good start so far. Some
> problems by decreasing importance:
>
> +wire [9:0] dtlb_write_tag;
> +wire [9:0] dtlb_read_tag;
>
> ===> "Tag" should be renamed "valid" (or something similar), made 1-bit wide
> and merged into the "data" memory.
After some chat on #milkymist it has been agreed that we need a 31 bits
wide TLB data containing { valid_bit, 10_top_virtual_address_bits,
20_top_physical_address_bits }
Indeed, let assume TLB has 1024 (2^10) entries.
Therefore TLB is only indexed by the 10 lower bits of the virtual page
frame number ( virtual_address[21:12] ) which has the benefit to only
use memory for 1024 entries instead of 2^20 ( > 1 million ) entries but
has the drawback of having 2^(20-10) = 2^10 = 1024 aliases for each TLB
entry.
Meaning that 0bxxxxxxxxxx yyyyyyyyyy and 0bzzzzzzzzzz yyyyyyyyyy (with x
!= z) virtual page frame numbers will index the same TLB entry => collision.
Thus, we need a mechanism to check whether the TLB entry indexed by the
yyyyyyyyyy of 0bxxxxxxxxxx yyyyyyyyyy is really valid for this virtual
page frame number or if it is intended to be an entry to map
0bxxxxxxxxxx zzzzzzzzzz
The solution is to store the 10 top virtual address bits in the TLB
entry lookup data in order to compare those bits with the virtual
address being looked up.
In fine, the dtlb_tag BlockRAM will be removed and the tlb_data BlockRAM
will be 31 bits wide containing the data described above.
> +// CSR Write
> +always @(posedge clk_i)
> +begin
> + if (csr_write_enable)
> + begin
> + case (csr)
>
> ===> This should have a reset.
Sure !
>
> +assign dtlb_write_tag = (dtlb_flushing == `TRUE)
> + ? `LM32_DTLB_INVALID_TAG
> + : {dtlb_update_vaddr_csr_reg[30:22], `TRUE}; // 10-1
> top VA
> bits
>
> ===> Do not use hardcoded indices (30:22) but macros/params instead to be
> consistent with the rest of the code.
OK, tag will be removed anyway.
>
> else
> begin
> // Set flag when bus error is detected
> - if ((D_ERR_I == `TRUE) && (D_CYC_O == `TRUE))
> + if (((D_ERR_I == `TRUE) && (D_CYC_O == `TRUE)))
> data_bus_error_seen <= `TRUE;
> // Clear flag when exception is taken
> if ((exception_m == `TRUE) && (kill_m == `FALSE))
>
> ===> Extra parentheses do nothing
Yes, my bad.
>
> refilling,
> - load_data
> + load_data,
> + // To pipeline
> + dtlb_miss,
> + kernel_mode,
> + pa,
> + csr_read_data
> );
>
> ===> fix indentation of // To pipeline
OK !
Thanks for the feedback :)
--
Yann Sionneau
_______________________________________________
http://lists.milkymist.org/listinfo.cgi/devel-milkymist.org
IRC: #milkymist@Freenode
