[PATCH v1 0/5] Setup memory management

[PATCH v1 0/5] Setup memory management

[Oleksii Kurochko]

Finish initializing the memory subsystem by mapping the direct map and
frame table.
In the case of RISC-V 64, which has a large virtual address space
(the minimum supported MMU mode is Sv39, providing TB of VA space),
the direct map and frame table are mapped starting from physical address
0. This simplifies the calculations and thereby improves performance for
page_to_mfn(), mfn_to_page(), and maddr_to_virt(), as there is no
need to account for {directmap, frametable}_base_pdx.
In addition, initialize the VMAP_DEFAULT region, finalize the boot allocator,
and update the system state from early_boot to boot.

Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this patch series,
the linkage error mentioned in patch 1 ("xen/riscv: add stub for
share_xen_page_with_guest()") began to occur, so patch 1 addresses this issue.

The function maddr_to_virt() is introduced as part of this patch series, as
setup_directmap_mappings() uses it indirectly through mfn_to_virt().

Oleksii Kurochko (5):
  xen/riscv: add stub for share_xen_page_with_guest()
  xen/riscv: implement maddr_to_virt()
  xen/riscv: introduce setup_mm()
  xen/riscv: initialize the VMAP_DEFAULT virtual range
  xen/riscv: finalize boot allocator and transition to boot state

 xen/arch/riscv/include/asm/mm.h    |  10 ++-
 xen/arch/riscv/include/asm/setup.h |   2 +
 xen/arch/riscv/mm.c                | 104 +++++++++++++++++++++++++++--
 xen/arch/riscv/pt.c                |   6 ++
 xen/arch/riscv/setup.c             |  14 ++++
 xen/arch/riscv/stubs.c             |  10 +++
 6 files changed, 138 insertions(+), 8 deletions(-)

-- 
2.47.0

[PATCH v1 1/5] xen/riscv: add stub for share_xen_page_with_guest()

[Oleksii Kurochko]

To avoid the following linkage fail the stub for share_xen_page_with_guest()
is introduced:
  riscv64-linux-gnu-ld: prelink.o: in function `tasklet_kill':
  /build/xen/common/tasklet.c:176: undefined reference to `share_xen_page_with_guest'
  riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `share_xen_page_with_guest' isn't defined
  riscv64-linux-gnu-ld: final link failed: bad value

$ find . -name \*.o | while read F; do nm $F | grep share_xen_page_with_guest && echo $F; done
                 U share_xen_page_with_guest
./xen/common/built_in.o
                 U share_xen_page_with_guest
./xen/common/trace.o
                 U share_xen_page_with_guest
./xen/prelink.o

Despite the linker fingering tasklet.c (very likely a toolchain bug),
it's trace.o which has the undefined reference.

Looking at trace.i, there is call of share_xen_page_with_guest() in
share_xen_page_with_privileged_guests() from asm/mm.h.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/stubs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 5951b0ce91..c9a590b225 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -2,7 +2,9 @@
 #include <xen/cpumask.h>
 #include <xen/domain.h>
 #include <xen/irq.h>
+#include <xen/mm.h>
 #include <xen/nodemask.h>
+#include <xen/sched.h>
 #include <xen/sections.h>
 #include <xen/time.h>
 #include <public/domctl.h>
@@ -409,3 +411,11 @@ unsigned long get_upper_mfn_bound(void)
 {
     BUG_ON("unimplemented");
 }
+
+/* mm.c */
+
+void share_xen_page_with_guest(struct page_info *page, struct domain *d,
+                               enum XENSHARE_flags flags)
+{
+    BUG_ON("unimplemented");
+}
-- 
2.47.0

Re: [PATCH v1 1/5] xen/riscv: add stub for share_xen_page_with_guest()

[Jan Beulich]

On 16.10.2024 11:15, Oleksii Kurochko wrote:
> To avoid the following linkage fail the stub for share_xen_page_with_guest()
> is introduced:

What do you intend to express with "is introduced"? Is there a problem now?
Would there be a problem with subsequent changes? I'm entirely fine with
adding that stub, but the description should make clear why /when exactly
it's needed.

Jan

>   riscv64-linux-gnu-ld: prelink.o: in function `tasklet_kill':
>   /build/xen/common/tasklet.c:176: undefined reference to `share_xen_page_with_guest'
>   riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `share_xen_page_with_guest' isn't defined
>   riscv64-linux-gnu-ld: final link failed: bad value
> 
> $ find . -name \*.o | while read F; do nm $F | grep share_xen_page_with_guest && echo $F; done
>                  U share_xen_page_with_guest
> ./xen/common/built_in.o
>                  U share_xen_page_with_guest
> ./xen/common/trace.o
>                  U share_xen_page_with_guest
> ./xen/prelink.o
> 
> Despite the linker fingering tasklet.c (very likely a toolchain bug),
> it's trace.o which has the undefined reference.
> 
> Looking at trace.i, there is call of share_xen_page_with_guest() in
> share_xen_page_with_privileged_guests() from asm/mm.h.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/stubs.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> index 5951b0ce91..c9a590b225 100644
> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -2,7 +2,9 @@
>  #include <xen/cpumask.h>
>  #include <xen/domain.h>
>  #include <xen/irq.h>
> +#include <xen/mm.h>
>  #include <xen/nodemask.h>
> +#include <xen/sched.h>
>  #include <xen/sections.h>
>  #include <xen/time.h>
>  #include <public/domctl.h>
> @@ -409,3 +411,11 @@ unsigned long get_upper_mfn_bound(void)
>  {
>      BUG_ON("unimplemented");
>  }
> +
> +/* mm.c */
> +
> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
> +                               enum XENSHARE_flags flags)
> +{
> +    BUG_ON("unimplemented");
> +}

Re: [PATCH v1 1/5] xen/riscv: add stub for share_xen_page_with_guest()

[oleksii.kurochko]

On Thu, 2024-10-17 at 16:51 +0200, Jan Beulich wrote:
> On 16.10.2024 11:15, Oleksii Kurochko wrote:
> > To avoid the following linkage fail the stub for
> > share_xen_page_with_guest()
> > is introduced:
> 
> What do you intend to express with "is introduced"? Is there a
> problem now?
> Would there be a problem with subsequent changes? I'm entirely fine
> with
> adding that stub, but the description should make clear why /when
> exactly
> it's needed.
I mentioned that in the cover letter:
```
   Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this
   patch series,
   the linkage error mentioned in patch 1 ("xen/riscv: add stub for
   share_xen_page_with_guest()") began to occur, so patch 1 addresses
   this issue.
```
I thought it would be the better then just mention in the commit
message that.

Will it be fine to mention instead:
```
   Introduction of setup memory management function will require
   share_xen_page_with_guest() defined, at least, as a stub otherwise
   the following linkage error will occur...
```

Perhaps it would be better just to merge these changes with patch 3 and
add an explanation in patch 3 commit message.

~ Oleksii
> >   riscv64-linux-gnu-ld: prelink.o: in function `tasklet_kill':
> >   /build/xen/common/tasklet.c:176: undefined reference to
> > `share_xen_page_with_guest'
> >   riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol
> > `share_xen_page_with_guest' isn't defined
> >   riscv64-linux-gnu-ld: final link failed: bad value
> > 
> > $ find . -name \*.o | while read F; do nm $F | grep
> > share_xen_page_with_guest && echo $F; done
> >                  U share_xen_page_with_guest
> > ./xen/common/built_in.o
> >                  U share_xen_page_with_guest
> > ./xen/common/trace.o
> >                  U share_xen_page_with_guest
> > ./xen/prelink.o
> > 
> > Despite the linker fingering tasklet.c (very likely a toolchain
> > bug),
> > it's trace.o which has the undefined reference.
> > 
> > Looking at trace.i, there is call of share_xen_page_with_guest() in
> > share_xen_page_with_privileged_guests() from asm/mm.h.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >  xen/arch/riscv/stubs.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> > index 5951b0ce91..c9a590b225 100644
> > --- a/xen/arch/riscv/stubs.c
> > +++ b/xen/arch/riscv/stubs.c
> > @@ -2,7 +2,9 @@
> >  #include <xen/cpumask.h>
> >  #include <xen/domain.h>
> >  #include <xen/irq.h>
> > +#include <xen/mm.h>
> >  #include <xen/nodemask.h>
> > +#include <xen/sched.h>
> >  #include <xen/sections.h>
> >  #include <xen/time.h>
> >  #include <public/domctl.h>
> > @@ -409,3 +411,11 @@ unsigned long get_upper_mfn_bound(void)
> >  {
> >      BUG_ON("unimplemented");
> >  }
> > +
> > +/* mm.c */
> > +
> > +void share_xen_page_with_guest(struct page_info *page, struct
> > domain *d,
> > +                               enum XENSHARE_flags flags)
> > +{
> > +    BUG_ON("unimplemented");
> > +}
> 

Re: [PATCH v1 1/5] xen/riscv: add stub for share_xen_page_with_guest()

[Jan Beulich]

On 18.10.2024 15:10, oleksii.kurochko@gmail.com wrote:
> On Thu, 2024-10-17 at 16:51 +0200, Jan Beulich wrote:
>> On 16.10.2024 11:15, Oleksii Kurochko wrote:
>>> To avoid the following linkage fail the stub for
>>> share_xen_page_with_guest()
>>> is introduced:
>>
>> What do you intend to express with "is introduced"? Is there a
>> problem now?
>> Would there be a problem with subsequent changes? I'm entirely fine
>> with
>> adding that stub, but the description should make clear why /when
>> exactly
>> it's needed.
> I mentioned that in the cover letter:
> ```
>    Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this
>    patch series,
>    the linkage error mentioned in patch 1 ("xen/riscv: add stub for
>    share_xen_page_with_guest()") began to occur, so patch 1 addresses
>    this issue.
> ```
> I thought it would be the better then just mention in the commit
> message that.

Mentioning in the cover letter is fine, but each patch needs to also
be self-contained.

> Will it be fine to mention instead:
> ```
>    Introduction of setup memory management function will require
>    share_xen_page_with_guest() defined, at least, as a stub otherwise
>    the following linkage error will occur...
> ```

Quoting the linker error is imo of limited use. What such an explanation
wants to say is why, all of the sudden, such an error occurs. After all
you don't change anything directly related to common/trace.c.

> Perhaps it would be better just to merge these changes with patch 3 and
> add an explanation in patch 3 commit message.

Perhaps, as you say.

Jan

Re: [PATCH v1 1/5] xen/riscv: add stub for share_xen_page_with_guest()

[oleksii.kurochko]

On Fri, 2024-10-18 at 15:27 +0200, Jan Beulich wrote:
> On 18.10.2024 15:10, oleksii.kurochko@gmail.com wrote:
> > On Thu, 2024-10-17 at 16:51 +0200, Jan Beulich wrote:
> > > On 16.10.2024 11:15, Oleksii Kurochko wrote:
> > > > To avoid the following linkage fail the stub for
> > > > share_xen_page_with_guest()
> > > > is introduced:
> > > 
> > > What do you intend to express with "is introduced"? Is there a
> > > problem now?
> > > Would there be a problem with subsequent changes? I'm entirely
> > > fine
> > > with
> > > adding that stub, but the description should make clear why /when
> > > exactly
> > > it's needed.
> > I mentioned that in the cover letter:
> > ```
> >    Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this
> >    patch series,
> >    the linkage error mentioned in patch 1 ("xen/riscv: add stub for
> >    share_xen_page_with_guest()") began to occur, so patch 1
> > addresses
> >    this issue.
> > ```
> > I thought it would be the better then just mention in the commit
> > message that.
> 
> Mentioning in the cover letter is fine, but each patch needs to also
> be self-contained.
> 
> > Will it be fine to mention instead:
> > ```
> >    Introduction of setup memory management function will require
> >    share_xen_page_with_guest() defined, at least, as a stub
> > otherwise
> >    the following linkage error will occur...
> > ```
> 
> Quoting the linker error is imo of limited use. What such an
> explanation
> wants to say is why, all of the sudden, such an error occurs. After
> all
> you don't change anything directly related to common/trace.c.
if maddr_to_virt() is defined as:
    static inline void *maddr_to_virt(paddr_t ma)
   {
       BUG_ON("unimplemented");
       return NULL;
       // /* Offset in the direct map, accounting for pdx compression */
       // unsigned long va_offset = maddr_to_directmapoff(ma);
   
       // ASSERT(va_offset < DIRECTMAP_SIZE);
   
       // return (void *)(DIRECTMAP_VIRT_START + va_offset);
   }
Then no stub for share_xen_page_with_privileged_guests() is needed but
it isn't clear at all why the definition of maddr_to_virt() affects
linkage of share_xen_page_with_privileged_guests().

I tried to look what is the difference after preprocessing stage for
common/trace.o and the only difference is in how maddr_to_virt() is
implemented:
   7574a7575,7577
   >     do { if (__builtin_expect(!!("unimplemented"),0)) do { do { ({
   _Static_assert(!((30) >> ((31 - 24) + (31 - 24))), "!(" "(30) >>
   (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)" ")"); }); ({
   _Static_assert(!((2) >= 4), "!(" "(2) >= BUGFRAME_NR" ")"); }); asm
   volatile ( ".Lbug%=:""unimp""\n" "   .pushsection
   .bug_frames.%""""[bf_type], \"a\", %%progbits\n" "   .p2align 2\n"
   ".Lfrm%=:\n" "   .long (.Lbug%= - .Lfrm%=) + %""""[bf_line_hi]\n" "
   .long (%""""[bf_ptr] - .Lfrm%=) + %""""[bf_line_lo]\n" "   .if " "0"
   "\n" "   .long 0, %""""[bf_msg] - .Lfrm%=\n" "   .endif\n" "  
   .popsection\n" :: [bf_type] "i" (2), [bf_ptr] "i"
   ("./arch/riscv/include/asm/mm.h"), [bf_msg] "i" (((void*)0)),
   [bf_line_lo] "i" (((30) & ((1 << (31 - 24)) - 1)) << 24),
   [bf_line_hi] "i" (((30) >> (31 - 24)) << 24) ); } while ( 0 );
   __builtin_unreachable(); } while ( 0 ); } while (0);
   >     return ((void*)0);
   > 
   7578d7580
   <     unsigned long va_offset = (ma);
   7580d7581
   <     do { if ( __builtin_expect(!!(!(va_offset < ((((vaddr_t)(509))
   << ((3 - 1) * (9) + 12)) - (((vaddr_t)(200)) << ((3 - 1) * (9) +
   12))))),0) ) do { do { ({ _Static_assert(!((35) >> ((31 - 24) + (31
   - 24))), "!(" "(35) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)"
   ")"); }); ({ _Static_assert(!((3) >= 4), "!(" "(3) >= BUGFRAME_NR"
   ")"); }); asm volatile ( ".Lbug%=:""unimp""\n" "   .pushsection
   .bug_frames.%""""[bf_type], \"a\", %%progbits\n" "   .p2align 2\n"
   ".Lfrm%=:\n" "   .long (.Lbug%= - .Lfrm%=) + %""""[bf_line_hi]\n" "
   .long (%""""[bf_ptr] - .Lfrm%=) + %""""[bf_line_lo]\n" "   .if " "1"
   "\n" "   .long 0, %""""[bf_msg] - .Lfrm%=\n" "   .endif\n" "  
   .popsection\n" :: [bf_type] "i" (3), [bf_ptr] "i"
   ("./arch/riscv/include/asm/mm.h"), [bf_msg] "i" ("va_offset <
   DIRECTMAP_SIZE"), [bf_line_lo] "i" (((35) & ((1 << (31 - 24)) - 1))
   << 24), [bf_line_hi] "i" (((35) >> (31 - 24)) << 24) ); } while ( 0
   ); __builtin_unreachable(); } while ( 0 ); } while (0);
   7582d7582
   <     return (void *)((((vaddr_t)(200)) << ((3 - 1) * (9) + 12)) +
   va_offset);

Could it be that DCE likely happen when maddr_to_virt() is defined as
stub and so code which call share_xen_page_with_privileged_guests() is
just eliminated? ( but I am not sure that I know fast way to check that
, do you have any pointers? )

~ Oleksii

Re: [PATCH v1 1/5] xen/riscv: add stub for share_xen_page_with_guest()

[Jan Beulich]

On 18.10.2024 17:57, oleksii.kurochko@gmail.com wrote:
> On Fri, 2024-10-18 at 15:27 +0200, Jan Beulich wrote:
>> On 18.10.2024 15:10, oleksii.kurochko@gmail.com wrote:
>>> On Thu, 2024-10-17 at 16:51 +0200, Jan Beulich wrote:
>>>> On 16.10.2024 11:15, Oleksii Kurochko wrote:
>>>>> To avoid the following linkage fail the stub for
>>>>> share_xen_page_with_guest()
>>>>> is introduced:
>>>>
>>>> What do you intend to express with "is introduced"? Is there a
>>>> problem now?
>>>> Would there be a problem with subsequent changes? I'm entirely
>>>> fine
>>>> with
>>>> adding that stub, but the description should make clear why /when
>>>> exactly
>>>> it's needed.
>>> I mentioned that in the cover letter:
>>> ```
>>>    Also, after patch 3 ("xen/riscv: introduce setup_mm()") of this
>>>    patch series,
>>>    the linkage error mentioned in patch 1 ("xen/riscv: add stub for
>>>    share_xen_page_with_guest()") began to occur, so patch 1
>>> addresses
>>>    this issue.
>>> ```
>>> I thought it would be the better then just mention in the commit
>>> message that.
>>
>> Mentioning in the cover letter is fine, but each patch needs to also
>> be self-contained.
>>
>>> Will it be fine to mention instead:
>>> ```
>>>    Introduction of setup memory management function will require
>>>    share_xen_page_with_guest() defined, at least, as a stub
>>> otherwise
>>>    the following linkage error will occur...
>>> ```
>>
>> Quoting the linker error is imo of limited use. What such an
>> explanation
>> wants to say is why, all of the sudden, such an error occurs. After
>> all
>> you don't change anything directly related to common/trace.c.
> if maddr_to_virt() is defined as:
>     static inline void *maddr_to_virt(paddr_t ma)
>    {
>        BUG_ON("unimplemented");
>        return NULL;
>        // /* Offset in the direct map, accounting for pdx compression */
>        // unsigned long va_offset = maddr_to_directmapoff(ma);
>    
>        // ASSERT(va_offset < DIRECTMAP_SIZE);
>    
>        // return (void *)(DIRECTMAP_VIRT_START + va_offset);
>    }
> Then no stub for share_xen_page_with_privileged_guests() is needed but
> it isn't clear at all why the definition of maddr_to_virt() affects
> linkage of share_xen_page_with_privileged_guests().
> 
> I tried to look what is the difference after preprocessing stage for
> common/trace.o and the only difference is in how maddr_to_virt() is
> implemented:
>    7574a7575,7577
>    >     do { if (__builtin_expect(!!("unimplemented"),0)) do { do { ({
>    _Static_assert(!((30) >> ((31 - 24) + (31 - 24))), "!(" "(30) >>
>    (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)" ")"); }); ({
>    _Static_assert(!((2) >= 4), "!(" "(2) >= BUGFRAME_NR" ")"); }); asm
>    volatile ( ".Lbug%=:""unimp""\n" "   .pushsection
>    .bug_frames.%""""[bf_type], \"a\", %%progbits\n" "   .p2align 2\n"
>    ".Lfrm%=:\n" "   .long (.Lbug%= - .Lfrm%=) + %""""[bf_line_hi]\n" "
>    .long (%""""[bf_ptr] - .Lfrm%=) + %""""[bf_line_lo]\n" "   .if " "0"
>    "\n" "   .long 0, %""""[bf_msg] - .Lfrm%=\n" "   .endif\n" "  
>    .popsection\n" :: [bf_type] "i" (2), [bf_ptr] "i"
>    ("./arch/riscv/include/asm/mm.h"), [bf_msg] "i" (((void*)0)),
>    [bf_line_lo] "i" (((30) & ((1 << (31 - 24)) - 1)) << 24),
>    [bf_line_hi] "i" (((30) >> (31 - 24)) << 24) ); } while ( 0 );
>    __builtin_unreachable(); } while ( 0 ); } while (0);
>    >     return ((void*)0);
>    > 
>    7578d7580
>    <     unsigned long va_offset = (ma);
>    7580d7581
>    <     do { if ( __builtin_expect(!!(!(va_offset < ((((vaddr_t)(509))
>    << ((3 - 1) * (9) + 12)) - (((vaddr_t)(200)) << ((3 - 1) * (9) +
>    12))))),0) ) do { do { ({ _Static_assert(!((35) >> ((31 - 24) + (31
>    - 24))), "!(" "(35) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)"
>    ")"); }); ({ _Static_assert(!((3) >= 4), "!(" "(3) >= BUGFRAME_NR"
>    ")"); }); asm volatile ( ".Lbug%=:""unimp""\n" "   .pushsection
>    .bug_frames.%""""[bf_type], \"a\", %%progbits\n" "   .p2align 2\n"
>    ".Lfrm%=:\n" "   .long (.Lbug%= - .Lfrm%=) + %""""[bf_line_hi]\n" "
>    .long (%""""[bf_ptr] - .Lfrm%=) + %""""[bf_line_lo]\n" "   .if " "1"
>    "\n" "   .long 0, %""""[bf_msg] - .Lfrm%=\n" "   .endif\n" "  
>    .popsection\n" :: [bf_type] "i" (3), [bf_ptr] "i"
>    ("./arch/riscv/include/asm/mm.h"), [bf_msg] "i" ("va_offset <
>    DIRECTMAP_SIZE"), [bf_line_lo] "i" (((35) & ((1 << (31 - 24)) - 1))
>    << 24), [bf_line_hi] "i" (((35) >> (31 - 24)) << 24) ); } while ( 0
>    ); __builtin_unreachable(); } while ( 0 ); } while (0);
>    7582d7582
>    <     return (void *)((((vaddr_t)(200)) << ((3 - 1) * (9) + 12)) +
>    va_offset);
> 
> Could it be that DCE likely happen when maddr_to_virt() is defined as
> stub and so code which call share_xen_page_with_privileged_guests() is
> just eliminated? ( but I am not sure that I know fast way to check that
> , do you have any pointers? )

Yes, I think DCE is the explanation here. And that's what (imo) wants saying
in the description.

Jan

[PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

[Oleksii Kurochko]

Implement the `maddr_to_virt()` function to convert a machine address
to a virtual address. This function is specifically designed to be used
only for the DIRECTMAP region, so a check has been added to ensure that
the address does not exceed `DIRECTMAP_SIZE`.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/mm.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index ebb142502e..0396e66f47 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -25,8 +25,12 @@
 
 static inline void *maddr_to_virt(paddr_t ma)
 {
-    BUG_ON("unimplemented");
-    return NULL;
+    /* Offset in the direct map, accounting for pdx compression */
+    unsigned long va_offset = maddr_to_directmapoff(ma);
+
+    ASSERT(va_offset < DIRECTMAP_SIZE);
+
+    return (void *)(DIRECTMAP_VIRT_START + va_offset);
 }
 
 /*
-- 
2.47.0

Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

[Alejandro Vallejo]

On Wed Oct 16, 2024 at 10:15 AM BST, Oleksii Kurochko wrote:
> Implement the `maddr_to_virt()` function to convert a machine address
> to a virtual address. This function is specifically designed to be used
> only for the DIRECTMAP region, so a check has been added to ensure that
> the address does not exceed `DIRECTMAP_SIZE`.
>

nit: Worth mentioning this comes from the x86 side of things.

> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/include/asm/mm.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
> index ebb142502e..0396e66f47 100644
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -25,8 +25,12 @@
>  
>  static inline void *maddr_to_virt(paddr_t ma)
>  {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    /* Offset in the direct map, accounting for pdx compression */
> +    unsigned long va_offset = maddr_to_directmapoff(ma);
> +
> +    ASSERT(va_offset < DIRECTMAP_SIZE);
> +
> +    return (void *)(DIRECTMAP_VIRT_START + va_offset);
>  }
>  
>  /*

Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

[Jan Beulich]

On 16.10.2024 11:15, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -25,8 +25,12 @@
>  
>  static inline void *maddr_to_virt(paddr_t ma)
>  {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    /* Offset in the direct map, accounting for pdx compression */
> +    unsigned long va_offset = maddr_to_directmapoff(ma);

Why the mentioning of PDX compression? At least right now it's unavailable
for RISC-V afaics. Are there plans to change that any time soon?

Jan

Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

[oleksii.kurochko]

On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote:
> On 16.10.2024 11:15, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -25,8 +25,12 @@
> >  
> >  static inline void *maddr_to_virt(paddr_t ma)
> >  {
> > -    BUG_ON("unimplemented");
> > -    return NULL;
> > +    /* Offset in the direct map, accounting for pdx compression */
> > +    unsigned long va_offset = maddr_to_directmapoff(ma);
> 
> Why the mentioning of PDX compression?
It was mentioned because if PDX will be enabled maddr_to_directmapoff()
will take into account PDX stuff.

>  At least right now it's unavailable
> for RISC-V afaics. Are there plans to change that any time soon?
At the moment, I don't have such plans, looking at available platform
there are no a lot of benefits of having PDX compression now.

Perhaps it would be good to add
BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which should
be updated when CONFIG_PDX will be enabled.

~ Oleksii

Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

[Alejandro Vallejo]

On Fri Oct 18, 2024 at 2:17 PM BST, oleksii.kurochko wrote:
> On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote:
> > On 16.10.2024 11:15, Oleksii Kurochko wrote:
> > > --- a/xen/arch/riscv/include/asm/mm.h
> > > +++ b/xen/arch/riscv/include/asm/mm.h
> > > @@ -25,8 +25,12 @@
> > >  
> > >  static inline void *maddr_to_virt(paddr_t ma)
> > >  {
> > > -    BUG_ON("unimplemented");
> > > -    return NULL;
> > > +    /* Offset in the direct map, accounting for pdx compression */
> > > +    unsigned long va_offset = maddr_to_directmapoff(ma);
> > 
> > Why the mentioning of PDX compression?
> It was mentioned because if PDX will be enabled maddr_to_directmapoff()
> will take into account PDX stuff.
>
> >  At least right now it's unavailable
> > for RISC-V afaics. Are there plans to change that any time soon?
> At the moment, I don't have such plans, looking at available platform
> there are no a lot of benefits of having PDX compression now.
>
> Perhaps it would be good to add
> BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which should
> be updated when CONFIG_PDX will be enabled.
>
> ~ Oleksii

I'd just forget about it unless you ever notice you're wasting a lot of entries
in the frame table due to empty space in the memory map. Julien measured the
effect on Amazon's Live Migration as a 10% improvement in downtime with PDX
off.

PDX compression shines when you have separate RAM banks at very, very
disparately far addresses (specifics in pdx.h). Unfortunately the flip side of
this compression is that you get several memory accesses for each single
pdx-(to/from)-mfn conversion. And we do a lot of those. One possible solution
would be to alt-patch the values in the code-stream and avoid the perf-hit, but
that's not merged. Jan had some patches but that didn't make it to staging,
IIRC.

Cheers,
Alejandro

Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

[oleksii.kurochko]

On Mon, 2024-10-21 at 08:56 +0100, Alejandro Vallejo wrote:
> On Fri Oct 18, 2024 at 2:17 PM BST, oleksii.kurochko wrote:
> > On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote:
> > > On 16.10.2024 11:15, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/include/asm/mm.h
> > > > +++ b/xen/arch/riscv/include/asm/mm.h
> > > > @@ -25,8 +25,12 @@
> > > >  
> > > >  static inline void *maddr_to_virt(paddr_t ma)
> > > >  {
> > > > -    BUG_ON("unimplemented");
> > > > -    return NULL;
> > > > +    /* Offset in the direct map, accounting for pdx
> > > > compression */
> > > > +    unsigned long va_offset = maddr_to_directmapoff(ma);
> > > 
> > > Why the mentioning of PDX compression?
> > It was mentioned because if PDX will be enabled
> > maddr_to_directmapoff()
> > will take into account PDX stuff.
> > 
> > >  At least right now it's unavailable
> > > for RISC-V afaics. Are there plans to change that any time soon?
> > At the moment, I don't have such plans, looking at available
> > platform
> > there are no a lot of benefits of having PDX compression now.
> > 
> > Perhaps it would be good to add
> > BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which
> > should
> > be updated when CONFIG_PDX will be enabled.
> > 
> > ~ Oleksii
> 
> I'd just forget about it unless you ever notice you're wasting a lot
> of entries
> in the frame table due to empty space in the memory map. Julien
> measured the
> effect on Amazon's Live Migration as a 10% improvement in downtime
> with PDX
> off.
> 
> PDX compression shines when you have separate RAM banks at very, very
> disparately far addresses (specifics in pdx.h). Unfortunately the
> flip side of
> this compression is that you get several memory accesses for each
> single
> pdx-(to/from)-mfn conversion. And we do a lot of those. One possible
> solution
> would be to alt-patch the values in the code-stream and avoid the
> perf-hit, but
> that's not merged. Jan had some patches but that didn't make it to
> staging,
> IIRC.
Could you please give me some links in the mailing list with mentioned
patches?

~ Oleksii

Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

[Alejandro Vallejo]

On Mon Oct 21, 2024 at 10:17 AM BST, oleksii.kurochko wrote:
> On Mon, 2024-10-21 at 08:56 +0100, Alejandro Vallejo wrote:
> > On Fri Oct 18, 2024 at 2:17 PM BST, oleksii.kurochko wrote:
> > > On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote:
> > > > On 16.10.2024 11:15, Oleksii Kurochko wrote:
> > > > > --- a/xen/arch/riscv/include/asm/mm.h
> > > > > +++ b/xen/arch/riscv/include/asm/mm.h
> > > > > @@ -25,8 +25,12 @@
> > > > >  
> > > > >  static inline void *maddr_to_virt(paddr_t ma)
> > > > >  {
> > > > > -    BUG_ON("unimplemented");
> > > > > -    return NULL;
> > > > > +    /* Offset in the direct map, accounting for pdx
> > > > > compression */
> > > > > +    unsigned long va_offset = maddr_to_directmapoff(ma);
> > > > 
> > > > Why the mentioning of PDX compression?
> > > It was mentioned because if PDX will be enabled
> > > maddr_to_directmapoff()
> > > will take into account PDX stuff.
> > > 
> > > >  At least right now it's unavailable
> > > > for RISC-V afaics. Are there plans to change that any time soon?
> > > At the moment, I don't have such plans, looking at available
> > > platform
> > > there are no a lot of benefits of having PDX compression now.
> > > 
> > > Perhaps it would be good to add
> > > BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which
> > > should
> > > be updated when CONFIG_PDX will be enabled.
> > > 
> > > ~ Oleksii
> > 
> > I'd just forget about it unless you ever notice you're wasting a lot
> > of entries
> > in the frame table due to empty space in the memory map. Julien
> > measured the
> > effect on Amazon's Live Migration as a 10% improvement in downtime
> > with PDX
> > off.
> > 
> > PDX compression shines when you have separate RAM banks at very, very
> > disparately far addresses (specifics in pdx.h). Unfortunately the
> > flip side of
> > this compression is that you get several memory accesses for each
> > single
> > pdx-(to/from)-mfn conversion. And we do a lot of those. One possible
> > solution
> > would be to alt-patch the values in the code-stream and avoid the
> > perf-hit, but
> > that's not merged. Jan had some patches but that didn't make it to
> > staging,
> > IIRC.
> Could you please give me some links in the mailing list with mentioned
> patches?
>
> ~ Oleksii

Sure.

Much of this was discussed in the "Make PDX compression optional" series. This
link is v1, but there were 3 in total and a pre-patch documenting pdx.h
explaining what the technique actually does to make sure we were all on the
same page (pun intended) and the pdx-off case wouldn't break the world.

  https://lore.kernel.org/xen-devel/20230717160318.2113-1-alejandro.vallejo@cloud.com/

This was Jan's 2018 take to turn PDX into alternatives. He mentioned it
somewhere in those threads, but I can't find that message anymore.

  https://lore.kernel.org/xen-devel/5B76740802000078001DF345@prv1-mh.provo.novell.com/

Cheers,
Alejandro

[PATCH v1 3/5] xen/riscv: introduce setup_mm()

[Oleksii Kurochko]

Introduce the implementation of setup_mm(), which includes:
1. Adding all free regions to the boot allocator, as memory is needed
   to allocate page tables used for frame table mapping.
2. Calculating RAM size and the RAM end address.
3. Setting up direct map mappings from the PFN of physical address 0
   to the PFN of RAM end.
4. Setting up frame table mappings from physical address 0 to ram_end.
5. Setting up total_pages and max_page.
6. Establishing `directmap_virt_end`, which is not currently used but is
   intended for future use in virt_to_page() to ensure that virtual
   addresses do not exceed directmap_virt_end.

In the case of RISC-V 64, which has a large virtual address space
(the minimum supported MMU mode is Sv39, providing TB of VA space),
the direct map and frame table are mapped starting from physical address
0. This simplifies the calculations and thereby improves performance for
page_to_mfn(), mfn_to_page(), and maddr_to_virt(), as there is no
need to account for {directmap, frametable}_base_pdx.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/mm.h    |  2 +
 xen/arch/riscv/include/asm/setup.h |  2 +
 xen/arch/riscv/mm.c                | 93 ++++++++++++++++++++++++++++++
 xen/arch/riscv/setup.c             |  3 +
 4 files changed, 100 insertions(+)

diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 0396e66f47..7b472220e5 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -12,6 +12,8 @@
 
 #include <asm/page-bits.h>
 
+extern vaddr_t directmap_virt_end;
+
 #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
 #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
 
diff --git a/xen/arch/riscv/include/asm/setup.h b/xen/arch/riscv/include/asm/setup.h
index c0214a9bf2..844a2f0ef1 100644
--- a/xen/arch/riscv/include/asm/setup.h
+++ b/xen/arch/riscv/include/asm/setup.h
@@ -5,6 +5,8 @@
 
 #define max_init_domid (0)
 
+void setup_mm(void);
+
 #endif /* ASM__RISCV__SETUP_H */
 
 /*
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 27026d803b..53b7483f75 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -8,6 +8,7 @@
 #include <xen/libfdt/libfdt.h>
 #include <xen/macros.h>
 #include <xen/mm.h>
+#include <xen/pdx.h>
 #include <xen/pfn.h>
 #include <xen/sections.h>
 #include <xen/sizes.h>
@@ -423,3 +424,95 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
 
     return fdt_virt;
 }
+
+#ifndef CONFIG_RISCV_32
+/* Map the region in the directmap area. */
+static void __init setup_directmap_mappings(unsigned long nr_mfns)
+{
+    if ( nr_mfns > PFN_DOWN(DIRECTMAP_SIZE) )
+        panic("The directmap cannot cover the physical region %#"PRIpaddr" - %#"PRIpaddr"\n",
+              0UL, nr_mfns << PAGE_SHIFT);
+
+    if ( map_pages_to_xen((vaddr_t)mfn_to_virt(0),
+                          _mfn(0), nr_mfns,
+                          PAGE_HYPERVISOR_RW) )
+        panic("Unable to setup the directmap mappings.\n");
+}
+
+/* Map a frame table to cover physical addresses ps through pe */
+static void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
+{
+    unsigned long nr_mfns = mfn_x(mfn_add(maddr_to_mfn(pe), -1)) -
+                            mfn_x(maddr_to_mfn(ps)) + 1;
+    unsigned long frametable_size = nr_mfns * sizeof(struct page_info);
+    mfn_t base_mfn;
+
+    if ( frametable_size > FRAMETABLE_SIZE )
+        panic("The frametable cannot cover the physical region %#"PRIpaddr" - %#"PRIpaddr"\n",
+              ps, pe);
+
+    frametable_size = ROUNDUP(frametable_size, MB(2));
+    base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 2<<(20-12));
+
+    if ( map_pages_to_xen(FRAMETABLE_VIRT_START, base_mfn,
+                          frametable_size >> PAGE_SHIFT,
+                          PAGE_HYPERVISOR_RW) )
+        panic("Unable to setup the frametable mappings.\n");
+
+    memset(&frame_table[0], 0, nr_mfns * sizeof(struct page_info));
+    memset(&frame_table[nr_mfns], -1,
+           frametable_size - (nr_mfns * sizeof(struct page_info)));
+}
+
+vaddr_t directmap_virt_end __read_mostly;
+
+/*
+ * Setup memory management
+ *
+ * RISC-V 64 has a large virtual address space (the minimum supported
+ * MMU mode is Sv39, which provides TBs of VA space).
+ * In the case of RISC-V 64, the directmap and frametable are mapped
+ * starting from physical address 0 to simplify the page_to_mfn(),
+ * mfn_to_page(), and maddr_to_virt() calculations, as there is no need
+ * to account for {directmap, frametable}_base_pdx in this setup.
+ */
+void __init setup_mm(void)
+{
+    const struct membanks *banks = bootinfo_get_mem();
+    paddr_t ram_end = 0;
+    paddr_t ram_size = 0;
+    unsigned int i;
+
+    /*
+     * We need some memory to allocate the page-tables used for the directmap
+     * mappings. But some regions may contain memory already allocated
+     * for other uses (e.g. modules, reserved-memory...).
+     *
+     * For simplicity, add all the free regions in the boot allocator.
+     */
+    populate_boot_allocator();
+
+    total_pages = 0;
+
+    for ( i = 0; i < banks->nr_banks; i++ )
+    {
+        const struct membank *bank = &banks->bank[i];
+        paddr_t bank_end = bank->start + bank->size;
+
+        ram_size = ram_size + bank->size;
+        ram_end = max(ram_end, bank_end);
+    }
+
+    setup_directmap_mappings(PFN_DOWN(ram_end));
+
+    total_pages = PFN_DOWN(ram_size);
+
+    directmap_virt_end = DIRECTMAP_VIRT_START + ram_end;
+
+    setup_frametable_mappings(0, ram_end);
+    max_page = PFN_DOWN(ram_end);
+}
+
+#else /* CONFIG_RISCV_32 */
+#error setup_mm(), setup_{directmap,frametable}_mapping() should be implemented for RV_32
+#endif
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index e29bd75d7c..2887a18c0c 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -12,6 +12,7 @@
 
 #include <asm/early_printk.h>
 #include <asm/sbi.h>
+#include <asm/setup.h>
 #include <asm/smp.h>
 #include <asm/traps.h>
 
@@ -59,6 +60,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
     printk("Command line: %s\n", cmdline);
     cmdline_parse(cmdline);
 
+    setup_mm();
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.47.0

Re: [PATCH v1 3/5] xen/riscv: introduce setup_mm()

[Jan Beulich]

On 16.10.2024 11:15, Oleksii Kurochko wrote:
> @@ -423,3 +424,95 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>  
>      return fdt_virt;
>  }
> +
> +#ifndef CONFIG_RISCV_32
> +/* Map the region in the directmap area. */
> +static void __init setup_directmap_mappings(unsigned long nr_mfns)
> +{
> +    if ( nr_mfns > PFN_DOWN(DIRECTMAP_SIZE) )
> +        panic("The directmap cannot cover the physical region %#"PRIpaddr" - %#"PRIpaddr"\n",
> +              0UL, nr_mfns << PAGE_SHIFT);

Here and elsewhere can you please use mathematical range expressions
(apparently "[%"PRIpaddr", %#"PRIpaddr")" here), to avoid ambiguity?

> +    if ( map_pages_to_xen((vaddr_t)mfn_to_virt(0),
> +                          _mfn(0), nr_mfns,
> +                          PAGE_HYPERVISOR_RW) )
> +        panic("Unable to setup the directmap mappings.\n");
> +}
> +
> +/* Map a frame table to cover physical addresses ps through pe */
> +static void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
> +{
> +    unsigned long nr_mfns = mfn_x(mfn_add(maddr_to_mfn(pe), -1)) -

This looks to be accounting for a partial page at the end.

> +                            mfn_x(maddr_to_mfn(ps)) + 1;

Whereas this doesn't do the same at the start. The sole present caller
passes 0, so that's going to be fine for the time being. Yet it's a
latent pitfall. I'd recommend to either drop the function parameter, or
to deal with it correctly right away.

> +    unsigned long frametable_size = nr_mfns * sizeof(struct page_info);
> +    mfn_t base_mfn;
> +
> +    if ( frametable_size > FRAMETABLE_SIZE )
> +        panic("The frametable cannot cover the physical region %#"PRIpaddr" - %#"PRIpaddr"\n",
> +              ps, pe);
> +
> +    frametable_size = ROUNDUP(frametable_size, MB(2));
> +    base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 2<<(20-12));

Nit: Missing blanks in binary expressions. Also please don't open-code
PFN_DOWN() (and again below).

> +    if ( map_pages_to_xen(FRAMETABLE_VIRT_START, base_mfn,
> +                          frametable_size >> PAGE_SHIFT,
> +                          PAGE_HYPERVISOR_RW) )
> +        panic("Unable to setup the frametable mappings.\n");

Nit (as indicated before): No full stop please at the end of log or
panic messages.

> +    memset(&frame_table[0], 0, nr_mfns * sizeof(struct page_info));
> +    memset(&frame_table[nr_mfns], -1,
> +           frametable_size - (nr_mfns * sizeof(struct page_info)));
> +}
> +
> +vaddr_t directmap_virt_end __read_mostly;

__ro_after_init? And moved ahead of the identifier, just like ...

> +/*
> + * Setup memory management
> + *
> + * RISC-V 64 has a large virtual address space (the minimum supported
> + * MMU mode is Sv39, which provides TBs of VA space).
> + * In the case of RISC-V 64, the directmap and frametable are mapped
> + * starting from physical address 0 to simplify the page_to_mfn(),
> + * mfn_to_page(), and maddr_to_virt() calculations, as there is no need
> + * to account for {directmap, frametable}_base_pdx in this setup.
> + */
> +void __init setup_mm(void)

... __init is placed e.g. here.

It's also unclear why the variable needs to be non-static.

> +{
> +    const struct membanks *banks = bootinfo_get_mem();
> +    paddr_t ram_end = 0;
> +    paddr_t ram_size = 0;
> +    unsigned int i;
> +
> +    /*
> +     * We need some memory to allocate the page-tables used for the directmap
> +     * mappings. But some regions may contain memory already allocated
> +     * for other uses (e.g. modules, reserved-memory...).
> +     *
> +     * For simplicity, add all the free regions in the boot allocator.
> +     */
> +    populate_boot_allocator();
> +
> +    total_pages = 0;
> +
> +    for ( i = 0; i < banks->nr_banks; i++ )
> +    {
> +        const struct membank *bank = &banks->bank[i];
> +        paddr_t bank_end = bank->start + bank->size;
> +
> +        ram_size = ram_size + bank->size;
> +        ram_end = max(ram_end, bank_end);
> +    }
> +
> +    setup_directmap_mappings(PFN_DOWN(ram_end));

While you may want to use non-offset-ed mappings, I can't see how you can
validly map just a single huge chunk, no matter whether there are holes
in there. Such holes could hold MMIO regions, which I'm sure would require
more careful mapping (to avoid cacheable accesses, or even speculative
ones).

> +    total_pages = PFN_DOWN(ram_size);

Imo the rounding down to page granularity needs to be done when ram_size
is accumulated, such that partial pages simply won't be counted. Unless
of course there's a guarantee that banks can never have partial pages at
their start/end.

Jan

Re: [PATCH v1 3/5] xen/riscv: introduce setup_mm()

[oleksii.kurochko]

On Thu, 2024-10-17 at 17:15 +0200, Jan Beulich wrote:
> On 16.10.2024 11:15, Oleksii Kurochko wrote:
> 
> > +    if ( map_pages_to_xen((vaddr_t)mfn_to_virt(0),
> > +                          _mfn(0), nr_mfns,
> > +                          PAGE_HYPERVISOR_RW) )
> > +        panic("Unable to setup the directmap mappings.\n");
> > +}
> > +
> > +/* Map a frame table to cover physical addresses ps through pe */
> > +static void __init setup_frametable_mappings(paddr_t ps, paddr_t
> > pe)
> > +{
> > +    unsigned long nr_mfns = mfn_x(mfn_add(maddr_to_mfn(pe), -1)) -
> 
> This looks to be accounting for a partial page at the end.
> 
> > +                            mfn_x(maddr_to_mfn(ps)) + 1;
> 
> Whereas this doesn't do the same at the start. The sole present
> caller
> passes 0, so that's going to be fine for the time being. Yet it's a
> latent pitfall. I'd recommend to either drop the function parameter,
> or
> to deal with it correctly right away.
Then it will be better to do in the next way to be sure that everything
is properly aligned:
   unsigned long nr_mfns = PFN_DOWN(ALIGN_UP(pe) - ALIGN_DOWN(ps));

> > +    memset(&frame_table[0], 0, nr_mfns * sizeof(struct
> > page_info));
> > +    memset(&frame_table[nr_mfns], -1,
> > +           frametable_size - (nr_mfns * sizeof(struct
> > page_info)));
> > +}
> > +
> > +vaddr_t directmap_virt_end __read_mostly;
> 
> __ro_after_init? And moved ahead of the identifier, just like ...
> 
> > +/*
> > + * Setup memory management
> > + *
> > + * RISC-V 64 has a large virtual address space (the minimum
> > supported
> > + * MMU mode is Sv39, which provides TBs of VA space).
> > + * In the case of RISC-V 64, the directmap and frametable are
> > mapped
> > + * starting from physical address 0 to simplify the page_to_mfn(),
> > + * mfn_to_page(), and maddr_to_virt() calculations, as there is no
> > need
> > + * to account for {directmap, frametable}_base_pdx in this setup.
> > + */
> > +void __init setup_mm(void)
> 
> ... __init is placed e.g. here.
> 
> It's also unclear why the variable needs to be non-static.
As it could be use then for some ASSERT(), for example, in
virt_to_page() as Arm or x86 do.

> 
> > +{
> > +    const struct membanks *banks = bootinfo_get_mem();
> > +    paddr_t ram_end = 0;
> > +    paddr_t ram_size = 0;
> > +    unsigned int i;
> > +
> > +    /*
> > +     * We need some memory to allocate the page-tables used for
> > the directmap
> > +     * mappings. But some regions may contain memory already
> > allocated
> > +     * for other uses (e.g. modules, reserved-memory...).
> > +     *
> > +     * For simplicity, add all the free regions in the boot
> > allocator.
> > +     */
> > +    populate_boot_allocator();
> > +
> > +    total_pages = 0;
> > +
> > +    for ( i = 0; i < banks->nr_banks; i++ )
> > +    {
> > +        const struct membank *bank = &banks->bank[i];
> > +        paddr_t bank_end = bank->start + bank->size;
> > +
> > +        ram_size = ram_size + bank->size;
> > +        ram_end = max(ram_end, bank_end);
> > +    }
> > +
> > +    setup_directmap_mappings(PFN_DOWN(ram_end));
> 
> While you may want to use non-offset-ed mappings, I can't see how you
> can
> validly map just a single huge chunk, no matter whether there are
> holes
> in there. Such holes could hold MMIO regions, which I'm sure would
> require
> more careful mapping (to avoid cacheable accesses, or even
> speculative
> ones).
My intention was to avoid subraction of directmap_start ( = ram_start )
in maddr_to_virt() to have less  operations during maddr to virt
calculation:
   static inline void *maddr_to_virt(paddr_t ma)
   {
       /* Offset in the direct map, accounting for pdx compression */
       unsigned long va_offset = maddr_to_directmapoff(ma);
   
       ASSERT(va_offset < DIRECTMAP_SIZE);
   
       return (void *)(DIRECTMAP_VIRT_START /* - directmap_start */ +
   va_offset);
   }
But it seems I don't have any chance to avoid that because of mentioned
by you reasons... Except probably to have a config which will hard code
RAM_START for each platform ( what on other hand will make Xen less
flexible in some point ).
Does it make sense to have a config instead of identifying ram_start in
runtime?

So I have to rework this part of code to look as:
    for ( i = 0; i < banks->nr_banks; i++ )
    {
        const struct membank *bank = &banks->bank[i];
        paddr_t bank_end = bank->start + bank->size;

        ram_size = ram_size + bank->size;
        ram_start = min(ram_start, bank->start);
        ram_end = max(ram_end, bank_end);

        setup_directmap_mappings(PFN_DOWN(bank->start),
                                 PFN_DOWN(bank->size));
    }


> 
> > +    total_pages = PFN_DOWN(ram_size);
> 
> Imo the rounding down to page granularity needs to be done when
> ram_size
> is accumulated, such that partial pages simply won't be counted.
> Unless
> of course there's a guarantee that banks can never have partial pages
> at
> their start/end.
Hmm, good point. I thought that start/end is always properly aligned
but I can't find in device tree spec that it is guaranteed for memory
node, so it will really better rounding down after ram_size is
accumulated.

~ Oleksii

Re: [PATCH v1 3/5] xen/riscv: introduce setup_mm()

[Jan Beulich]

On 18.10.2024 16:27, oleksii.kurochko@gmail.com wrote:
> On Thu, 2024-10-17 at 17:15 +0200, Jan Beulich wrote:
>> On 16.10.2024 11:15, Oleksii Kurochko wrote:
>>> +vaddr_t directmap_virt_end __read_mostly;
>>
>> __ro_after_init? And moved ahead of the identifier, just like ...
>>
>>> +/*
>>> + * Setup memory management
>>> + *
>>> + * RISC-V 64 has a large virtual address space (the minimum
>>> supported
>>> + * MMU mode is Sv39, which provides TBs of VA space).
>>> + * In the case of RISC-V 64, the directmap and frametable are
>>> mapped
>>> + * starting from physical address 0 to simplify the page_to_mfn(),
>>> + * mfn_to_page(), and maddr_to_virt() calculations, as there is no
>>> need
>>> + * to account for {directmap, frametable}_base_pdx in this setup.
>>> + */
>>> +void __init setup_mm(void)
>>
>> ... __init is placed e.g. here.
>>
>> It's also unclear why the variable needs to be non-static.
> As it could be use then for some ASSERT(), for example, in
> virt_to_page() as Arm or x86 do.

"Could be" is too little here. Misra dislikes identifiers which could
be static, but aren't. If it's not used right now (nor any time soon),
it should imo be static until a use appears requiring it to be globally
visible.

>>> +{
>>> +    const struct membanks *banks = bootinfo_get_mem();
>>> +    paddr_t ram_end = 0;
>>> +    paddr_t ram_size = 0;
>>> +    unsigned int i;
>>> +
>>> +    /*
>>> +     * We need some memory to allocate the page-tables used for
>>> the directmap
>>> +     * mappings. But some regions may contain memory already
>>> allocated
>>> +     * for other uses (e.g. modules, reserved-memory...).
>>> +     *
>>> +     * For simplicity, add all the free regions in the boot
>>> allocator.
>>> +     */
>>> +    populate_boot_allocator();
>>> +
>>> +    total_pages = 0;
>>> +
>>> +    for ( i = 0; i < banks->nr_banks; i++ )
>>> +    {
>>> +        const struct membank *bank = &banks->bank[i];
>>> +        paddr_t bank_end = bank->start + bank->size;
>>> +
>>> +        ram_size = ram_size + bank->size;
>>> +        ram_end = max(ram_end, bank_end);
>>> +    }
>>> +
>>> +    setup_directmap_mappings(PFN_DOWN(ram_end));
>>
>> While you may want to use non-offset-ed mappings, I can't see how you
>> can
>> validly map just a single huge chunk, no matter whether there are
>> holes
>> in there. Such holes could hold MMIO regions, which I'm sure would
>> require
>> more careful mapping (to avoid cacheable accesses, or even
>> speculative
>> ones).
> My intention was to avoid subraction of directmap_start ( = ram_start )
> in maddr_to_virt() to have less  operations during maddr to virt
> calculation:
>    static inline void *maddr_to_virt(paddr_t ma)
>    {
>        /* Offset in the direct map, accounting for pdx compression */
>        unsigned long va_offset = maddr_to_directmapoff(ma);
>    
>        ASSERT(va_offset < DIRECTMAP_SIZE);
>    
>        return (void *)(DIRECTMAP_VIRT_START /* - directmap_start */ +
>    va_offset);
>    }
> But it seems I don't have any chance to avoid that because of mentioned
> by you reasons... Except probably to have a config which will hard code
> RAM_START for each platform ( what on other hand will make Xen less
> flexible in some point ).
> Does it make sense to have a config instead of identifying ram_start in
> runtime?

I don't think building Xen for just one (kind of) platform is going to be
overly useful (except maybe for development purposes, yet the goal ought
to be to be flexible, and hence it may be a bad idea even while developing
new code).

Jan

[PATCH v1 4/5] xen/riscv: initialize the VMAP_DEFAULT virtual range

[Oleksii Kurochko]

Call vm_init() to initialize the VMAP_DEFAULT virtual range.

To support this, introduce the populate_pt_range() and
arch_vmap_virt_end() functions, which are used by
vm_init()->vm_init_type().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/mm.c    | 11 +++++------
 xen/arch/riscv/pt.c    |  6 ++++++
 xen/arch/riscv/setup.c |  3 +++
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 53b7483f75..2a55246f80 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -352,12 +352,6 @@ void arch_dump_shared_mem_info(void)
     BUG_ON("unimplemented");
 }
 
-int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
-{
-    BUG_ON("unimplemented");
-    return -1;
-}
-
 int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
                               union add_to_physmap_extra extra,
                               unsigned long idx, gfn_t gfn)
@@ -516,3 +510,8 @@ void __init setup_mm(void)
 #else /* CONFIG_RISCV_32 */
 #error setup_mm(), setup_{directmap,frametable}_mapping() should be implemented for RV_32
 #endif
+
+void *__init arch_vmap_virt_end(void)
+{
+    return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
+}
diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c
index cc5e2d3266..d62aceb36c 100644
--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -1,6 +1,7 @@
 #include <xen/bug.h>
 #include <xen/domain_page.h>
 #include <xen/errno.h>
+#include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/pfn.h>
@@ -419,3 +420,8 @@ int map_pages_to_xen(unsigned long virt,
 
     return pt_update(virt, mfn, nr_mfns, flags);
 }
+
+int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns)
+{
+    return pt_update(virt, INVALID_MFN, nr_mfns, PTE_POPULATE);
+}
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 2887a18c0c..3652cb056d 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -7,6 +7,7 @@
 #include <xen/init.h>
 #include <xen/mm.h>
 #include <xen/shutdown.h>
+#include <xen/vmap.h>
 
 #include <public/version.h>
 
@@ -62,6 +63,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     setup_mm();
 
+    vm_init();
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.47.0

Re: [PATCH v1 4/5] xen/riscv: initialize the VMAP_DEFAULT virtual range

[Jan Beulich]

On 16.10.2024 11:15, Oleksii Kurochko wrote:
> Call vm_init() to initialize the VMAP_DEFAULT virtual range.
> 
> To support this, introduce the populate_pt_range() and
> arch_vmap_virt_end() functions, which are used by
> vm_init()->vm_init_type().
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[PATCH v1 5/5] xen/riscv: finalize boot allocator and transition to boot state

[Oleksii Kurochko]

Add a call to end_boot_allocator() in start_xen() to finalize the
boot memory allocator, moving free pages to the domain sub-allocator.

After initializing the memory subsystem, update `system_state` from
`SYS_STATE_early_boot` to `SYS_STATE_boot`, signifying the end of the
early boot phase.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/setup.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 3652cb056d..9680332fee 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -65,6 +65,14 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     vm_init();
 
+    end_boot_allocator();
+
+    /*
+     * The memory subsystem has been initialized, we can now switch from
+     * early_boot -> boot.
+     */
+    system_state = SYS_STATE_boot;
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.47.0

Re: [PATCH v1 5/5] xen/riscv: finalize boot allocator and transition to boot state

[Jan Beulich]

On 16.10.2024 11:15, Oleksii Kurochko wrote:
> Add a call to end_boot_allocator() in start_xen() to finalize the
> boot memory allocator, moving free pages to the domain sub-allocator.
> 
> After initializing the memory subsystem, update `system_state` from
> `SYS_STATE_early_boot` to `SYS_STATE_boot`, signifying the end of the
> early boot phase.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>