On 21.08.2024 18:06, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -41,8 +41,10 @@
> * Start addr | End addr | Slot | area description
> *
> ============================================================================
> * ..... L2 511 Unused
> - * 0xffffffffc0600000 0xffffffffc0800000 L2 511 Fixmap
> - * 0xffffffffc0200000 0xffffffffc0600000 L2 511 FDT
> + * 0xffffffffc0A00000 0xffffffffc0C00000 L2 511 Fixmap
Nit: Please can you avoid using mixed case in numbers?
> @@ -74,6 +76,15 @@
> #error "unsupported RV_STAGE1_MODE"
> #endif
>
> +#define GAP_SIZE MB(2)
> +
> +#define XEN_VIRT_SIZE MB(2)
> +
> +#define BOOT_FDT_VIRT_START (XEN_VIRT_START + XEN_VIRT_SIZE + GAP_SIZE)
> +#define BOOT_FDT_VIRT_SIZE MB(4)
> +
> +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE +
> GAP_SIZE)
Nit: Overly long line.
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/fixmap.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * fixmap.h: compile-time virtual memory allocation
> + */
> +#ifndef ASM_FIXMAP_H
> +#define ASM_FIXMAP_H
> +
> +#include <xen/bug.h>
> +#include <xen/page-size.h>
> +#include <xen/pmap.h>
> +
> +#include <asm/page.h>
> +
> +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE)
> +
> +/* Fixmap slots */
> +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
> +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */
> +#define FIX_MISC (FIX_PMAP_END + 1) /* Ephemeral mappings of hardware */
> +
> +#define FIX_LAST FIX_MISC
> +
> +#define FIXADDR_START FIXMAP_ADDR(0)
> +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST + 1)
> +
> +#ifndef __ASSEMBLY__
> +
> +/*
> + * Direct access to xen_fixmap[] should only happen when {set,
> + * clear}_fixmap() is unusable (e.g. where we would end up to
> + * recursively call the helpers).
> + */
> +extern pte_t xen_fixmap[];
I'm afraid I keep being irritated by the comment: What recursive use of
helpers is being talked about here? I can't see anything recursive in this
patch. If this starts happening with a subsequent patch, then you have
two options: Move the declaration + comment there, or clarify in the
description (in enough detail) what this is about.
> @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned long mfn,
> bool sync_icache)
> BUG_ON("unimplemented");
> }
>
> +/* Write a pagetable entry. */
> +static inline void write_pte(pte_t *p, pte_t pte)
> +{
> + write_atomic(p, pte);
> +}
> +
> +/* Read a pagetable entry. */
> +static inline pte_t read_pte(pte_t *p)
> +{
> + return read_atomic(p);
This only works because of the strange type trickery you're playing in
read_atomic(). Look at x86 code - there's a strict expectation that the
type can be converted to/from unsigned long. And page table accessors
are written with that taken into consideration. Same goes for write_pte()
of course, with the respective comment on the earlier patch in mind.
Otoh I see that Arm does something very similar. If you have a strong
need / desire to follow that, then please at least split the two
entirely separate aspects that patch 1 presently changes both in one go.
Jan
