Re: [PATCH v5 2/7] xen/riscv: set up fixmap mappings

[Jan Beulich <jbeulich@xxxxxxxx>]

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