Re: [PATCH v3 3/3] xen/ppc: Implement initial Radix MMU support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.08.2023 00:48, Shawn Anastasio wrote:
> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/page.h
> @@ -0,0 +1,181 @@
> +#ifndef _ASM_PPC_PAGE_H
> +#define _ASM_PPC_PAGE_H
> +
> +#include <xen/types.h>
> +
> +#include <asm/bitops.h>
> +#include <asm/byteorder.h>
> +
> +#define PDE_VALID     PPC_BIT(0)
> +#define PDE_NLB_MASK  0xfffffffffffffUL
> +#define PDE_NLB_SHIFT 5UL
> +#define PDE_NLS_MASK  0x1f
> +
> +#define PTE_VALID     PPC_BIT(0)
> +#define PTE_LEAF      PPC_BIT(1)
> +#define PTE_REFERENCE PPC_BIT(55)
> +#define PTE_CHANGE    PPC_BIT(56)
> +
> +/* PTE Attributes */
> +#define PTE_ATT_SAO            PPC_BIT(59) /* Strong Access Ordering */
> +#define PTE_ATT_NON_IDEMPOTENT PPC_BIT(58)
> +#define PTE_ATT_TOLERANT       (PPC_BIT(58) | PPC_BIT(59))
> +
> +/* PTE Encoded Access Authority*/
> +#define PTE_EAA_PRIVILEGED PPC_BIT(60)
> +#define PTE_EAA_READ       PPC_BIT(61)
> +#define PTE_EAA_WRITE      PPC_BIT(62)
> +#define PTE_EAA_EXECUTE    PPC_BIT(63)
> +
> +/* Field shifts/masks */
> +#define PTE_RPN_MASK  0x1fffffffffffUL
> +#define PTE_RPN_SHIFT 12UL

I still don't understand why you need two constants here. Even more so that
now all their uses are as (PTE_RPN_MASK << PTE_RPN_SHIFT). With (only)

#define PTE_RPN_MASK  0x1fffffffffff000UL

instead, as indicated before, you would then (should the need arise) have
this usable with MASK_EXTR() / MASK_INSR(). The again you likely know much
better than me what further uses are going to appear.

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/regs.h
> @@ -0,0 +1,138 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.

There's still an SPDX header and a full license here.

> --- /dev/null
> +++ b/xen/arch/ppc/mm-radix.c
> @@ -0,0 +1,265 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include <xen/init.h>
> +#include <xen/kernel.h>
> +#include <xen/types.h>
> +#include <xen/lib.h>
> +
> +#include <asm/bitops.h>
> +#include <asm/byteorder.h>
> +#include <asm/early_printk.h>
> +#include <asm/mm.h>
> +#include <asm/page.h>
> +#include <asm/processor.h>
> +#include <asm/regs.h>
> +#include <asm/msr.h>
> +
> +void enable_mmu(void);
> +
> +#define INITIAL_LVL1_PD_COUNT      1
> +#define INITIAL_LVL2_LVL3_PD_COUNT 2
> +#define INITIAL_LVL4_PT_COUNT      256
> +
> +static size_t initial_lvl1_pd_pool_used;
> +static struct lvl1_pd initial_lvl1_pd_pool[INITIAL_LVL1_PD_COUNT];
> +
> +static size_t initial_lvl2_lvl3_pd_pool_used;
> +static struct lvl2_pd initial_lvl2_lvl3_pd_pool[INITIAL_LVL2_LVL3_PD_COUNT];
> +
> +static size_t initial_lvl4_pt_pool_used;
> +static struct lvl4_pt initial_lvl4_pt_pool[INITIAL_LVL4_PT_COUNT];

The initial_lvl..._pool_used variables can likely all be __initdata?

> +static void __init setup_initial_mapping(struct lvl1_pd *lvl1,
> +                                         vaddr_t map_start,
> +                                         vaddr_t map_end,
> +                                         paddr_t phys_base)
> +{
> +    uint64_t page_addr;
> +
> +    if ( map_start & ~PAGE_MASK )
> +    {
> +        early_printk("Xen _start be aligned to 64k (PAGE_SIZE) boundary\n");
> +        die();
> +    }
> +
> +    if ( phys_base & ~PAGE_MASK )
> +    {
> +        early_printk("Xen should be loaded at 64k (PAGE_SIZE) boundary\n");
> +        die();
> +    }
> +
> +    for ( page_addr = map_start; page_addr < map_end; page_addr += PAGE_SIZE 
> )
> +    {
> +        struct lvl2_pd *lvl2;
> +        struct lvl3_pd *lvl3;
> +        struct lvl4_pt *lvl4;
> +        pde_t *pde;
> +        pte_t *pte;
> +
> +        /* Allocate LVL 2 PD if necessary */
> +        pde = pt_entry(lvl1, page_addr);
> +        if ( !pde_is_valid(*pde) )
> +        {
> +            lvl2 = lvl2_pd_pool_alloc();
> +            *pde = paddr_to_pde(__pa(lvl2), PDE_VALID,
> +                                XEN_PT_ENTRIES_LOG2_LVL_2);
> +        }
> +        else
> +            lvl2 = (struct lvl2_pd *)__va(pde_to_paddr(*pde));

I don't think the cast here (and similar ones below) is needed.

> +static void __init setup_partition_table(struct lvl1_pd *root)
> +{
> +    unsigned long ptcr;
> +
> +    /* Configure entry for LPID 0 to enable Radix and point to root PD */
> +    uint64_t patb0 = RTS_FIELD | __pa(root) | XEN_PT_ENTRIES_LOG2_LVL_1 |
> +                     PATB0_HR;
> +    uint64_t patb1 = __pa(initial_prtb) | (PRTB_SIZE_LOG2 - 12) | PATB1_GR;
> +    initial_patb[0].patb0 = cpu_to_be64(patb0);
> +    initial_patb[0].patb1 = cpu_to_be64(patb1);

Nit: Blank line please between declaration(s) and statement(s).

> +    ptcr = __pa(initial_patb) | (PATB_SIZE_LOG2 - 12);
> +    mtspr(SPRN_PTCR, ptcr);
> +}
> +
> +static void __init setup_process_table(struct lvl1_pd *root)
> +{
> +    /* Configure entry for PID 0 to point to root PD */
> +    uint64_t prtb0 = RTS_FIELD | __pa(root) | XEN_PT_ENTRIES_LOG2_LVL_1;
> +    initial_prtb[0].prtb0 = cpu_to_be64(prtb0);

Same here then.

Jan