Re: [PATCH v8 1/5] xen/riscv: add VM space layout

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-05-22 at 14:47 +0200, Jan Beulich wrote:
> On 22.05.2023 14:18, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -4,6 +4,42 @@
> >  #include <xen/const.h>
> >  #include <xen/page-size.h>
> >  
> > +/*
> > + * RISC-V64 Layout:
> > + *
> > + * #ifdef RV_STAGE1_MODE == SATP_MODE_SV39
> 
> Nit: #if please, not #ifdef. Also may I stress again that ideally
> this
> would be formatted such that when e.g. grep-ing for SATP_MODE_SV39,
> the
> matching line here would _not_ give the impression of being "a
> comment
> only" (making people possibly pay less attention)? My referral to the
> x86 way of doing things remains.
Oh, I double checked the x86's config.h and understood ( my editor's
colour scheme draw it as a comment so I missed that there is no '*' at
the start if "#ifndef CONFIG_BIGMEM" line ) what you mean:
  '*' should be removed at the start of "#ifdef RV..." line.
> 
> > + * 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.
> > + *
> > + *
> > ===================================================================
> > =========
> > + *    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
> 
> And another, yet more minor nit: Would be nice if all addresses here
> were spelled uniformly, i.e. also with upper vs lower case of hex
> digit letter consistent.
Sure. I will make all upper case. Thanks.
> 
> > + *                 ...                  |  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
> > + *
> > ===================================================================
> > =========
> > + *
> > + * #endif
> > + */
> > +
> >  #if defined(CONFIG_RISCV_64)
> >  # define LONG_BYTEORDER 3
> >  # define ELFSIZE 64
> 

~ Oleksii