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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.)

>>> +    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.)

>>> +    /*
>>> +     * 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.

>>> --- 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.

>>> --- 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).

Jan