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

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

On 11/08/2013 09:48 AM, Ian Campbell wrote:
> On Thu, 2013-11-07 at 23:29 -0800, Julien Grall wrote:
> 
>>> +#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.
> 
> Oops. I removed the others but missed this one. Nothing about this
> particular #if would prevent you having a 32 bit dom0 on arm64.
> 
> But I'll remove the #define and add a comment anyway.
> 
>> BTW, what does prevent the developer to use arm64 defines on arm64?
> 
> Did you mean s/64/32/ in one of those? The answer is nothing other than
> code review, getting bug reports etc. I don't think there is any way we
> can prevent it programmatically. Maybe separate enums for 32 vs 64-bit
> PSR values, but I think that would get nasty fast.

What about defining arm64 specific stuff only if we are in xen tools or
in xen and build for arm64?

ie:
XEN_TOOLS || (XEN && 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.
> 
> Yes.
> 
>>
>>> -#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.
> 
> Note that this is inside
>         #if defined(__XEN__) || defined(__XEN_TOOLS__)
> so it is not exposed to guests and therefore not part of the guest ABI.
> The tools ABI is completely fungible and we can change it at will.
> 
>>  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?
> 
> That would be overkill until (if!) we get to the point of needing a more
> dynamic guest layout.

The current solution has some memory limitations. The layout doesn't
allow to allocate more than 768MB (space between the GUEST_RAM_BASE and
GUEST_GNTTAB_BASE).

> 
> I'm not going to implement that now. I thought I'd expressed this in the
> commit message or the cover letter but it seems I forgot. I'll add some
> words to the commit message for the next posting.

Ok. Can you document somewhere the guest memory layout?

-- 
Julien Grall