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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-12-14 at 16:48 +0100, Jan Beulich wrote:
> On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > Also the patchs adds some helpful macros.
> 
> In how far they're (going to be) helpful is hard to tell without uses
> and without some suitable comments.
> 
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -77,12 +77,31 @@
> >    name:
> >  #endif
> >  
> > +#define VPN_BITS    (9)
> > +#define OFFSET_BITS (12)
> 
> Whose offset? In how far is this different from PAGE_SHIFT?
It is page offset ( RISC-V terminology names it so ) and it is not
different with PAGE_SHIFT. OFFSET_BITS can be dropped.
> 
> >  #ifdef CONFIG_RISCV_64
> > +
> > +#define SLOTN_ENTRY_BITS        (HYP_PT_ROOT_LEVEL * VPN_BITS +
> > OFFSET_BITS)
> > +#define SLOTN(slot)             (_AT(vaddr_t,slot) <<
> > SLOTN_ENTRY_BITS)
> 
> Nit: Missing blank after comma.
Thanks. I'll update that.

> 
> > +#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?
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.

> 
> > @@ -95,6 +114,8 @@
> >  #define RV_STAGE1_MODE SATP_MODE_SV32
> >  #endif
> >  
> > +#define HYP_PT_ROOT_LEVEL (CONFIG_PAGING_LEVELS - 1)
> 
> I understand that CONFIG_PAGING_LEVELS is defined only just up from
> here,
> but what that identifier stands for is quite clear. It would seem to
> me
> that moving this up ahead if its first use would help clarity.
Thanks. It can be useful, so I'll move it up.

~ Oleksii