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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-11-11 at 14:25 +0000, Julien Grall wrote:
> 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)

That could work I suppose. Is it really worth it for this one bit?

> >> 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).

Yes. We should fix this, but it is out of scope for this series.

> > 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?

The stuff which  this patch adds is supposed to be that document.
Specifically:

+/* 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
+
+#define GUEST_GNTTAB_BASE 0xb0000000ULL
+#define GUEST_GNTTAB_SIZE 0x00020000ULL

I could duplicate those numbers in a comment but it seems a bit
pointless and prone to skew.

Ian.


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