Re: [PATCH v2 30/39] xen/riscv: define an address of frame table

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-12-18 at 12:22 +0100, Jan Beulich wrote:
> On 18.12.2023 11:36, Oleksii wrote:
> > On Thu, 2023-12-14 at 16:48 +0100, Jan Beulich wrote:
> > > On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > > > +#define SLOTN_ENTRY_SIZE        SLOTN(1)
> > > > +
> > > >  #define XEN_VIRT_START 0xFFFFFFFFC0000000 /* (_AC(-1, UL) + 1
> > > > -
> > > > GB(1)) */
> > > > +
> > > > +#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)
> > > 
> > > May I suggest that you keep these blocks sorted by slot number?
> > > Or
> > > wait,
> > > the layout comment further up is also in decreasing order, so
> > > that's
> > > fine here, but then can all of this please be moved next to the
> > > comment
> > > actually providing the necessary context (thus eliminating the
> > > need
> > > for
> > > new comments)?
> > Sure, I'll put this part close to layout comment.
> > 
> > >  You'll then also notice that the generalization here
> > > (keeping basically the same layout for e.g. SATP_MODE_SV48, just
> > > shifted
> > > by 9 bits) isn't in line with the comment there.
> > Does it make sense to add another one table with updated addresses
> > for
> > SATP_MODE_SV48?
> 
> Well, especially if you mean to support that mode, its layout surely
> wants writing down. I was hoping though that maybe you/we could get
> away
> without multiple tables, but e.g. use one having multiple columns.
I came up with the following but I am not sure that it is really
convient:
/*
 * RISC-V64 Layout:
 *
#if RV_STAGE1_MODE == SATP_MODE_SV39
 *
 * From the riscv-privileged doc:
 *   When mapping between narrower and wider addresses,
 *   RISC-V zero-extends a narrower physical address to a wider size.
 *   The mapping between 64-bit virtual addresses and the 39-bit usable
 *   address space of Sv39 is not based on zero-extension but instead
 *   follows an entrenched convention that allows an OS to use one or
 *   a few of the most-significant bits of a full-size (64-bit) virtual
 *   address to quickly distinguish user and supervisor address
regions.
 *
 * It means that:
 *   top VA bits are simply ignored for the purpose of translating to
PA.
#endif
 *
 *       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
 * 
 *
=======================================================================
=====
 * Start addr    |   End addr           |  Size  | Slot       |area
description
 *
=======================================================================
=====
 * BA0 + 0x800000 |  FFFFFFFFFFFFFFFF   |1016 MB |
L${HYP_PT_ROOT_LEVEL} 511     | Unused
 * BA0 + 0x400000 |  BA0 + 0x800000     |  2 MB  |
L${HYP_PT_ROOT_LEVEL} 511     | Fixmap
 * BA0 + 0x200000 |  BA0 + 0x400000     |  4 MB  |
L${HYP_PT_ROOT_LEVEL} 511     | FDT
 * BA0            |  BA0 + 0x200000     |  2 MB  |
L${HYP_PT_ROOT_LEVEL} 511     | Xen
 *                 ...                  |  1 GB  |
L${HYP_PT_ROOT_LEVEL} 510     | Unused
 * BA1 + 0x000000 |  BA1 + 0x4D80000000 | 309 GB |
L${HYP_PT_ROOT_LEVEL} 200-509 | Direct map
 *                 ...                  |  1 GB  |
L${HYP_PT_ROOT_LEVEL} 199     | Unused
 * BA2 + 0x000000 |  BA2 + 0xC0000000   |  3 GB  |
L${HYP_PT_ROOT_LEVEL} 196-198 | Frametable
 *                 ...                  |  1 GB  |
L${HYP_PT_ROOT_LEVEL} 195     | Unused
 * BA3 + 0x000000 |  BA3 + 0x40000000   |  1 GB  |
L${HYP_PT_ROOT_LEVEL} 194     | VMAP
 *                 ...                  | 194 GB |
L${HYP_PT_ROOT_LEVEL} 0 - 193 | Unused
 *
=======================================================================
=====
 */

Do you have better ideas?

Thanks in advamce.

~ Oleksii
> 
> 
> > Microchip has h/w which requires SATP_MODE_SV48 ( at least ), so I
> > have
> > a patch which introduces SATP_MODE_SV48 and I planned to update the
> > layout table in this patch.
> 
> 
>