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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-11-30 at 12:08 +0000, Stefano Stabellini wrote:
> 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.

Agreed.

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

Right, with the above change this is indeed pointless.

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

This indent is pretty odd. Better to align the "!"s I think.

Also you appear to have hard tabs in here (perhaps that is the problem)

> +     {
> +             printk("error: incorrect physical address of the GIC 
> interfaces.\n");

Incorrect isn't quite right, they are missing.

>From a debugging PoV it might be worth checking GICD and GICC (which are
common) from GICH and GICV (which we might plausibly expect to be
missing on occasion) and/or print out the actual values.

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

Alignment (and hard tabs) again.

> +     {
> +             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



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