Re: [PATCH v5 1/4] xen/riscv: add VM space layout

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien, 
On Mon, 2023-04-24 at 12:08 +0100, Julien Grall wrote:
> > > > > > On 19.04.2023 17:42, Oleksii Kurochko wrote:
> > > > > > > + *
> > > > > > > =========================================================
> > > > > > > ==========
> > > > > > > =========
> > > > > > > + *    Start addr    |   End addr        |  Size  | VM
> > > > > > > area
> > > > > > > description
> > > > > > > + *
> > > > > > > =========================================================
> > > > > > > ==========
> > > > > > > =========
> > > > > > > + * FFFFFFFFC0000000 |  FFFFFFFFC0200000 |  2 MB  | Xen
> > > > > > > + * FFFFFFFFC0200000 |  FFFFFFFFC0600000 |  4 MB  | FDT
> > > > > > > + * FFFFFFFFC0600000 |  FFFFFFFFC0800000 |  2 MB  |
> > > > > > > Fixmap
> > > > > > 
> > > > > > These are all L2 slot 511 aiui, which may be worth
> > > > > > mentioning
> > > > > > especially since
> > > > > > the top bits don't match the top bits further down in the
> > > > > > table
> > > > > > (because of the
> > > > > > aliasing).
> > > > > 
> > > > > Than I'll add one more column where I'll put slot number
> > > > > 
> > > > > > 
> > > > > > > + *     .................. unused ..................
> > > > > > 
> > > > > > This is covering slot 510, which again may be worth
> > > > > > mentioning.
> > > > > > 
> > > > > > > + * 0000003200000000 |  0000007f40000000 | 331 GB |
> > > > > > > Direct map(L2
> > > > > > > slot: 200-509)
> > > > > > > + * 0000003100000000 |  0000003140000000 |  1 GB  |
> > > > > > > Frametable(L2
> > > > > > > slot: 196-197)
> > > > > > 
> > > > > > 1Gb is, if I'm not mistaken, a single L2 slot.
> > > > > Yeah, it can be misunderstood. I meant [196, 197), so 197
> > > > > isn't
> > > > > included. I'll update the table.
> > > > > 
> > > > > > 
> > > > > > Also assuming a 32-byte struct page_info (I don't think
> > > > > > you'll get
> > > > > > away with
> > > > > > less than that, when even Arm32 requires this much),
> > > > > > there's a
> > > > > > mismatch
> > > > > > between direct map and frame table size: With 4k pages, the
> > > > > > scaling
> > > > > > factor
> > > > > > would be 128 if I'm not mistaken. So perhaps you really
> > > > > > mean 3Gb here
> > > > > > to
> > > > > > cover for (slightly more than) the 331Gb of memory you mean
> > > > > > to be
> > > > > > able to
> > > > > > map?
> > > > > For RV64 page_info size will be 56-byte and 32-byte for RV32
> > > > > but you
> > > > > are right it should 3 Gb in that case what will be enough (
> > > > > taking into
> > > > > account both available sizes of page_info structure ).
> > > > 
> > > > As to the plan to it being 56 bytes (i.e. like on Arm): Arm
> > > > forever has
> > > > had a 64-bit padding field at the end. My best guess is that
> > > > the field
> > > > was introduced to have a 32-byte struct on Arm32.
> > > 
> > > I can't exactly remember. But I would like to rework the struct
> > > page_info on Arm64 because...
> > > 
> > > But then why
> > > > artificially increase the struct from 48 to 56 bytes on Arm64?
> > > > And hence
> > > > why have the same oddity on RV64?
> > > 
> > > 
> > > ... with 56 bytes, some struct page_info may cross a cache
> > > boundary.
> > 
> > I guess that's going to be challenging, unless you mean to go
> > further up
> > to 64 bytes?
> 
> Yes.
> 
> > 
> > > For
> > > RISC-V, I would recommend to make sure the struct page_info will
> > > never
> > > cross a cache boundary.
Do you mean that size(struct page_info) <= cache line size?

> > 
> > Since going up to 64 bytes is wasteful, 
> 
> Well yes. But this is a trade-off between performance and memory
> usage. 
> With the current situation, you may have to pull two cache lines for 
> struct page_info.
Just for my understanding.

stuct page_info will consume two cache lines (it happens in case of 3
consecutive which exceed the size of the cache line) if they are not
aligned on 64 bytes boundary as cache line size on ARM is 128 ( if
believe to ARM's asm/cache.h ).

Shouldn't alignment ( by adding empty fields inside page_info ) fix an
issue? It looks like if the size of page_info will not be aligned that
it will be always performance issue.

> 
> I suspect you might see some slow down when using the grant. But I
> don't 
> have any concrete numbers.
> 
> > and going down to 32 bytes likely
> > isn't going to be easy, sticking to 48 bytes for now would seem
> > reasonable
> > to me.
> 
> It may be more difficult to argue for an increase (if we notice any 
> performance degradation) in the future because this would reduce the 
> memory usable for every users.
> 
> Anyway, I haven't fully explore the problem on Arm yet and it is 
> possible we could deal with any performance degradation differently 
> (e.g. re-order the field and/or slightly increasing/decreasing the
> size).
> 
> I thought I would point it out just in case the RISC-V folks care
> about it.
I think it will be useful as RISC-V uses the same page_info structure.

~ Oleksii