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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-08-17 at 17:00 +0200, Jan Beulich wrote:
> 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}?
Thanks. It should be updated to RISC-V.
> 
> > + */
> > +#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.
Thank you. Understood. I'll make the update.
> 
> > +#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.
I'll make the update. Thanks.

> 
> > +/* 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.
I'll take it into account.
> 
> > +#define XEN_LEGACY_MAX_VCPUS 1
> > +
> > +#endif /* __ASSEMBLY__ */
> > +
> > +#ifndef __ASSEMBLY__
> 
> Why not continue the earlier !__ASSEMBLY__ section?
Sure, we can continue the earlier !__ASSEBLY__ section. I'll update
this part.

~ Oleksii