Re: [PATCH v4 2/2] xen/riscv: introduce identity mapping

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-07-26 at 17:00 +0200, Jan Beulich wrote:
> On 26.07.2023 15:12, Oleksii wrote:
> > On Wed, 2023-07-26 at 13:58 +0200, Jan Beulich wrote:
> > > On 26.07.2023 13:23, Oleksii wrote:
> > > > I would like to ask for advice on whether it would be easier,
> > > > less
> > > > bug-
> > > > provoking ( during identity mapping to remove of whole Xen ) to
> > > > have a
> > > > separate identity section that won't be more than PAGE_SIZE.
> > > 
> > > I'm afraid you can't safely do this in C, or at least not without
> > > further checking on what the compiler actually did.
> > > 
> > > > @@ -264,6 +268,19 @@ void __init enable_mmu(void)
> > > >                RV_STAGE1_MODE << SATP_MODE_SHIFT);
> > > >  }
> > > >  
> > > > +void __attribute__((naked)) __section(".ident")
> > > > turn_on_mmu(unsigned
> > > > long ra)
> > > 
> > > Did you read what gcc doc says about "naked"? Extended asm()
> > > isn't
> > > supported there. Since ...
> > > 
> > > > +{
> > > > +    /* Ensure page table writes precede loading the SATP */
> > > > +    sfence_vma();
> > > > +
> > > > +    /* Enable the MMU and load the new pagetable for Xen */
> > > > +    csr_write(CSR_SATP,
> > > > +              PFN_DOWN((unsigned long)stage1_pgtbl_root) |
> > > > +              RV_STAGE1_MODE << SATP_MODE_SHIFT);
> > > > +
> > > > +    asm volatile( "jr %0\n" : : "r"(ra) );
> > > > +}
> > > 
> > > ... none of this really requires C, I think we're at the point
> > > where
> > > (iirc) Andrew's and my suggestion wants following, moving this to
> > > assembly code (at which point it doesn't need to be a separate
> > > function). You can still build page tables in C, of course.
> > > (Likely
> > > you then also won't need a separate section; some minimal
> > > alignment
> > > guarantees ought to suffice to make sure the critical code is
> > > confined to a single page.)
> > 
> > Thanks. I'll move all of this to assembly code.
> > Regarding alignment it is needed alignment on start and end of
> > function:
> >     .balign PAGE_SIZE
> >     GLOBAL(turn_on_mmu)
> >         ...
> >     .balign PAGE_SIZE
> >     ENDPROC(turn_on_mmu)
> > 
> > Does the better way exist?
> 
> The function is only going to be a handful of instructions. Its
> alignment doesn't need to be larger than the next power of 2. I
> expect you'll be good with 64-byte alignment. (In no case do you
> need to align the end of the function: Putting other stuff there
> is not a problem at all.) What you want in any event is a build
> time check that the within-a-page constraint is met.
But shouldn't be an address be aligned to a boundary equal to page
size?

According to the RISC-V privileged spec:
Any level of PTE may be a leaf PTE, so in addition to 4 KiB pages, Sv39
supports 2 MiB megapages
and 1 GiB gigapages, each of which must be virtually and physically
aligned to a boundary equal
to its size. A page-fault exception is raised if the physical address
is insufficiently aligned.

~ Oleksii