Re: [PATCH 2/9] xen/ppc: Add public/arch-ppc.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.08.2023 01:02, Shawn Anastasio wrote:
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  xen/include/public/arch-ppc.h | 140 ++++++++++++++++++++++++++++++++++
>  1 file changed, 140 insertions(+)
>  create mode 100644 xen/include/public/arch-ppc.h
> 
> diff --git a/xen/include/public/arch-ppc.h b/xen/include/public/arch-ppc.h
> new file mode 100644
> index 0000000000..0eb7ce4208
> --- /dev/null
> +++ b/xen/include/public/arch-ppc.h
> @@ -0,0 +1,140 @@
> +/*
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
> and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.

Any reason for a spelled out license rather than an SPDX header?

> + * Copyright (C) IBM Corp. 2005, 2006
> + * Copyright (C) Raptor Engineering, LLC 2023
> + *
> + * Authors: Hollis Blanchard <hollisb@xxxxxxxxxx>
> + *          Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>
> + */
> +
> +#ifndef __XEN_PUBLIC_ARCH_PPC64_H__
> +#define __XEN_PUBLIC_ARCH_PPC64_H__

The 64 wants dropping here, considering the name of the header.

> +#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
> +#define uint64_aligned_t uint64_t __attribute__((aligned(8)))

I understand arch-arm.h has it this way too, but in public headers I
think we're better off using __aligned__ (in the example here).

> +#ifndef __ASSEMBLY__
> +#define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
> +    typedef union { type *p; unsigned long q; }                 \
> +        __guest_handle_ ## name;                                \
> +    typedef union { type *p; uint64_aligned_t q; }              \
> +        __guest_handle_64_ ## name
> +
> +#define __DEFINE_XEN_GUEST_HANDLE(name, type) \
> +    ___DEFINE_XEN_GUEST_HANDLE(name, type);   \
> +    ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type)
> +#define DEFINE_XEN_GUEST_HANDLE(name)   __DEFINE_XEN_GUEST_HANDLE(name, name)
> +#define __XEN_GUEST_HANDLE(name)        __guest_handle_64_ ## name
> +#define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
> +#define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_ ## name
> +#define set_xen_guest_handle_raw(hnd, val)                  \
> +    do {                                                    \
> +        __typeof__(&(hnd)) _sxghr_tmp = &(hnd);             \

In new code, can you please avoid underscore-prefixed macro locals,
which violate name space rules set forth by the standard? We appear
to be adopting underscore-suffixed naming for such locals.

> +        _sxghr_tmp->q = 0;                                  \
> +        _sxghr_tmp->p = val;                                \

"val" need parenthesizing here.

> +    } while ( 0 )
> +#define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
> +
> +#ifdef __XEN_TOOLS__
> +#define get_xen_guest_handle(val, hnd)  do { val = (hnd).p; } while (0)
> +#endif
> +
> +typedef uint64_t xen_pfn_t;
> +#define PRI_xen_pfn PRIx64
> +#define PRIu_xen_pfn PRIu64
> +
> +/*
> + * Maximum number of virtual CPUs in legacy multi-processor guests.
> + * Only one. All other VCPUS must use VCPUOP_register_vcpu_info.
> + */
> +#define XEN_LEGACY_MAX_VCPUS 1
> +
> +typedef uint64_t xen_ulong_t;
> +#define PRI_xen_ulong PRIx64
> +#endif
> +
> +/*
> + * Pointers and other address fields inside interface structures are padded 
> to
> + * 64 bits. This means that field alignments aren't different between 32- and
> + * 64-bit architectures.
> + */
> +/* NB. Multi-level macro ensures __LINE__ is expanded before concatenation. 
> */
> +#define __MEMORY_PADDING(_X)
> +#define _MEMORY_PADDING(_X)  __MEMORY_PADDING(_X)
> +#define MEMORY_PADDING       _MEMORY_PADDING(__LINE__)

This doesn't parallel anything in other architectures afaics, and it is
also not used anywhere in this header. What is this about? If it needs
to stay, it'll need properly moving into XEN_* namespace.

> +/* And the trap vector is... */
> +#define TRAP_INSTR "li 0,-1; sc" /* XXX just "sc"? */

Same question / remark here.

> +#ifndef __ASSEMBLY__
> +
> +#define XENCOMM_INLINE_FLAG (1UL << 63)

Is this an indication that you mean to revive xencomm.h?

> +typedef uint64_t xen_ulong_t;
> +
> +/* User-accessible registers: nost of these need to be saved/restored
> + * for every nested Xen invocation. */

Nit: comment style (and s/nost/most/).

> +struct vcpu_guest_core_regs
> +{
> +    uint64_t gprs[32];
> +    uint64_t lr;
> +    uint64_t ctr;
> +    uint64_t srr0;
> +    uint64_t srr1;
> +    uint64_t pc;
> +    uint64_t msr;
> +    uint64_t fpscr;             /* XXX Is this necessary */
> +    uint64_t xer;
> +    uint64_t hid4;              /* debug only */
> +    uint64_t dar;               /* debug only */
> +    uint32_t dsisr;             /* debug only */
> +    uint32_t cr;
> +    uint32_t __pad;             /* good spot for another 32bit reg */
> +    uint32_t entry_vector;
> +};
> +typedef struct vcpu_guest_core_regs vcpu_guest_core_regs_t;
> +
> +typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */ /* XXX timebase */
> +
> +/* ONLY used to communicate with dom0! See also struct exec_domain. */
> +struct vcpu_guest_context {
> +    vcpu_guest_core_regs_t user_regs;         /* User-level CPU registers    
>  */
> +    uint64_t sdr1;                     /* Pagetable base               */
> +    /* XXX etc */
> +};
> +typedef struct vcpu_guest_context vcpu_guest_context_t;
> +DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> +
> +struct arch_shared_info {
> +    uint64_t boot_timebase;
> +};
> +
> +struct arch_vcpu_info {
> +};
> +
> +struct xen_arch_domainconfig {
> +};
> +
> +typedef struct xen_pmu_arch { uint8_t dummy; } xen_pmu_arch_t;
> +
> +/* Support for multi-processor guests. */
> +#endif

Stray comment?

Jan