locore.S - two cosmetic changes

[Mitchell Horne]

On Fri, Oct 30, 2020 at 12:00 PM Antonin Houska <ah at melesmeles.cz> wrote:
>
> Kristof Provost <kp at FreeBSD.org> wrote:
>
> > On 26 Oct 2020, at 7:23, Antonin Houska wrote:
> > > Hello,
> > >
> > > while trying to understand the code, I've spotted two things that might need
> > > to be improved - please see the attachments.
> > >
> > > remove_useless_assignment.patch removes setting of register value which is
> > > not
> > > used until the next value is set. use_constant_not_literal.patch probably
> > > needs no explanation.
> > >
> > That’s gone in as in r367078. Thanks!
>
> Thanks, this is my first contribution to the FreeBSD code :-)
>

Congrats :)

> Actually Mitchell Horne replied to my patch too but probably forgot to CC the
> mailing list. He suggested that some more replacements of literals by
> preprocessor constants can be included in the patch. While thinking about
> that, I came across something that I don't understand.
>
> On line 156, the position of the L1 entry for particular virtual address is
> computed:
>
> 151:    /* Create an L1 page for early devmap */
> 152:    lla     s1, pagetable_l1
> 153:    lla     s2, pagetable_l2_devmap /* Link to next level PN */
> 154:    srli    s2, s2, PAGE_SHIFT
> 155:
> 156:    li      a5, (VM_MAX_KERNEL_ADDRESS - L2_SIZE)
>
> However the corresponding L2 entry is at position 510:
>
> 183:    /* Store PTE entry to position */
> 184:    li      a6, PTE_SIZE
> 185:    li      a5, 510
>
> I'd expect 511 since the virtual addres is (1 * L2_SIZE) below
> VM_MAX_KERNEL_ADDRESS. On the other hand, the (obviously related) virtual
> address passed to initriscv() is
>
> 249:    li      t0, (VM_EARLY_DTB_ADDRESS)
>
> defined in vmparam.h as
>
> #define VM_EARLY_DTB_ADDRESS    (VM_MAX_KERNEL_ADDRESS - (2 * L2_SIZE))
>
> So I wonder if the assignment on line 156 should rather be
>
> 156:    li      a5, (VM_EARLY_DTB_ADDRESS)
>
> I suspect that the (VM_MAX_KERNEL_ADDRESS - L2_SIZE) expression is a thinko
> which does not break anything because
>
> (VM_MAX_KERNEL_ADDRESS - L2_SIZE) >> L1_SHIFT
>
> appears to be equal to
>
> VM_EARLY_DTB_ADDRESS >> L1_SHIFT
>
> Do I miss anything?
>

This particular portion of the code is confusing, and it is in part
due to the fact that VM_EARLY_DTB_ADDRESS and the start of the static
devmap (VM_MAX_KERNEL_ADDRESS - L2_SIZE) share an L1 entry, as you
have shown.

There are two things being done here:
  1. Setting up the static "devmap" region (the last 2MB of the kernel
virtual address space)
  2. Creating a temporary mapping for the device tree blob (DTB),
which is passed as a physical address from the firmware.

For 1., all we need to do in locore.S is create the link between
pagetable_l1 -> pagetable_l2_devmap. More is done later during
pmap_bootstrap(), at which point we allocate an L3 page table for the
static devmap, and link it back to pagetable_l2_devmap, at index 511.
That is why you don't see that being done here.

For 2., we need to consume the DTB very early, before
pmap_bootstrap(). Therefore, we create a "superpage" mapping here in
locore.S, which skips the third level page table. So, we are mapping
the physical address of the DTB (held in the a1 register) to the
virtual address (VM_MAX_KERNEL_ADDRESS - 2 * L2_SIZE). This virtual
address corresponds to index 510 of pagetable_l2_devmap. This mapping
is temporary, and will be destroyed in pmap_bootstrap().

We could create the temporary DTB mapping anywhere in the kernel
address space, but it was chosen to place it right in front of the
static devmap because we know the pagetable_l1 -> pagetable_l2_devmap
link was already created.

In short, the code is correct, but confusing. I have some in-progress
work that moves this logic out of locore.S, with the aim of making
this process a little easier to understand. Perhaps it's time I tried
to finish it :)

Please send me a follow-up if you have more questions, and I'll do my
best to answer.

Cheers,
Mitchell

> --
> Antonín Houska
> www.melesmeles.cz
> _______________________________________________
> freebsd-riscv at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-riscv
> To unsubscribe, send any mail to "freebsd-riscv-unsubscribe at freebsd.org"