Re: [PATCH v3 27/34] xen/riscv: define an address of frame table

[Oleksii <oleksii.kurochko@gmail.com>]

On Tue, 2024-01-23 at 12:32 +0100, Jan Beulich wrote:
> On 22.12.2023 16:13, Oleksii Kurochko wrote:
> > Also, the patch adds some helpful macros that assist in avoiding
> > the redefinition of memory layout for each MMU mode.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V3:
> >  - drop OFFSET_BITS, and use PAGE_SHIFT instead.
> >  - code style fixes.
> >  - add comment how macros are useful.
> >  - move all memory related layout definitions close to comment with
> > memory layout description.
> >  - make memory layout description generic for any MMU mode.
> > ---
> > Changes in V2:
> >  - Nothing changed. Only rebase.
> > ---
> >  xen/arch/riscv/include/asm/config.h | 85 +++++++++++++++++++------
> > ----
> >  1 file changed, 55 insertions(+), 30 deletions(-)
> > 
> > diff --git a/xen/arch/riscv/include/asm/config.h
> > b/xen/arch/riscv/include/asm/config.h
> > index f0544c6a20..fb9fc9daaa 100644
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -6,6 +6,14 @@
> >  #include <xen/const.h>
> >  #include <xen/page-size.h>
> >  
> > +#ifdef CONFIG_RISCV_64
> > +#define CONFIG_PAGING_LEVELS 3
> > +#define RV_STAGE1_MODE SATP_MODE_SV39
> > +#else
> > +#define CONFIG_PAGING_LEVELS 2
> > +#define RV_STAGE1_MODE SATP_MODE_SV32
> > +#endif
> > +
> >  /*
> >   * RISC-V64 Layout:
> >   *
> > @@ -22,25 +30,56 @@
> >   *
> >   * It means that:
> >   *   top VA bits are simply ignored for the purpose of translating
> > to PA.
> > +#endif
> >   *
> > - *
> > ===================================================================
> > =========
> > - *    Start addr    |   End addr        |  Size  | Slot      
> > |area description
> > - *
> > ===================================================================
> > =========
> > - * FFFFFFFFC0800000 |  FFFFFFFFFFFFFFFF |1016 MB | L2 511     |
> > Unused
> > - * FFFFFFFFC0600000 |  FFFFFFFFC0800000 |  2 MB  | L2 511     |
> > Fixmap
> > - * FFFFFFFFC0200000 |  FFFFFFFFC0600000 |  4 MB  | L2 511     |
> > FDT
> > - * FFFFFFFFC0000000 |  FFFFFFFFC0200000 |  2 MB  | L2 511     |
> > Xen
> > - *                 ...                  |  1 GB  | L2 510     |
> > Unused
> > - * 0000003200000000 |  0000007F80000000 | 309 GB | L2 200-509 |
> > Direct map
> > - *                 ...                  |  1 GB  | L2 199     |
> > Unused
> > - * 0000003100000000 |  00000031C0000000 |  3 GB  | L2 196-198 |
> > Frametable
> > - *                 ...                  |  1 GB  | L2 195     |
> > Unused
> > - * 0000003080000000 |  00000030C0000000 |  1 GB  | L2 194     |
> > VMAP
> > - *                 ...                  | 194 GB | L2 0 - 193 |
> > Unused
> > - *
> > ===================================================================
> > =========
> > + *       SATP_MODE_SV32   | SATP_MODE_SV39   | SATP_MODE_SV48   |
> > SATP_MODE_SV57
> > + *     
> > ==================|==================|==================|==========
> > =======
> > + * BA0 | FFFFFFFFFFE00000 | FFFFFFFFC0000000 | FFFFFF8000000000 |
> > FFFF000000000000
> > + * BA1 | 0000000019000000 | 0000003200000000 | 0000640000000000 |
> > 00C8000000000000
> > + * BA2 | 0000000018800000 | 0000003100000000 | 0000620000000000 |
> > 00C4000000000000
> > + * BA3 | 0000000018400000 | 0000003080000000 | 0000610000000000 |
> > 00C2000000000000
> >   *
> > -#endif
> > + *
> > ===================================================================
> > ============
> > + * Start addr     |   End addr          |  Size  | Root PT slot |
> > Area description
> > + *
> > ===================================================================
> > ============
> > + * BA0 + 0x800000 |  FFFFFFFFFFFFFFFF   |1016 MB |     511      |
> > Unused
> > + * BA0 + 0x400000 |  BA0 + 0x800000     |  2 MB  |     511      |
> > Fixmap
> > + * BA0 + 0x200000 |  BA0 + 0x400000     |  4 MB  |     511      |
> > FDT
> > + * BA0            |  BA0 + 0x200000     |  2 MB  |     511      |
> > Xen
> > + *                 ...                  |  1 GB  |     510      |
> > Unused
> > + * BA1 + 0x000000 |  BA1 + 0x4D80000000 | 309 GB |   200-509    |
> > Direct map
> 
> This definitely can't be right for SV32. Others may be problematic,
> too, like ...
> 
> > + *                 ...                  |  1 GB  |     199      |
> > Unused
> > + * BA2 + 0x000000 |  BA2 + 0xC0000000   |  3 GB  |   196-198    |
> > Frametable
> 
> ... this one. Otoh I'd expect both to potentially be much larger in
> SV48 and SV57 modes.
Regarding Sv32, it looks to me the only BA0 and End addr at the first
line isn't correct as address size is 32.

Regarding other modes, yes, it should be changed Size column. Also, the
size of frame table should be recalculated.

Do we really need size column?

Wouldn't it be enough only have PT slot number?

Would it be better to have separate table for each mode?

Probably, it would be more useful to print memory map to Xen console.

> 
> > + *                 ...                  |  1 GB  |     195      |
> > Unused
> > + * BA3 + 0x000000 |  BA3 + 0x40000000   |  1 GB  |     194      |
> > VMAP
> > + *                 ...                  | 194 GB |   0 - 193    |
> > Unused
> > + *
> > ===================================================================
> > ============
> >   */
> > +#define VPN_BITS    (9)
> 
> This need to move ...
> 
> > +#define HYP_PT_ROOT_LEVEL (CONFIG_PAGING_LEVELS - 1)
> > +
> > +#ifdef CONFIG_RISCV_64
> 
> ... here, I think, for not being applicable to SV32?
You are right, it is not applicable for Sv32. In case of Sv32, it
should be 10.
But I am not sure that it is correct only to move this definition as
RISCV-64 can also use Sv32. So it looks like VPN_BITS should be "#ifdef
RV_STAGE1_MODE == Sv32".

> 
> > +#define SLOTN_ENTRY_BITS        (HYP_PT_ROOT_LEVEL * VPN_BITS +
> > PAGE_SHIFT)
> > +#define SLOTN(slot)             (_AT(vaddr_t, slot) <<
> > SLOTN_ENTRY_BITS)
> > +#define SLOTN_ENTRY_SIZE        SLOTN(1)
> 
> Do you have any example of how/where this going to be used?
Yes, it will be used to define DIRECTMAP_SIZE:
#define DIRECTMAP_SIZE          (SLOTN_ENTRY_SIZE * (509-200))
> 
> > +#define XEN_VIRT_START 0xFFFFFFFFC0000000 /* (_AC(-1, UL) + 1 -
> > GB(1)) */
> 
> Won't /* -GB(1) */ do, thus allowing the line to also be padded such
> that
> it matches neighboring ones in layout?
Could you please clarify what do you mean by padded here? The intention
was to show that 1 GB is used for Xen, FDT and fixmap.

~ Oleksii
> 
> > +#define FRAMETABLE_VIRT_START   SLOTN(196)
> > +#define FRAMETABLE_SIZE         GB(3)
> > +#define FRAMETABLE_NR           (FRAMETABLE_SIZE /
> > sizeof(*frame_table))
> > +#define FRAMETABLE_VIRT_END     (FRAMETABLE_VIRT_START +
> > FRAMETABLE_SIZE - 1)
> > +
> > +#define VMAP_VIRT_START         SLOTN(194)
> > +#define VMAP_VIRT_SIZE          GB(1)
> > [...]
>