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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-07-26 at 17:59 +0200, Jan Beulich wrote:
> On 26.07.2023 17:54, Oleksii wrote:
> > 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.
> 
> You'd simply map the page containing the chunk, i.e. masking off the
> low 12 bits. If far enough away from the Xen virtual range, you could
> as well map a 2M page masking off the low 21 bits, or a 1G page with
> the low 30 bits of the address cleared.
Agree, then it will work.

But still it doesn't clear what to do if turn_on_mmu will be bigger
then 64 ( ASSERT( (turn_on_mmu_end - turn_on_mmu) <= 64 ) somewhere in
xen.lds.S ). Right now turn_on_mmu() function is 0x22 bytes and it is
enough ( we are sure that we don't cross 4k boundary ) to be 64-byte
aligned. But if the size will be more then 64 bytes then the alignment
need to be changed to 0x128.
Am i right?


~ Oleksii