Re: [PATCH v12 5/8] xen/riscv: add minimal stuff to mm.h to build full Xen

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-05-30 at 18:23 +0100, Andrew Cooper wrote:
> On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> This patch looks like it can go in independently?  Or does it depend
> on
> having bitops.h working in practice?
> 
> However, one very strong suggestion...
> 
> 
> > diff --git a/xen/arch/riscv/include/asm/mm.h
> > b/xen/arch/riscv/include/asm/mm.h
> > index 07c7a0abba..cc4a07a71c 100644
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -3,11 +3,246 @@
> > <snip>
> > +/* PDX of the first page in the frame table. */
> > +extern unsigned long frametable_base_pdx;
> > +
> > +/* Convert between machine frame numbers and page-info structures.
> > */
> > +#define
> > mfn_to_page(mfn)                                            \
> > +    (frame_table + (mfn_to_pdx(mfn) - frametable_base_pdx))
> > +#define
> > page_to_mfn(pg)                                             \
> > +    pdx_to_mfn((unsigned long)((pg) - frame_table) +
> > frametable_base_pdx)
> 
> Do yourself a favour and not introduce frametable_base_pdx to begin
> with.

To drop frametable_base_pdx if the following changes will be enough:
   diff --git a/xen/arch/riscv/include/asm/mm.h
   b/xen/arch/riscv/include/asm/mm.h
   index cc4a07a71c..fdac7e0646 100644
   --- a/xen/arch/riscv/include/asm/mm.h
   +++ b/xen/arch/riscv/include/asm/mm.h
   @@ -107,14 +107,11 @@ struct page_info
    
    #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
    
   -/* PDX of the first page in the frame table. */
   -extern unsigned long frametable_base_pdx;
   -
    /* Convert between machine frame numbers and page-info structures. */
    #define mfn_to_page(mfn)                                            \
   -    (frame_table + (mfn_to_pdx(mfn) - frametable_base_pdx))
   +    (frame_table + mfn - FRAMETABLE_BASE_OFFSET))
    #define page_to_mfn(pg)                                             \
   -    pdx_to_mfn((unsigned long)((pg) - frame_table) +
   frametable_base_pdx)
   +    ((unsigned long)((pg) - frame_table) + FRAMETABLE_BASE_OFFSET)
    
    static inline void *page_to_virt(const struct page_info *pg)
    {
   diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
   index 9c0fd80588..8f6dbdc699 100644
   --- a/xen/arch/riscv/mm.c
   +++ b/xen/arch/riscv/mm.c
   @@ -15,7 +15,7 @@
    #include <asm/page.h>
    #include <asm/processor.h>
    
   -unsigned long __ro_after_init frametable_base_pdx;
    unsigned long __ro_after_init frametable_virt_end;
    
    struct mmu_desc {


> 
> Every RISC-V board I can find has things starting from 0 in physical
> address space, with RAM starting immediately after.
> 
> Taking the microchip board as an example, RAM actually starts at
> 0x8000000, which means that having frametable_base_pdx and assuming
> it
> does get set to 0x8000 (which isn't even a certainty, given that I
> think
> you'll need struct pages covering the PLICs), then what you are
> trading
> off is:
> 
> * Saving 32k of virtual address space only (no need to even allocate
> memory for this range of the framtable), by
> * Having an extra memory load and add/sub in every page <-> mfn
> conversion, which is a screaming hotpath all over Xen.
Are you referring here to `mfn_to_pdx()` used in `mfn_to_page()` and
`pdx_to_mfn()` in `page_to_mfn()`?

My expectation was that when CONFIG_PDX_COMPRESSION is disabled then
this macros doesn't do anything:
/* pdx<->pfn == identity */
#define pdx_to_pfn(x) (x)
#define pfn_to_pdx(x) (x)


> 
> It's a terribly short-sighted tradeoff.
> 
> 32k of VA space might be worth saving in a 32bit build (I personally
> wouldn't - especially as there's no need to share Xen's VA space with
> guests, given no PV guests on ARM/RISC-V), but it's absolutely not at
> all in an a 64bit build with TB of VA space available.
Why 32k? If RAM is started at 0x8000_0000 then we have to cover 0x80000
entries, and the size of it is 0x8_0000 = 524288 * 64 = 33554432, so it
is 32 Mb.
Am I confusing something?

P.S.: Should I map this start 32 mb? Or it is enough to have a slide (
FRAMETABLE_BASE_OFFSET ).

~ Oleksii

> 
> Even if we do find a board with the first interesting thing in the
> frametable starting sufficiently away from 0 that it might be worth
> considering this slide, then it should still be Kconfig-able in a
> similar way to PDX_COMPRESSION.
> 
> You don't want to be penalising everyone because of a
> theoretical/weird
> corner case.
> 
> ~Andrew