Re: [PATCH v1 02/57] xen/riscv: add public arch-riscv.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2023 12:19, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/include/public/arch-riscv.h
> @@ -0,0 +1,90 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Guest OS interface to RISC-V Xen.
> + * Initially based on the ARM implementation.
> + */
> +
> +#ifndef __XEN_PUBLIC_ARCH_RISCV_H__
> +#define __XEN_PUBLIC_ARCH_RISCV_H__
> +
> +#define  int64_aligned_t  int64_t __attribute__((__aligned__(8)))
> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
> +
> +#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
> +
> +/*
> + * XEN_GUEST_HANDLE represents a guest pointer, when passed as a field
> + * in a struct in memory. On RISCV is always 8 bytes sizes and 8 bytes
> + * aligned.
> + * XEN_GUEST_HANDLE_PARAM represents a guest pointer, when passed as an
> + * hypercall argument. It is 4 bytes on aarch32 and 8 bytes on aarch64.

Nit: aarch{32,64}?

> + */
> +#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);                 \
> +        _sxghr_tmp->q = 0;                                  \
> +        _sxghr_tmp->p = (val);                              \
> +    } while ( 0 )

While I realize you simply took this from Arm, in new code I think it
would be helpful to avoid name space violations from the beginning.
Hence no leading underscore please for macro local variables. Trailing
underscores is what we mean to use instead.

It's also not really valid to use a gcc extension here, but I guess
that's hard to avoid.

> +#define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
> +
> +typedef uint64_t xen_pfn_t;
> +#define PRI_xen_pfn PRIx64
> +#define PRIu_xen_pfn PRIu64
> +
> +typedef uint64_t xen_ulong_t;
> +#define PRI_xen_ulong PRIx64
> +
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
> +struct vcpu_guest_context {
> +};
> +typedef struct vcpu_guest_context vcpu_guest_context_t;
> +DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> +
> +struct xen_arch_domainconfig {
> +};

While these are okay to remain empty, ...

> +#endif
> +
> +struct arch_vcpu_info {
> +};
> +typedef struct arch_vcpu_info arch_vcpu_info_t;
> +
> +struct arch_shared_info {
> +};
> +typedef struct arch_shared_info arch_shared_info_t;

... these two need to gain a "todo" marker so that we won't forget
to at least add a placeholder entry if no real ones surface.

> +/* Maximum number of virtual CPUs in legacy multi-processor guests. */
> +/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */

Nit: Style (missing full stop). And quite likely the two comments could
be joined to a single one.

> +#define XEN_LEGACY_MAX_VCPUS 1
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#ifndef __ASSEMBLY__

Why not continue the earlier !__ASSEMBLY__ section?

Jan