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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

> 
> > +#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.

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.

> > +/* 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.

No, it is part of the guest ABI which Xen is presenting, even if Xen
isn't actually using it. It needs to fit in with the other physical
address properties. It is consistent for it to be defined here.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel