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@eu.citrix.com>
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
@@ -26,6 +26,7 @@
#include <xen/errno.h>
#include <xen/softirq.h>
#include <xen/list.h>
+#include <xen/device_tree.h>
#include <asm/p2m.h>
#include <asm/domain.h>
@@ -34,9 +35,9 @@
/* 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) \
- + (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)))
static void gic_restore_pending_irqs(struct vcpu *v);
/* Global state */
@@ -44,6 +45,7 @@ static struct {
paddr_t dbase; /* Address of distributor registers */
paddr_t cbase; /* Address of CPU interface registers */
paddr_t hbase; /* Address of virtual interface registers */
+ paddr_t vbase; /* Address of virtual cpu interface registers */
unsigned int lines;
unsigned int cpus;
spinlock_t lock;
@@ -306,10 +308,10 @@ static void __cpuinit gic_hyp_disable(void)
/* Set up the GIC */
void __init gic_init(void)
{
- /* XXX FIXME get this from devicetree */
- gic.dbase = GIC_BASE_ADDRESS + GIC_DR_OFFSET;
- gic.cbase = GIC_BASE_ADDRESS + GIC_CR_OFFSET;
- gic.hbase = GIC_BASE_ADDRESS + GIC_HR_OFFSET;
+ gic.dbase = early_info.gic.gic_dist_addr;
+ gic.cbase = early_info.gic.gic_cpu_addr;
+ gic.hbase = early_info.gic.gic_hyp_addr;
+ gic.vbase = early_info.gic.gic_vcpu_addr;
set_fixmap(FIXMAP_GICD, gic.dbase >> PAGE_SHIFT, DEV_SHARED);
BUILD_BUG_ON(FIXMAP_ADDR(FIXMAP_GICC1) !
FIXMAP_ADDR(FIXMAP_GICC2)-PAGE_SIZE);
@@ -569,9 +571,9 @@ int gicv_setup(struct domain *d)
{
/* map the gic virtual cpu interface in the gic cpu interface region of
* the guest */
- return map_mmio_regions(d, GIC_BASE_ADDRESS + GIC_CR_OFFSET,
- GIC_BASE_ADDRESS + GIC_CR_OFFSET + (2 * PAGE_SIZE) - 1,
- GIC_BASE_ADDRESS + GIC_VR_OFFSET);
+ return map_mmio_regions(d, gic.cbase,
+ gic.cbase + (2 * PAGE_SIZE) - 1,
+ gic.vbase);
}
static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs
*regs)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index da0af77..79b1dd1 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -54,6 +54,27 @@ bool_t device_tree_type_matches(const void *fdt, int node,
const char *match)
return !strncmp(prop, match, len);
}
+bool_t device_tree_node_compatible(const void *fdt, int node, const char
*match)
+{
+ int len, l;
+ const void *prop;
+
+ prop = fdt_getprop(fdt, node, "compatible", &len);
+ if ( prop == NULL )
+ return 0;
+
+ while ( len > 0 ) {
+ if ( !strncmp(prop, match, strlen(match)) )
+ return 1;
+ l = strlen(prop) + 1;
+ prop += l;
+ len -= l;
+ }
+
+ return 0;
+}
+
+
static void __init get_val(const u32 **cell, u32 cells, u64 *val)
{
*val = 0;
@@ -270,6 +291,50 @@ static void __init process_cpu_node(const void *fdt, int
node,
cpumask_set_cpu(start, &cpu_possible_map);
}
+static void __init process_gic_node(const void *fdt, int node,
+ const char *name,
+ u32 address_cells, u32 size_cells)
+{
+ const struct fdt_property *prop;
+ const u32 *cell;
+ paddr_t start, size;
+ int reg_cells, interfaces;
+
+ if ( address_cells < 1 || size_cells < 1 )
+ {
+ early_printk("fdt: node `%s'': invalid #address-cells or
#size-cells",
+ name);
+ return;
+ }
+
+ prop = fdt_get_property(fdt, node, "reg", NULL);
+ if ( !prop )
+ {
+ early_printk("fdt: node `%s'': missing `reg''
property\n", name);
+ return;
+ }
+
+ cell = (const u32 *)prop->data;
+ device_tree_get_reg(&cell, address_cells, size_cells, &start,
&size);
+
+ cell = (const u32 *)prop->data;
+ reg_cells = address_cells + size_cells;
+ interfaces = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));
+ if ( interfaces < 4 )
+ {
+ early_printk("fdt: node `%s'': invalid `reg''
property\n", name);
+ return;
+ }
+ device_tree_get_reg(&cell, address_cells, size_cells, &start,
&size);
+ early_info.gic.gic_dist_addr = start;
+ device_tree_get_reg(&cell, address_cells, size_cells, &start,
&size);
+ early_info.gic.gic_cpu_addr = start;
+ device_tree_get_reg(&cell, address_cells, size_cells, &start,
&size);
+ early_info.gic.gic_hyp_addr = start;
+ device_tree_get_reg(&cell, address_cells, size_cells, &start,
&size);
+ early_info.gic.gic_vcpu_addr = start;
+}
+
static int __init early_scan_node(const void *fdt,
int node, const char *name, int depth,
u32 address_cells, u32 size_cells,
@@ -279,6 +344,8 @@ static int __init early_scan_node(const void *fdt,
process_memory_node(fdt, node, name, address_cells, size_cells);
else if ( device_tree_type_matches(fdt, node, "cpu") )
process_cpu_node(fdt, node, name, address_cells, size_cells);
+ else if ( device_tree_node_compatible(fdt, node,
"arm,cortex-a15-gic") )
+ process_gic_node(fdt, node, name, address_cells, size_cells);
return 0;
}
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 4d010c0..a0e3a97 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -26,8 +26,16 @@ struct dt_mem_info {
struct membank bank[NR_MEM_BANKS];
};
+struct dt_gic_info {
+ paddr_t gic_dist_addr;
+ paddr_t gic_cpu_addr;
+ paddr_t gic_hyp_addr;
+ paddr_t gic_vcpu_addr;
+};
+
struct dt_early_info {
struct dt_mem_info mem;
+ struct dt_gic_info gic;
};
typedef int (*device_tree_node_func)(const void *fdt,
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@eu.citrix.com>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? Also now that these are dynamic perhaps an actual variable initialised at start of day would be better than a #define? Ian.
On 23/11/12 15:21, Stefano Stabellini wrote:> Get the address of the GIC distributor, cpu, virtual and virtual cpu > interfaces registers from device tree.The original intention of the early DTB parsing was to get the minimum of info needed before the DTB could be translated into a more useful form (similar to what Linux does). Is this something that is still being considered in the long term? If so, should the GIC be initialized from this translated form, instead of using the early parsing? David> > > 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@eu.citrix.com> >[...]> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index da0af77..79b1dd1 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -54,6 +54,27 @@ bool_t device_tree_type_matches(const void *fdt, int node, const char *match) > return !strncmp(prop, match, len); > } > > +bool_t device_tree_node_compatible(const void *fdt, int node, const char *match) > +{ > + int len, l; > + const void *prop; > + > + prop = fdt_getprop(fdt, node, "compatible", &len); > + if ( prop == NULL ) > + return 0; > + > + while ( len > 0 ) { > + if ( !strncmp(prop, match, strlen(match)) ) > + return 1; > + l = strlen(prop) + 1; > + prop += l; > + len -= l; > + }This doesn''t look right. Don''t you need to split the prop string on comma boundaries?> + > + return 0; > +} > + > + > static void __init get_val(const u32 **cell, u32 cells, u64 *val) > { > *val = 0; > @@ -270,6 +291,50 @@ static void __init process_cpu_node(const void *fdt, int node, > cpumask_set_cpu(start, &cpu_possible_map); > } > > +static void __init process_gic_node(const void *fdt, int node, > + const char *name, > + u32 address_cells, u32 size_cells) > +{ > + const struct fdt_property *prop; > + const u32 *cell; > + paddr_t start, size; > + int reg_cells, interfaces; > + > + if ( address_cells < 1 || size_cells < 1 ) > + { > + early_printk("fdt: node `%s'': invalid #address-cells or #size-cells", > + name); > + return; > + } > + > + prop = fdt_get_property(fdt, node, "reg", NULL); > + if ( !prop ) > + { > + early_printk("fdt: node `%s'': missing `reg'' property\n", name); > + return; > + } > + > + cell = (const u32 *)prop->data; > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);Is this needed? This cell is reread below.> + > + cell = (const u32 *)prop->data; > + reg_cells = address_cells + size_cells; > + interfaces = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));Perhaps wrap this in a helper function? Look like it might be useful elsewhere.> + if ( interfaces < 4 ) > + { > + early_printk("fdt: node `%s'': invalid `reg'' property\n", name); > + return; > + } > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > + early_info.gic.gic_dist_addr = start; > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > + early_info.gic.gic_cpu_addr = start; > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > + early_info.gic.gic_hyp_addr = start; > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > + early_info.gic.gic_vcpu_addr = start;Is the GIC driver still hardcoding the register region sizes? Or does it not need the size?> +} > + > static int __init early_scan_node(const void *fdt, > int node, const char *name, int depth, > u32 address_cells, u32 size_cells, > @@ -279,6 +344,8 @@ static int __init early_scan_node(const void *fdt, > process_memory_node(fdt, node, name, address_cells, size_cells); > else if ( device_tree_type_matches(fdt, node, "cpu") ) > process_cpu_node(fdt, node, name, address_cells, size_cells); > + else if ( device_tree_node_compatible(fdt, node, "arm,cortex-a15-gic") ) > + process_gic_node(fdt, node, name, address_cells, size_cells);Long term, I don''t think you want a long list of all the different interrupt controllers here. David
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@eu.citrix.com> > > 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@eu.citrix.com> 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;
At 12:08 +0000 on 30 Nov (1354277311), Stefano Stabellini wrote:> 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@eu.citrix.com> > > 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 ||Some hard tabs have snuck in here.> + !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;Maybe panic() here (and below) rather than printk?> + } > + if ( (early_info.gic.gic_dist_addr & ((1 << PAGE_SHIFT) - 1)) ||(foo & ~PAGE_MASK) is usual for that. Cheers, Tim.> + (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@lists.xen.org > http://lists.xen.org/xen-devel
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@eu.citrix.com> > > > > 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@eu.citrix.com> > > 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@lists.xen.org > http://lists.xen.org/xen-devel
On Fri, 30 Nov 2012, David Vrabel wrote:> On 23/11/12 15:21, Stefano Stabellini wrote: > > Get the address of the GIC distributor, cpu, virtual and virtual cpu > > interfaces registers from device tree. > > The original intention of the early DTB parsing was to get the minimum > of info needed before the DTB could be translated into a more useful > form (similar to what Linux does). > > Is this something that is still being considered in the long term? If > so, should the GIC be initialized from this translated form, instead of > using the early parsing?Regardless of the final form of DT parsing in Xen, I think that CPU, memory and GIC are the three things that should be parsed early.> > 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@eu.citrix.com> > > > [...] > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > > index da0af77..79b1dd1 100644 > > --- a/xen/common/device_tree.c > > +++ b/xen/common/device_tree.c > > @@ -54,6 +54,27 @@ bool_t device_tree_type_matches(const void *fdt, int node, const char *match) > > return !strncmp(prop, match, len); > > } > > > > +bool_t device_tree_node_compatible(const void *fdt, int node, const char *match) > > +{ > > + int len, l; > > + const void *prop; > > + > > + prop = fdt_getprop(fdt, node, "compatible", &len); > > + if ( prop == NULL ) > > + return 0; > > + > > + while ( len > 0 ) { > > + if ( !strncmp(prop, match, strlen(match)) ) > > + return 1; > > + l = strlen(prop) + 1; > > + prop += l; > > + len -= l; > > + } > > This doesn''t look right. Don''t you need to split the prop string on > comma boundaries?Nope, the compatible string to match is, for example, the entirety of "arm,cortex-a15-gic", or alternatively "arm,cortex-a9-gic". Trying to match just "cortex-a15-gic" would be incorrect. This is what Linux does too.> > + return 0; > > +} > > + > > + > > static void __init get_val(const u32 **cell, u32 cells, u64 *val) > > { > > *val = 0; > > @@ -270,6 +291,50 @@ static void __init process_cpu_node(const void *fdt, int node, > > cpumask_set_cpu(start, &cpu_possible_map); > > } > > > > +static void __init process_gic_node(const void *fdt, int node, > > + const char *name, > > + u32 address_cells, u32 size_cells) > > +{ > > + const struct fdt_property *prop; > > + const u32 *cell; > > + paddr_t start, size; > > + int reg_cells, interfaces; > > + > > + if ( address_cells < 1 || size_cells < 1 ) > > + { > > + early_printk("fdt: node `%s'': invalid #address-cells or #size-cells", > > + name); > > + return; > > + } > > + > > + prop = fdt_get_property(fdt, node, "reg", NULL); > > + if ( !prop ) > > + { > > + early_printk("fdt: node `%s'': missing `reg'' property\n", name); > > + return; > > + } > > + > > + cell = (const u32 *)prop->data; > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > > Is this needed? This cell is reread below.Yes, because device_tree_get_reg increments the cell pointer> > + > > + cell = (const u32 *)prop->data; > > + reg_cells = address_cells + size_cells; > > + interfaces = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32)); > > Perhaps wrap this in a helper function? Look like it might be useful > elsewhere.OK> > + if ( interfaces < 4 ) > > + { > > + early_printk("fdt: node `%s'': invalid `reg'' property\n", name); > > + return; > > + } > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > > + early_info.gic.gic_dist_addr = start; > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > > + early_info.gic.gic_cpu_addr = start; > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > > + early_info.gic.gic_hyp_addr = start; > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > > + early_info.gic.gic_vcpu_addr = start; > > Is the GIC driver still hardcoding the register region sizes? Or does > it not need the size?Yes, it is. However we do know the size of all the GIC interfaces because they are specified in the GIC docs.> > +} > > + > > static int __init early_scan_node(const void *fdt, > > int node, const char *name, int depth, > > u32 address_cells, u32 size_cells, > > @@ -279,6 +344,8 @@ static int __init early_scan_node(const void *fdt, > > process_memory_node(fdt, node, name, address_cells, size_cells); > > else if ( device_tree_type_matches(fdt, node, "cpu") ) > > process_cpu_node(fdt, node, name, address_cells, size_cells); > > + else if ( device_tree_node_compatible(fdt, node, "arm,cortex-a15-gic") ) > > + process_gic_node(fdt, node, name, address_cells, size_cells); > > Long term, I don''t think you want a long list of all the different > interrupt controllers here.Well, in practice there is just one interrupt controller in the ARMv7 and ARMv8 worlds right now, that is the GIC. I don''t think that''s going to change anytime soon.
On Fri, 2012-11-30 at 12:25 +0000, Stefano Stabellini wrote:> > > > > + if ( interfaces < 4 ) > > > + { > > > + early_printk("fdt: node `%s'': invalid `reg'' property\n", > name); > > > + return; > > > + } > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > &size); > > > + early_info.gic.gic_dist_addr = start; > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > &size); > > > + early_info.gic.gic_cpu_addr = start; > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > &size); > > > + early_info.gic.gic_hyp_addr = start; > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > &size); > > > + early_info.gic.gic_vcpu_addr = start; > > > > Is the GIC driver still hardcoding the register region sizes? Or > does > > it not need the size? > > Yes, it is. However we do know the size of all the GIC interfaces > because they are specified in the GIC docs.Perhaps we ought to check that the size given meets (either == or >=) our expectations? Ian.
On Fri, 30 Nov 2012, Ian Campbell wrote:> On Fri, 2012-11-30 at 12:25 +0000, Stefano Stabellini wrote: > > > > > > > > + if ( interfaces < 4 ) > > > > + { > > > > + early_printk("fdt: node `%s'': invalid `reg'' property\n", > > name); > > > > + return; > > > > + } > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > &size); > > > > + early_info.gic.gic_dist_addr = start; > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > &size); > > > > + early_info.gic.gic_cpu_addr = start; > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > &size); > > > > + early_info.gic.gic_hyp_addr = start; > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > &size); > > > > + early_info.gic.gic_vcpu_addr = start; > > > > > > Is the GIC driver still hardcoding the register region sizes? Or > > does > > > it not need the size? > > > > Yes, it is. However we do know the size of all the GIC interfaces > > because they are specified in the GIC docs. > > Perhaps we ought to check that the size given meets (either == or >=) > our expectations?OK, in that case I am going to merge the patch with the checks with the original
On 30/11/12 12:25, Stefano Stabellini wrote:> On Fri, 30 Nov 2012, David Vrabel wrote: >> On 23/11/12 15:21, Stefano Stabellini wrote: >>> Get the address of the GIC distributor, cpu, virtual and virtual cpu >>> interfaces registers from device tree. >> >> The original intention of the early DTB parsing was to get the minimum >> of info needed before the DTB could be translated into a more useful >> form (similar to what Linux does). >> >> Is this something that is still being considered in the long term? If >> so, should the GIC be initialized from this translated form, instead of >> using the early parsing? > > Regardless of the final form of DT parsing in Xen, I think that CPU, > memory and GIC are the three things that should be parsed early.Ok.>>> +bool_t device_tree_node_compatible(const void *fdt, int node, const char *match) >>> +{ >>> + int len, l; >>> + const void *prop; >>> + >>> + prop = fdt_getprop(fdt, node, "compatible", &len); >>> + if ( prop == NULL ) >>> + return 0; >>> + >>> + while ( len > 0 ) { >>> + if ( !strncmp(prop, match, strlen(match)) ) >>> + return 1; >>> + l = strlen(prop) + 1; >>> + prop += l; >>> + len -= l; >>> + } >> >> This doesn''t look right. Don''t you need to split the prop string on >> comma boundaries? > > Nope, the compatible string to match is, for example, the entirety of > "arm,cortex-a15-gic", or alternatively "arm,cortex-a9-gic". Trying to > match just "cortex-a15-gic" would be incorrect. > This is what Linux does too.Yes, you''re right. I was misrembering the format.>>> + cell = (const u32 *)prop->data; >>> + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); >> >> Is this needed? This cell is reread below. > > Yes, because device_tree_get_reg increments the cell pointerPerhaps I wasn''t clear. The result of this device_tree_get_reg() is not used anywhere, and the side effect of advancing cell is also not used.>>> + >>> + cell = (const u32 *)prop->data; >>> + reg_cells = address_cells + size_cells; >>> + interfaces = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));David
On Fri, 30 Nov 2012, Stefano Stabellini wrote:> On Fri, 30 Nov 2012, Ian Campbell wrote: > > On Fri, 2012-11-30 at 12:25 +0000, Stefano Stabellini wrote: > > > > > > > > > > > + if ( interfaces < 4 ) > > > > > + { > > > > > + early_printk("fdt: node `%s'': invalid `reg'' property\n", > > > name); > > > > > + return; > > > > > + } > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > &size); > > > > > + early_info.gic.gic_dist_addr = start; > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > &size); > > > > > + early_info.gic.gic_cpu_addr = start; > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > &size); > > > > > + early_info.gic.gic_hyp_addr = start; > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > &size); > > > > > + early_info.gic.gic_vcpu_addr = start; > > > > > > > > Is the GIC driver still hardcoding the register region sizes? Or > > > does > > > > it not need the size? > > > > > > Yes, it is. However we do know the size of all the GIC interfaces > > > because they are specified in the GIC docs. > > > > Perhaps we ought to check that the size given meets (either == or >=) > > our expectations? > > OK, in that case I am going to merge the patch with the checks with the > originalActually I found an issue in the GIC DT specs: - reg : Specifies base physical address(s) and size of the GIC registers. The first region is the GIC distributor register base and size. The 2nd region is the GIC cpu interface register base and size. however this is what I get for vexpress: reg = <0 0x2c001000 0 0x1000>, <0 0x2c002000 0 0x1000>, <0 0x2c004000 0 0x2000>, <0 0x2c006000 0 0x2000>; Leaving aside the last two regions that are due to the virtualization extensions, the size of the cpu interface is one page according to DT. However the GIC specs state that the GICC_DIR register is at offset 0x1000, so it is certainly slightly bigger than one page. I think that nobody noticed this in the Linux world because they don''t seem to be using the GICC_DIR register at all so they can get away with a cpu interface of just one page. So I think we should not check for sizes :)
On Fri, 2012-11-30 at 14:02 +0000, Stefano Stabellini wrote:> On Fri, 30 Nov 2012, Stefano Stabellini wrote: > > On Fri, 30 Nov 2012, Ian Campbell wrote: > > > On Fri, 2012-11-30 at 12:25 +0000, Stefano Stabellini wrote: > > > > > > > > > > > > > > + if ( interfaces < 4 ) > > > > > > + { > > > > > > + early_printk("fdt: node `%s'': invalid `reg'' property\n", > > > > name); > > > > > > + return; > > > > > > + } > > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > > &size); > > > > > > + early_info.gic.gic_dist_addr = start; > > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > > &size); > > > > > > + early_info.gic.gic_cpu_addr = start; > > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > > &size); > > > > > > + early_info.gic.gic_hyp_addr = start; > > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > > &size); > > > > > > + early_info.gic.gic_vcpu_addr = start; > > > > > > > > > > Is the GIC driver still hardcoding the register region sizes? Or > > > > does > > > > > it not need the size? > > > > > > > > Yes, it is. However we do know the size of all the GIC interfaces > > > > because they are specified in the GIC docs. > > > > > > Perhaps we ought to check that the size given meets (either == or >=) > > > our expectations? > > > > OK, in that case I am going to merge the patch with the checks with the > > original > > Actually I found an issue in the GIC DT specs: > > - reg : Specifies base physical address(s) and size of the GIC registers. The > first region is the GIC distributor register base and size. The 2nd region is > the GIC cpu interface register base and size. > > however this is what I get for vexpress: > > reg = <0 0x2c001000 0 0x1000>, > <0 0x2c002000 0 0x1000>, > <0 0x2c004000 0 0x2000>, > <0 0x2c006000 0 0x2000>; > > Leaving aside the last two regions that are due to the virtualization > extensions, the size of the cpu interface is one page according to DT. > However the GIC specs state that the GICC_DIR register is at offset > 0x1000, so it is certainly slightly bigger than one page.I think, and I''m not at all sure about this, that 0x1000 is GIC v1 and the second page of GICC stuff was introduced with GIC v2. I wonder what version the cortex a9 implemented. This could well be a meaningful difference between compat = arm,cortex-a9-gic and arm,cortex-a15-gic which isn''t current expressed in the DT?> I think that nobody noticed this in the Linux world because they don''t > seem to be using the GICC_DIR register at all so they can get away with > a cpu interface of just one page. > > So I think we should not check for sizes :)
On Fri, 30 Nov 2012, David Vrabel wrote:> >>> + cell = (const u32 *)prop->data; > >>> + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > >> > >> Is this needed? This cell is reread below. > > > > Yes, because device_tree_get_reg increments the cell pointer > > Perhaps I wasn''t clear. The result of this device_tree_get_reg() is not > used anywhere, and the side effect of advancing cell is also not used.You are right, I am not sure how it ended up there, I''ll remove those two lines. Thanks!
On Fri, 30 Nov 2012, Ian Campbell wrote:> On Fri, 2012-11-30 at 14:02 +0000, Stefano Stabellini wrote: > > On Fri, 30 Nov 2012, Stefano Stabellini wrote: > > > On Fri, 30 Nov 2012, Ian Campbell wrote: > > > > On Fri, 2012-11-30 at 12:25 +0000, Stefano Stabellini wrote: > > > > > > > > > > > > > > > > > + if ( interfaces < 4 ) > > > > > > > + { > > > > > > > + early_printk("fdt: node `%s'': invalid `reg'' property\n", > > > > > name); > > > > > > > + return; > > > > > > > + } > > > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > > > &size); > > > > > > > + early_info.gic.gic_dist_addr = start; > > > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > > > &size); > > > > > > > + early_info.gic.gic_cpu_addr = start; > > > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > > > &size); > > > > > > > + early_info.gic.gic_hyp_addr = start; > > > > > > > + device_tree_get_reg(&cell, address_cells, size_cells, &start, > > > > > &size); > > > > > > > + early_info.gic.gic_vcpu_addr = start; > > > > > > > > > > > > Is the GIC driver still hardcoding the register region sizes? Or > > > > > does > > > > > > it not need the size? > > > > > > > > > > Yes, it is. However we do know the size of all the GIC interfaces > > > > > because they are specified in the GIC docs. > > > > > > > > Perhaps we ought to check that the size given meets (either == or >=) > > > > our expectations? > > > > > > OK, in that case I am going to merge the patch with the checks with the > > > original > > > > Actually I found an issue in the GIC DT specs: > > > > - reg : Specifies base physical address(s) and size of the GIC registers. The > > first region is the GIC distributor register base and size. The 2nd region is > > the GIC cpu interface register base and size. > > > > however this is what I get for vexpress: > > > > reg = <0 0x2c001000 0 0x1000>, > > <0 0x2c002000 0 0x1000>, > > <0 0x2c004000 0 0x2000>, > > <0 0x2c006000 0 0x2000>; > > > > Leaving aside the last two regions that are due to the virtualization > > extensions, the size of the cpu interface is one page according to DT. > > However the GIC specs state that the GICC_DIR register is at offset > > 0x1000, so it is certainly slightly bigger than one page. > > I think, and I''m not at all sure about this, that 0x1000 is GIC v1 and > the second page of GICC stuff was introduced with GIC v2.That''s right> I wonder what version the cortex a9 implemented. This could well be a > meaningful difference between compat = arm,cortex-a9-gic and > arm,cortex-a15-gic which isn''t current expressed in the DT?yes, I think the DT hasn''t been updated