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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2023-12-22 at 09:08 +0100, Jan Beulich wrote:
> On 21.12.2023 20:59, Oleksii wrote:
> > 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?
> 
> It doesn't look too bad imo, at the first glance, albeit the line
> wrapping damage of course makes it a little hard to look at. In the
> last table with all lines saying L${HYP_PT_ROOT_LEVEL}, perhaps that
> could be put in the table heading (instead of "Slot" say e.g. "Root
> PT slot")?
Thanks for the remark. It would be definitely better.

~ Oleksii