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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-01-24 at 09:07 +0100, Jan Beulich wrote:
> On 23.01.2024 17:50, Oleksii wrote:
> > On Tue, 2024-01-23 at 12:32 +0100, Jan Beulich wrote:
> > > On 22.12.2023 16:13, Oleksii Kurochko wrote:
> > > > @@ -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?
> 
> Perhaps.
> 
> > Would it be better to have separate table for each mode?
> 
> Don't know.
Then I'll play around different ways to display memory layout.

> 
> > > > +#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".
> 
> Can it? The spec talks of SXLEN=32 implying SV32, while SXLEN=64
> permits
> SV39, SV48, and SV57. No mention of SV32 there.
According to spec it can't, but when I tried that in baremetal it
worked.

Let's stick to the spec, and then it would be better to move VPN_BITS
to #ifdef CONFIG_RISCV_64.

> 
> > > > +#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))
> 
> How about
> 
> #define DIRECTMAP_SIZE          (SLOTN(509) - SLOTN(200))
> 
> instead?
It would be better, I'll drop SLOTN_ENTRY_SIZE then. Thanks.

> 
> > > > +#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.
> 
> I'm talking of blank padding in the source file. Note how preceding
> and
> following #define-s blank-pad expansions so they all align. Just this
> one in the middle does not.
I see what you mean now. Thanks.

~ 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)
> > > > [...]
> > > 
> > 
>