Re: [PATCH v1 1/8] xen/riscv: make sure that identity mapping isn't bigger then page size

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-06-12 at 09:09 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> Such commits without description are worrying. This may be okay for
> entirely trivial and obvious changes, but that's going to be the
> exception.
> 
> > --- a/xen/arch/riscv/xen.lds.S
> > +++ b/xen/arch/riscv/xen.lds.S
> > @@ -20,6 +20,7 @@ SECTIONS
> >      . = XEN_VIRT_START;
> >      _start = .;
> >      .text : {
> > +        _idmap_start = .;
> >          _stext = .;            /* Text section */
> >          *(.text.header)
> >  
> > @@ -35,6 +36,7 @@ SECTIONS
> >          *(.gnu.warning)
> >          . = ALIGN(POINTER_ALIGN);
> >          _etext = .;             /* End of text section */
> > +        _idmap_end = .;
> >      } :text
> 
> So this covers all of .text. Why is it expected that .text will be
> (and
> remain) ...
> 
> > @@ -174,3 +176,10 @@ ASSERT(!SIZEOF(.got),      ".got non-empty")
> >  ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
> >  
> >  ASSERT(_end - _start <= MB(2), "Xen too large for early-boot
> > assumptions")
> > +
> > +/*
> > + * We require that Xen is loaded at a page boundary, so this
> > ensures that any
> > + * code running on the identity map cannot cross a page boundary.
> > + */
> > +ASSERT(IS_ALIGNED(_idmap_start, PAGE_SIZE), "_idmap_start should
> > be page-aligned")
> > +ASSERT(_idmap_end - _idmap_start <= PAGE_SIZE, "Identity mapped
> > code is larger than a page size")
> 
> ... less than 4k in size? And why is only .text of interest, but not
> other sections?
An idea was to keep identity mapping as small as possible because
basically identity mapping is needed only for a few instructions.
(probably it will be better to create a separate section and put all
necessary functions there)

Another point was to map the necessary code for switching from 1:1
mapping in one cycle. ( we are using 4K as a page size )

But it looks like PAGE_SIZE isn't enough. I rebased all my patches that
are needed to run Dom0 and compiler complains that _idmap is bigger
than PAGE_SIZE so I probably have to reject this idea ( to map only
PAGE_SIZE ).

Actually not only .text section is needed but also stack should be 1:1
mapped. ( what is done in setup_initial_pagetables() )
> 
> I find the other assertion a little puzzling too: Isn't that merely
> checking that XEN_VIRT_START is page aligned?
Yeah, you are right.

~ Oleksii