Re: [PATCH v3 08/14] xen: arm: define guest virtual platform in API headers

[Julien Grall <julien.grall@linaro.org>]

On 11/07/2013 08:44 AM, Ian Campbell wrote:
> The tools and the hypervisor need to agree on various aspects of the guest
> environment, such as interrupt numbers, memory layout, initial register values
> for registers which are implementation defined etc. Therefore move the
> associated defines into the public interface headers, or create them as
> necessary. However these should not be exposed to guests, they should find
> these things out via device tree or should not be relying on implementation
> defined defaults.
>
> Various bits of the hypervisor needed to change to configure dom0 with the real
> platform values while using the virtual platform configuration for guests.
> Arrange for this where appropriate and plumb through as needed.
>
> We also need to expose some 64-bit values (e.g. PSR_GUEST64_INIT) for the
> benefit of 32 bit toolstacks building 64 bit guests.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   tools/libxc/xc_dom_arm.c        |    4 +--
>   xen/arch/arm/domain.c           |    8 ++++--
>   xen/arch/arm/domain_build.c     |   13 +++++----
>   xen/arch/arm/gic.c              |   21 +++++++++-----
>   xen/arch/arm/psci.c             |    2 +-
>   xen/arch/arm/traps.c            |    2 +-
>   xen/arch/arm/vtimer.c           |   13 ++++++---
>   xen/include/asm-arm/domain.h    |    1 +
>   xen/include/asm-arm/event.h     |    3 +-
>   xen/include/asm-arm/gic.h       |    3 --
>   xen/include/asm-arm/processor.h |    7 -----
>   xen/include/asm-arm/psci.h      |    5 ----
>   xen/include/public/arch-arm.h   |   60 +++++++++++++++++++++++++++++++--------
>   13 files changed, 91 insertions(+), 51 deletions(-)
>

[..]

>   /* HCR Hyp Configuration Register */
>   #define HCR_RW          (1<<31) /* Register Width, ARM64 only */
>   #define HCR_TGE         (1<<27) /* Trap General Exceptions */
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index fdba636..67d4c35 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -6,11 +6,6 @@
>   #define PSCI_EINVAL  -2
>   #define PSCI_DENIED  -3
>
> -#define __PSCI_cpu_suspend 0
> -#define __PSCI_cpu_off     1
> -#define __PSCI_cpu_on      2
> -#define __PSCI_migrate     3
> -
>   int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
>   int do_psci_cpu_off(uint32_t power_state);
>   int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 5d359af..70a0766 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -268,8 +268,21 @@ typedef uint64_t xen_callback_t;
>
>   #endif
>
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
>   /* PSR bits (CPSR, SPSR)*/
>
> +#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
> +#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
> +#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
> +#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
> +#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
> +#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */

If you keep __aarch64__, you won't be able to have dom0 in 32 bits on arm64.
BTW, what does prevent the developer to use arm64 defines on arm64?

> +#define PSR_DBG_MASK    (1<<9)
> +#endif
> +#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
> +#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
> +
>   /* 32 bit modes */
>   #define PSR_MODE_USR 0x10
>   #define PSR_MODE_FIQ 0x11
> @@ -281,8 +294,6 @@ typedef uint64_t xen_callback_t;
>   #define PSR_MODE_UND 0x1b
>   #define PSR_MODE_SYS 0x1f
>
> -/* 64 bit modes */

I would keep this commit. It's good to know that it's ARM64 specific.

> -#ifdef __aarch64__
>   #define PSR_MODE_BIT  0x10 /* Set iff AArch32 */
>   #define PSR_MODE_EL3h 0x0d
>   #define PSR_MODE_EL3t 0x0c
> @@ -291,18 +302,43 @@ typedef uint64_t xen_callback_t;
>   #define PSR_MODE_EL1h 0x05
>   #define PSR_MODE_EL1t 0x04
>   #define PSR_MODE_EL0t 0x00
> -#endif
>
> -#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
> -#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
> -#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
> -#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
> -#define PSR_BIG_ENDIAN  (1<<9)        /* Big Endian Mode */
> -#ifdef __aarch64__ /* For Aarch64 bit 9 is repurposed. */
> -#define PSR_DBG_MASK    (1<<9)
> +#define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h)
> +
> +#define SCTLR_GUEST_INIT    0x00c50078
> +
> +/*
> + * Virtual machine platform (memory layout, interrupts)
> + *
> + * These are defined for consistency between the tools and the
> + * hypervisor. Guests must not rely on these hardcoded values but
> + * should instead use the FDT.
> + */
> +

I don't like the solution to hardcode GIC, IRQs (timer + evtchn) in the 
public interface. If we keep this solution, we will need to modify, 
again, the ABI for ARM, why can't we add/extend some hypercalls to give 
theses values to the hypervisor?

> +/* Physical Address Space */
> +#define GUEST_GICD_BASE   0x2c001000ULL
> +#define GUEST_GICD_SIZE   0x1000ULL
> +#define GUEST_GICC_BASE   0x2c002000ULL
> +#define GUEST_GICC_SIZE   0x100ULL
> +
> +#define GUEST_RAM_BASE    0x80000000ULL

This define should be libxc/libxl specific. Xen should not use this value.

> +
> +#define GUEST_GNTTAB_BASE 0xb0000000ULL
> +#define GUEST_GNTTAB_SIZE 0x00020000ULL
> +
> +/* Interrupts */
> +#define GUEST_TIMER_VIRT_PPI    27
> +#define GUEST_TIMER_PHYS_S_PPI  29
> +#define GUEST_TIMER_PHYS_NS_PPI 30
> +#define GUEST_EVTCHN_PPI        31
> +
> +/* PSCI functions */
> +#define PSCI_cpu_suspend 0
> +#define PSCI_cpu_off     1
> +#define PSCI_cpu_on      2
> +#define PSCI_migrate     3
> +
>   #endif
> -#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
> -#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
>
>   #endif /*  __XEN_PUBLIC_ARCH_ARM_H__ */
>
>

-- 
Julien Grall