Re: [PATCH v5 2/4] xen/riscv: introduce setup_initial_pages

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-04-24 at 12:18 +0200, Jan Beulich wrote:
> On 21.04.2023 18:01, Oleksii wrote:
> > On Thu, 2023-04-20 at 16:36 +0200, Jan Beulich wrote:
> > > On 19.04.2023 17:42, Oleksii Kurochko wrote:
> > > > +                        /* panic(), <asm/bug.h> aren't ready
> > > > now.
> > > > */
> > > > +                        die();
> > > > +                    }
> > > > +                }
> > > > +        }
> > > > +
> > > > +        /* Point to next page */
> > > > +        page_addr += XEN_PT_LEVEL_SIZE(0);
> > > 
> > > Seeing this as well as the loop heading - maybe more suitable as
> > > a
> > > for(;;) loop?
> > I am not sure that I understand the benefits of changing "while (
> > page_addr < map_end )" to "for(;;)".
> > Could you please explain me what the benefits are?
> 
> The common case of using while() is in situations where one cannot
> use for(). for() is (imo) preferable in all cases where the third of
> the controlling expressions isn't empty (and is to be carried out
> after every iteration): Any use of "continue" inside the loop will
> then properly effect loop progress. (Of course there are cases where
> this behavior isn't wanted; that's where while() may come into play
> then.)
Thanks for clarification. Now it is more clear.
> 
> > > > +    csr_write(CSR_SATP,
> > > > +              ((unsigned long)stage1_pgtbl_root >> PAGE_SHIFT)
> > > > |
> > > > +              satp_mode << SATP_MODE_SHIFT);
> > > > +
> > > > +    if ( (csr_read(CSR_SATP) >> SATP_MODE_SHIFT) == satp_mode
> > > > )
> > > > +        is_mode_supported = true;
> > > > +
> > > > +    /* Clean MMU root page table and disable MMU */
> > > > +    stage1_pgtbl_root[index] = paddr_to_pte(0x0, 0x0);
> > > > +
> > > > +    csr_write(CSR_SATP, 0);
> > > > +    asm volatile("sfence.vma");
> > > 
> > > I guess what you do in this function could do with some more
> > > comments.
> > > Looks like you're briefly enabling the MMU to check that what you
> > > wrote
> > > to SATP you can also read back. (Isn't there a register reporting
> > > whether the feature is available?)
> > I supposed that it has to be but I couldn't find a register in
> > docs.
> 
> Well, yes, interestingly the register is marked WARL, so apparently
> intended to by used for probing like you do. (I find the definition
> of
> WARL a little odd though, as such writes supposedly aren't
> necessarily
> value preserving. For SATP this might mean that translation is
> enabled
> by a write of an unsupported mode, with a different number of levels.
> This isn't going to work very well, I'm afraid.)
Agree. It will be an issue in case of a different number of levels.

Then it looks there is no way to check if SATP mode is supported.

So we have to rely on the fact that the developer specified
RV_STAGE1_MODE correctly in the config file.

> 
> > > > +    /*
> > > > +     * Stack should be re-inited as:
> > > > +     * 1. Right now an address of the stack is relative to
> > > > load
> > > > time
> > > > +     *    addresses what will cause an issue in case of load
> > > > start
> > > > address
> > > > +     *    isn't equal to linker start address.
> > > > +     * 2. Addresses in stack are all load time relative which
> > > > can
> > > > be an
> > > > +     *    issue in case when load start address isn't equal to
> > > > linker
> > > > +     *    start address.
> > > > +     */
> > > > +    asm volatile ("mv sp, %0" : : "r"((unsigned
> > > > long)cpu0_boot_stack + STACK_SIZE));
> > > 
> > > Nit: Style (overly long line).
> > > 
> > > What's worse - I don't think it is permitted to alter sp in the
> > > middle of
> > > a function. The compiler may maintain local variables on the
> > > stack
> > > which
> > > don't correspond to any programmer specified ones, including
> > > pointer
> > > ones
> > > which point into the stack frame. This is specifically why both
> > > x86
> > > and
> > > Arm have switch_stack_and_jump() macros.
> > but the macros (from ARM) looks equal to the code mentioned above:
> > #define switch_stack_and_jump(stack, fn) do
> > {                         
> > \
> >     asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn)
> > :
> > "memory" ); \
> 
> Note how writing SP and branch are contained in a single asm() there.
> By checking ...
> 
> >    
> > unreachable();                                                    
> > \
> > } while ( false )
> > 
> > Here is part of disassembled enable_mmu():
> > 
> > ffffffffc004aedc:       18079073                csrw    satp,a5
> > ffffffffc004aee0:       00016797                auipc   a5,0x16
> > ffffffffc004aee4:       12078793                addi    a5,a5,288
> > ffffffffc004aee8:       813e                    mv      sp,a5
> > ffffffffc004af00:       0f4000ef                jal    
> > ra,ffffffffc004aff4 <cont_after_mmu_is_enabled>
> > ...
> 
> ... what the generated code in your case is you won't guarantee that
> things remain that way with future (or simply different) compilers.
Agree. Thanks for clarification. I'll take into account during the next
version of patch series.

> 
> > > > --- a/xen/arch/riscv/riscv64/head.S
> > > > +++ b/xen/arch/riscv/riscv64/head.S
> > > > @@ -1,4 +1,5 @@
> > > >  #include <asm/asm.h>
> > > > +#include <asm/asm-offsets.h>
> > > >  #include <asm/riscv_encoding.h>
> > > >  
> > > >          .section .text.header, "ax", %progbits
> > > > @@ -32,3 +33,4 @@ ENTRY(start)
> > > >          add     sp, sp, t0
> > > >  
> > > >          tail    start_xen
> > > > +
> > > 
> > > ???
> > Shouldn't it be the one empty line at the end of a file?
> 
> There should be a newline at the end of a file, but not normally a
> blank one. When you introduce a new file, it can be viewed as a
> matter
> of taste whether to have an empty last line, but when you have a
> seemingly unrelated change to a file like the one here, this is at
> least odd.
Agree. Then I'll remove this change from the patch series.

> 
> > > > --- a/xen/arch/riscv/xen.lds.S
> > > > +++ b/xen/arch/riscv/xen.lds.S
> > > > @@ -136,6 +136,7 @@ SECTIONS
> > > >      . = ALIGN(POINTER_ALIGN);
> > > >      __init_end = .;
> > > >  
> > > > +    . = ALIGN(PAGE_SIZE);
> > > >      .bss : {                     /* BSS */
> > > >          __bss_start = .;
> > > >          *(.bss.stack_aligned)
> > > 
> > > Why do you need this? You properly use __aligned(PAGE_SIZE) for
> > > the
> > > page tables you define, and PAGE_SIZE wouldn't be enough here
> > > anyway
> > > if STACK_SIZE > PAGE_SIZE (as .bss.stack_aligned comes first).
> > > The
> > > only time you'd need such an ALIGN() is if the following label
> > > (__bss_start in this case) needed to be aligned at a certain
> > > boundary. (I'm a little puzzled though that __bss_start isn't
> > > [immediately] preceded by ". = ALIGN(POINTER_ALIGN);" - didn't
> > > .bss
> > > clearing rely on such alignment?)
> > ALIGN(PAGE_SIZE)  isn't needed anymore.
> > I used it to have 4k aligned physical address for PTE when I mapped
> > each section separately ( it was so in the previous verstion of MMU
> > patch series )
> > 
> > Regarding ". = ALIGN(POINTER_ALIGN);" I would say that it is enough
> > to
> > have aligned __bss_end ( what was done ) to be sure that we can
> > clear
> > __SIZEOF_POINTER__ bytes each iteration of .bss clearing loop and
> > don't
> > worry that size of .bss section may not be divisible by
> > __SIZEOF_POINTER__.
> 
> How would guaranteeing this only for __bss_end help? __bss_start
> could
> still be misaligned, and then you'd
> (a) use misaligned stores for clearing and
> (b) extend clearing to outside of the .bss (as the last of the
> misaligned
> stores would cross the __bss_end boundary).
It seems you are right. I'll create a separate commit to align
__bss_start properly.

~ Oleksii