Re: [PATCH 5/9] xen/ppc: Define minimal stub headers required for full build

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.08.2023 01:03, Shawn Anastasio wrote:
> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/altp2m.h
> @@ -0,0 +1,39 @@
> +/*
> + * Alternate p2m
> + *
> + * Copyright (c) 2014, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
> + */

Please use an SPDX header instead in new code. I also wonder about the
Intel copyright. I realize it's that way in the Arm header that you
apparently copied, but even there it's pretty odd. I don't think such
a pair of stub functions is reasonably copyrightable.

> +#ifndef __ASM_PPC_ALTP2M_H__
> +#define __ASM_PPC_ALTP2M_H__
> +
> +#include <xen/sched.h>

I don't think this is needed here (nor in Arm's original). All you
need are forward decls of struct domain and struct vcpu. And
xen/bug.h plus xen/types.h.

> --- a/xen/arch/ppc/include/asm/bug.h
> +++ b/xen/arch/ppc/include/asm/bug.h
> @@ -4,6 +4,7 @@
>  #define _ASM_PPC_BUG_H
>  
>  #include <xen/stringify.h>
> +#include <asm/processor.h>
>  
>  /*
>   * Power ISA guarantees that an instruction consisting of all zeroes is
> @@ -15,4 +16,10 @@
>  
>  #define BUG_FN_REG r0
>  
> +#define BUG() do { \
> +    die(); \
> +} while (0)

This looks like it's temporary. I think any construct that later needs
updating wants marking in some common way (such that it's easy to grep
for items left to be dealt with; you have such a comment in e.g.
asm/event.h). Of course if an entire header consists of _only_ stubs,
perhaps a single such comment would suffice.

> --- a/xen/arch/ppc/include/asm/cache.h
> +++ b/xen/arch/ppc/include/asm/cache.h
> @@ -3,4 +3,6 @@
>  #ifndef _ASM_PPC_CACHE_H
>  #define _ASM_PPC_CACHE_H
>  
> +#define __read_mostly __section(".data.read_mostly")

Not something for you to do, but we really want to move this to
xen/cache.h.

> diff --git a/xen/arch/ppc/include/asm/desc.h b/xen/arch/ppc/include/asm/desc.h
> new file mode 100644
> index 0000000000..e69de29bb2

Along the lines of the above - common code should not include this
header, and Arm shouldn't need one either. I'll see if I can sort
this.

> --- a/xen/arch/ppc/include/asm/mm.h
> +++ b/xen/arch/ppc/include/asm/mm.h
> @@ -1,19 +1,270 @@
>  #ifndef _ASM_PPC_MM_H
>  #define _ASM_PPC_MM_H
>  
> +#include <public/xen.h>
> +#include <xen/pdx.h>
> +#include <xen/types.h>
>  #include <asm/config.h>
>  #include <asm/page-bits.h>
>  
> +void setup_initial_pagetables(void);
> +
> +extern unsigned long total_pages;
> +
>  #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
>  #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
> +#define paddr_to_pdx(pa)    mfn_to_pdx(maddr_to_mfn(pa))
> +#define gfn_to_gaddr(gfn)   pfn_to_paddr(gfn_x(gfn))
> +#define gaddr_to_gfn(ga)    _gfn(paddr_to_pfn(ga))
> +#define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
> +#define maddr_to_mfn(ma)    _mfn(paddr_to_pfn(ma))
> +#define vmap_to_mfn(va)     maddr_to_mfn(virt_to_maddr((vaddr_t)va))
> +#define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
>  
>  #define virt_to_maddr(va) ((paddr_t)((vaddr_t)(va) & PADDR_MASK))
> -#define maddr_to_virt(pa) ((void *)((paddr_t)(pa) | XEN_VIRT_START)
> +#define maddr_to_virt(pa) ((void *)((paddr_t)(pa) | XEN_VIRT_START))
>  
>  /* Convert between Xen-heap virtual addresses and machine addresses. */
>  #define __pa(x)             (virt_to_maddr(x))
>  #define __va(x)             (maddr_to_virt(x))
>  
> -void setup_initial_pagetables(void);
> +/* Convert between Xen-heap virtual addresses and machine frame numbers. */
> +#define __virt_to_mfn(va) (virt_to_maddr(va) >> PAGE_SHIFT)
> +#define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
> +
> +/* Convert between Xen-heap virtual addresses and page-info structures. */
> +static inline struct page_info *virt_to_page(const void *v)
> +{
> +    BUG();
> +    return NULL;
> +}
> +
> +/*
> + * We define non-underscored wrappers for above conversion functions.
> + * These are overriden in various source files while underscored version
> + * remain intact.
> + */
> +#define virt_to_mfn(va)     __virt_to_mfn(va)
> +#define mfn_to_virt(mfn)    __mfn_to_virt(mfn)
> +
> +#define PG_shift(idx)   (BITS_PER_LONG - (idx))
> +#define PG_mask(x, idx) (x ## UL << PG_shift(idx))
> +
> +#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
> +#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
> +#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
> +
> + /* 2-bit count of uses of this frame as its current type. */
> +#define PGT_count_mask    PG_mask(3, 3)
> +
> +/* Cleared when the owning guest 'frees' this page. */
> +#define _PGC_allocated    PG_shift(1)
> +#define PGC_allocated     PG_mask(1, 1)
> +/* Page is Xen heap? */
> +#define _PGC_xen_heap     PG_shift(2)
> +#define PGC_xen_heap      PG_mask(1, 2)
> +/* Page is static memory */
> +#define PGC_static     0

You don't need this.

> +/* Page is broken? */
> +#define _PGC_broken       PG_shift(7)
> +#define PGC_broken        PG_mask(1, 7)
> + /* Mutually-exclusive page states: { inuse, offlining, offlined, free }. */
> +#define PGC_state         PG_mask(3, 9)
> +#define PGC_state_inuse   PG_mask(0, 9)
> +#define PGC_state_offlining PG_mask(1, 9)
> +#define PGC_state_offlined PG_mask(2, 9)
> +#define PGC_state_free    PG_mask(3, 9)
> +#define page_state_is(pg, st) (((pg)->count_info&PGC_state) == 
> PGC_state_##st)
> +/* Page is not reference counted */
> +#define _PGC_extra        PG_shift(10)
> +#define PGC_extra         PG_mask(1, 10)
> +
> +/* Count of references to this frame. */
> +#define PGC_count_width   PG_shift(10)
> +#define PGC_count_mask    ((1UL<<PGC_count_width)-1)
> +
> +/*
> + * Page needs to be scrubbed. Since this bit can only be set on a page that 
> is
> + * free (i.e. in PGC_state_free) we can reuse PGC_allocated bit.
> + */
> +#define _PGC_need_scrub   _PGC_allocated
> +#define PGC_need_scrub    PGC_allocated
> +
> +#define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
> +#define is_xen_heap_mfn(mfn) \
> +    (mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(mfn)))
> +
> +#define is_xen_fixed_mfn(mfn)                                   \
> +    ((mfn_to_maddr(mfn) >= virt_to_maddr(&_start)) &&           \
> +     (mfn_to_maddr(mfn) <= virt_to_maddr((vaddr_t)_end - 1)))
> +
> +#define page_get_owner(_p)    (_p)->v.inuse.domain
> +#define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
> +
> +/* TODO: implement */
> +#define mfn_valid(mfn) ({ (void) (mfn); 0; })
> +#define max_page ((unsigned long )0)

It's clear this is temporary, but it would still be nice if you could
omit stray blanks.

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/monitor.h
> @@ -0,0 +1,48 @@
> +/* Derived from xen/arch/arm/include/asm/monitor.h */
> +#ifndef __ASM_PPC_MONITOR_H__
> +#define __ASM_PPC_MONITOR_H__
> +
> +#include <public/domctl.h>
> +
> +#include <xen/sched.h>
> +#include <public/domctl.h>

Judging from the contents of the file, you don't need either (and you
certainly don't need public/domctl.h twice). Only xen/types.h looks to
be needed right now.

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/numa.h
> @@ -0,0 +1,26 @@
> +#ifndef __ASM_PPC_NUMA_H__
> +#define __ASM_PPC_NUMA_H__
> +
> +#include <xen/types.h>
> +#include <xen/mm.h>
> +
> +typedef uint8_t nodeid_t;
> +
> +/* Fake one node for now. See also node_online_map. */
> +#define cpu_to_node(cpu) 0
> +#define node_to_cpumask(node)   (cpu_online_map)
> +
> +/*
> + * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> + * is required because the dummy helpers are using it.
> + */
> +extern mfn_t first_valid_mfn;

At least "Arm" wants replacing in the comment, I think.

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/percpu.h
> @@ -0,0 +1,26 @@
> +#ifndef __PPC_PERCPU_H__
> +#define __PPC_PERCPU_H__
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/types.h>

Looks like nothing in the file requires this.

> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/procarea.h
> @@ -0,0 +1,38 @@
> +/*
> + * 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.

SPDX again please (and there's at least one more below).

> + * Copyright (C) IBM Corp. 2005
> + *
> + * Authors: Hollis Blanchard <hollisb@xxxxxxxxxx>
> + */
> +
> +#ifndef _ASM_PROCAREA_H_
> +#define _ASM_PROCAREA_H_
> +
> +#include <xen/types.h>

Again nothing looks to require this.

> +struct vcpu;
> +struct gdb_state;

The later of these is unused below. The former is used, but in a way
that would require a forward decl only in C++.

> --- a/xen/arch/ppc/include/asm/processor.h
> +++ b/xen/arch/ppc/include/asm/processor.h
> @@ -110,6 +110,10 @@
>  /* Macro to adjust thread priority for hardware multithreading */
>  #define HMT_very_low()  asm volatile ( "or %r31, %r31, %r31" )
>  
> +/* TODO: This isn't correct */
> +#define cpu_to_core(_cpu)   (0)
> +#define cpu_to_socket(_cpu) (0)

As mentioned elsewhere, please try to avoid leading underscores in
macro parameter names (or macro local variables, just to mention
it again in this context).

> @@ -175,6 +179,8 @@ static inline void noreturn die(void)
>          HMT_very_low();
>  }
>  
> +#define cpu_relax() asm volatile ( "or %r1, %r1, %r1; or %r2, %r2, %r2" )

Just like HMT_very_low() this could do with a comment explaining
the "interesting" ORs (until such time when the assembler supports
suitable mnemonics).

> --- a/xen/arch/ppc/include/asm/regs.h
> +++ b/xen/arch/ppc/include/asm/regs.h
> @@ -23,6 +23,8 @@
>  #ifndef _ASM_REG_DEFS_H_
>  #define _ASM_REG_DEFS_H_
>  
> +#include <xen/types.h>
> +
>  /* Special Purpose Registers */
>  #define SPRN_VRSAVE 256
>  #define SPRN_DSISR  18

Why would this #include be needed here all of the sudden?

> --- a/xen/arch/ppc/include/asm/system.h
> +++ b/xen/arch/ppc/include/asm/system.h
> @@ -1,6 +1,247 @@
> +/*
> + * 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.
> + *
> + * Copyright (C) IBM Corp. 2005
> + * Copyright (C) Raptor Engineering LLC
> + *
> + * Authors: Jimi Xenidis <jimix@xxxxxxxxxxxxxx>
> + *          Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> + */
> +
>  #ifndef _ASM_SYSTEM_H_
>  #define _ASM_SYSTEM_H_
>  
> -#define smp_wmb() __asm__ __volatile__ ( "lwsync" : : : "memory" )
> +#include <xen/config.h>

As mentioned before - any such #include you find is a leftover.

Jan