Re: [PATCH v12 5/8] xen/riscv: add minimal stuff to mm.h to build full Xen

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

This patch looks like it can go in independently?  Or does it depend on
having bitops.h working in practice?

However, one very strong suggestion...


> diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
> index 07c7a0abba..cc4a07a71c 100644
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -3,11 +3,246 @@
> <snip>
> +/* PDX of the first page in the frame table. */
> +extern unsigned long frametable_base_pdx;
> +
> +/* Convert between machine frame numbers and page-info structures. */
> +#define mfn_to_page(mfn)                                            \
> +    (frame_table + (mfn_to_pdx(mfn) - frametable_base_pdx))
> +#define page_to_mfn(pg)                                             \
> +    pdx_to_mfn((unsigned long)((pg) - frame_table) + frametable_base_pdx)

Do yourself a favour and not introduce frametable_base_pdx to begin with.

Every RISC-V board I can find has things starting from 0 in physical
address space, with RAM starting immediately after.

Taking the microchip board as an example, RAM actually starts at
0x8000000, which means that having frametable_base_pdx and assuming it
does get set to 0x8000 (which isn't even a certainty, given that I think
you'll need struct pages covering the PLICs), then what you are trading
off is:

* Saving 32k of virtual address space only (no need to even allocate
memory for this range of the framtable), by
* Having an extra memory load and add/sub in every page <-> mfn
conversion, which is a screaming hotpath all over Xen.

It's a terribly short-sighted tradeoff.

32k of VA space might be worth saving in a 32bit build (I personally
wouldn't - especially as there's no need to share Xen's VA space with
guests, given no PV guests on ARM/RISC-V), but it's absolutely not at
all in an a 64bit build with TB of VA space available.

Even if we do find a board with the first interesting thing in the
frametable starting sufficiently away from 0 that it might be worth
considering this slide, then it should still be Kconfig-able in a
similar way to PDX_COMPRESSION.

You don't want to be penalising everyone because of a theoretical/weird
corner case.

~Andrew