Re: [Xen-devel] [PATCH] xen: get GIC addresses from DT

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 29 Nov 2012, Ian Campbell wrote:
> On Fri, 2012-11-23 at 15:21 +0000, Stefano Stabellini wrote:
> > Get the address of the GIC distributor, cpu, virtual and virtual cpu
> > interfaces registers from device tree.
> > 
> > 
> > Note: I couldn't completely get rid of GIC_BASE_ADDRESS, GIC_DR_OFFSET
> > and friends because we are using them from mode_switch.S, that is
> > executed before device tree has been parsed. But at least mode_switch.S
> > is known to contain vexpress specific code anyway.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> I've a few comments on mostly pre-existing things which I happened to
> notice while reviewing this:
> 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 0c6fab9..8efbeb3 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -34,9 +35,9 @@
> >  /* Access to the GIC Distributor registers through the fixmap */
> >  #define GICD ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICD))
> 
> There's an implicit assumption here that the GICD is correctly aligned
> -- I suspect it really ought to be handled like the others... but...
> 
> >  #define GICC ((volatile uint32_t *) (FIXMAP_ADDR(FIXMAP_GICC1)  \
> > -                                     + (GIC_CR_OFFSET & 0xfff)))
> > +                                     + ((uint32_t) gic.cbase & 0xfff)))
> >  #define GICH ((volatile uint32_t *) (FIXMAP_ADDR(FIXMAP_GICH)  \
> > -                                     + (GIC_HR_OFFSET & 0xfff)))
> > +                                     + ((uint32_t) gic.hbase & 0xfff)))
> 
> ... is there actually any chance of these being non-page aligned?

I couldn't find any specific references to interface alignment in the
GIC documentation, but I strongly doubt they can be non-page aligned.
I think we should just add a check to see if they are page aligned,
aborting if they are not.


> Also now that these are dynamic perhaps an actual variable initialised
> at start of day would be better than a #define?

After all the virtual addresses for gicd, gicc and gich are always going
to be the same, no matter what the their physical address is.
I don't think is worth turning the #define into variables.


---

arm: add few checks to gic_init

Check for:
- uninitialized GIC interface addresses;
- non-page aligned GIC interface addresses.

Return in both cases with an error message.
Also remove the code from GICH and GICC to handle non-page aligned
interfaces.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 8efbeb3..301d223 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -34,10 +34,8 @@
 
 /* Access to the GIC Distributor registers through the fixmap */
 #define GICD ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICD))
-#define GICC ((volatile uint32_t *) (FIXMAP_ADDR(FIXMAP_GICC1)  \
-                                     + ((uint32_t) gic.cbase & 0xfff)))
-#define GICH ((volatile uint32_t *) (FIXMAP_ADDR(FIXMAP_GICH)  \
-                                     + ((uint32_t) gic.hbase & 0xfff)))
+#define GICC ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICC1)) 
+#define GICH ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICH))
 static void gic_restore_pending_irqs(struct vcpu *v);
 
 /* Global state */
@@ -308,6 +306,23 @@ static void __cpuinit gic_hyp_disable(void)
 /* Set up the GIC */
 void __init gic_init(void)
 {
+       if ( !early_info.gic.gic_dist_addr ||
+                       !early_info.gic.gic_cpu_addr ||
+                       !early_info.gic.gic_hyp_addr ||
+                       !early_info.gic.gic_vcpu_addr )
+       {
+               printk("error: incorrect physical address of the GIC 
interfaces.\n");
+               return;
+       }
+       if ( (early_info.gic.gic_dist_addr & ((1 << PAGE_SHIFT) - 1)) ||
+                       (early_info.gic.gic_cpu_addr & ((1 << PAGE_SHIFT) - 1)) 
||
+                       (early_info.gic.gic_hyp_addr & ((1 << PAGE_SHIFT) - 1)) 
||
+                       (early_info.gic.gic_vcpu_addr & ((1 << PAGE_SHIFT) - 
1)) )
+       {
+               printk("error: GIC interfaces not page aligned.\n");
+               return;
+       }
+
     gic.dbase = early_info.gic.gic_dist_addr;
     gic.cbase = early_info.gic.gic_cpu_addr;
     gic.hbase = early_info.gic.gic_hyp_addr;

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