| Message ID | 1415033196-30529-8-git-send-email-frediano.ziglio@huawei.com |
|---|---|
| State | New |
| Headers | show |
Hi Frediano, On 11/03/2014 04:46 PM, Frediano Ziglio wrote: > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com> > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com> > --- > xen/arch/arm/gic-v2.c | 15 +++++++++++++-- > xen/include/asm-arm/gic.h | 2 ++ > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index cea9edc..eb8cc19 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct domain *d, > return -FDT_ERR_XEN(ENOMEM); > > tmp = new_cells; > - dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); > - dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); > + > + if ( nr_gic_cpu_if == 16 ) > + { > + dt_set_range(&tmp, node, d->arch.vgic.dbase - HIP04_VGIC_REG_OFFSET, > + PAGE_SIZE); > + dt_set_range(&tmp, node, d->arch.vgic.cbase - HIP04_VGIC_REG_OFFSET, > + PAGE_SIZE * 2); > + } > + else > + { > + dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); > + dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); > + } > > res = fdt_property(fdt, "reg", new_cells, len); > xfree(new_cells); > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 3d2b3db..5af8201 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -147,6 +147,8 @@ > #define GICH_LR_PENDING 1 > #define GICH_LR_ACTIVE 2 > > +#define HIP04_VGIC_REG_OFFSET 0xe0000000 > + Please move this define in gic-v2.c. The header gic.h should only contains value that needs to be shared with the vgic and/or the other GIC drivers. Also, where does come from the offset? Any pointer to the documentation? Regards,
> > Hi Frediano, > > On 11/03/2014 04:46 PM, Frediano Ziglio wrote: > > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com> > > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com> > > --- > > xen/arch/arm/gic-v2.c | 15 +++++++++++++-- > > xen/include/asm-arm/gic.h | 2 ++ > > 2 files changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index > > cea9edc..eb8cc19 100644 > > --- a/xen/arch/arm/gic-v2.c > > +++ b/xen/arch/arm/gic-v2.c > > @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct > domain *d, > > return -FDT_ERR_XEN(ENOMEM); > > > > tmp = new_cells; > > - dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); > > - dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); > > + > > + if ( nr_gic_cpu_if == 16 ) > > + { > > + dt_set_range(&tmp, node, d->arch.vgic.dbase - > HIP04_VGIC_REG_OFFSET, > > + PAGE_SIZE); > > + dt_set_range(&tmp, node, d->arch.vgic.cbase - > HIP04_VGIC_REG_OFFSET, > > + PAGE_SIZE * 2); > > + } > > + else > > + { > > + dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); > > + dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); > > + } > > > > res = fdt_property(fdt, "reg", new_cells, len); > > xfree(new_cells); > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > > index 3d2b3db..5af8201 100644 > > --- a/xen/include/asm-arm/gic.h > > +++ b/xen/include/asm-arm/gic.h > > @@ -147,6 +147,8 @@ > > #define GICH_LR_PENDING 1 > > #define GICH_LR_ACTIVE 2 > > > > +#define HIP04_VGIC_REG_OFFSET 0xe0000000 > > + > > Please move this define in gic-v2.c. The header gic.h should only > contains value that needs to be shared with the vgic and/or the other > GIC drivers. > > Also, where does come from the offset? Any pointer to the documentation? > Well, I think I already sent a mail for this problem but got no reply (or I missed it). The problem came from how the DTB is in Linux and how Xen override devices in the DTB. On Linux I have / { ... soc { /* It's a 32-bit SoC. */ #address-cells = <1>; #size-cells = <1>; compatible = "simple-bus"; interrupt-parent = <&gic>; ranges = <0 0 0xe0000000 0x10000000>; gic: interrupt-controller@c01000 { compatible = "hisilicon,hip04-intc"; #interrupt-cells = <3>; #address-cells = <0>; interrupt-controller; interrupts = <1 9 0xf04>; reg = <0xc01000 0x1000>, <0xc02000 0x1000>, <0xc04000 0x2000>, <0xc06000 0x2000>; }; ... So the address of controller is 0xec01000 (see ranges in /soc and reg in /soc/interrupt-controller@c01000). Now Xen compute address as 0xec01000 (which is fine) but then when it has to provide a virtual gic it just replace the gic entry with one with reg with 0xec01000 not taking into account the range is putting the reg into. This lead kernel to think the address is 0xe0000000+0xec01000 instead of 0xe00000000+0xc01000. Now... the DTB from Linux is perfectly legal but Xen does not handle it properly. I mostly consider this as a temporary workaround (I wrote a small comment on first version). About solution to this there are different options: - put gic always in the root to to have full range without any offset; - fix reg according to range. This however could lead to extend the range; - remove all ranges (and fix all devices' reg) to have always no offsets. Regards, Frediano
On Tue, 2014-11-04 at 14:42 +0000, Frediano Ziglio wrote: > > > > Hi Frediano, > > > > On 11/03/2014 04:46 PM, Frediano Ziglio wrote: > > > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com> > > > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com> > > > --- > > > xen/arch/arm/gic-v2.c | 15 +++++++++++++-- > > > xen/include/asm-arm/gic.h | 2 ++ > > > 2 files changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index > > > cea9edc..eb8cc19 100644 > > > --- a/xen/arch/arm/gic-v2.c > > > +++ b/xen/arch/arm/gic-v2.c > > > @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct > > domain *d, > > > return -FDT_ERR_XEN(ENOMEM); > > > > > > tmp = new_cells; > > > - dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); > > > - dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); > > > + > > > + if ( nr_gic_cpu_if == 16 ) > > > + { > > > + dt_set_range(&tmp, node, d->arch.vgic.dbase - > > HIP04_VGIC_REG_OFFSET, > > > + PAGE_SIZE); > > > + dt_set_range(&tmp, node, d->arch.vgic.cbase - > > HIP04_VGIC_REG_OFFSET, > > > + PAGE_SIZE * 2); > > > + } > > > + else > > > + { > > > + dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); > > > + dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); > > > + } > > > > > > res = fdt_property(fdt, "reg", new_cells, len); > > > xfree(new_cells); > > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > > > index 3d2b3db..5af8201 100644 > > > --- a/xen/include/asm-arm/gic.h > > > +++ b/xen/include/asm-arm/gic.h > > > @@ -147,6 +147,8 @@ > > > #define GICH_LR_PENDING 1 > > > #define GICH_LR_ACTIVE 2 > > > > > > +#define HIP04_VGIC_REG_OFFSET 0xe0000000 > > > + > > > > Please move this define in gic-v2.c. The header gic.h should only > > contains value that needs to be shared with the vgic and/or the other > > GIC drivers. > > > > Also, where does come from the offset? Any pointer to the documentation? > > > > Well, > I think I already sent a mail for this problem but got no reply (or I missed it). > > The problem came from how the DTB is in Linux and how Xen override devices in the DTB. > > On Linux I have > > / { > ... > soc { > /* It's a 32-bit SoC. */ > #address-cells = <1>; > #size-cells = <1>; > compatible = "simple-bus"; > interrupt-parent = <&gic>; > ranges = <0 0 0xe0000000 0x10000000>; > > gic: interrupt-controller@c01000 { > compatible = "hisilicon,hip04-intc"; > #interrupt-cells = <3>; > #address-cells = <0>; > interrupt-controller; > interrupts = <1 9 0xf04>; > > reg = <0xc01000 0x1000>, <0xc02000 0x1000>, > <0xc04000 0x2000>, <0xc06000 0x2000>; > }; > ... > > So the address of controller is 0xec01000 (see ranges in /soc and reg in /soc/interrupt-controller@c01000). > > Now Xen compute address as 0xec01000 (which is fine) but then when it has to provide a virtual gic it just replace the gic entry with one with reg with 0xec01000 not taking into account the range is putting the reg into. This lead kernel to think the address is 0xe0000000+0xec01000 instead of 0xe00000000+0xc01000. Now... the DTB from Linux is perfectly legal but Xen does not handle it properly. I mostly consider this as a temporary workaround (I wrote a small comment on first version). > > About solution to this there are different options: > - put gic always in the root to to have full range without any offset; > - fix reg according to range. This however could lead to extend the range; > - remove all ranges (and fix all devices' reg) to have always no offsets. - Propagate the host GICC register value literally over, having mapping the GICV there. i.e. don't translate the value which is written to the DT. None of the other options sound very good to me. Ian.
> On Tue, 2014-11-04 at 14:42 +0000, Frediano Ziglio wrote: > > > > > > Hi Frediano, > > > > > > On 11/03/2014 04:46 PM, Frediano Ziglio wrote: > > > > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com> > > > > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com> > > > > --- > > > > xen/arch/arm/gic-v2.c | 15 +++++++++++++-- > > > > xen/include/asm-arm/gic.h | 2 ++ > > > > 2 files changed, 15 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index > > > > cea9edc..eb8cc19 100644 > > > > --- a/xen/arch/arm/gic-v2.c > > > > +++ b/xen/arch/arm/gic-v2.c > > > > @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct > > > domain *d, > > > > return -FDT_ERR_XEN(ENOMEM); > > > > > > > > tmp = new_cells; > > > > - dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); > > > > - dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); > > > > + > > > > + if ( nr_gic_cpu_if == 16 ) > > > > + { > > > > + dt_set_range(&tmp, node, d->arch.vgic.dbase - > > > HIP04_VGIC_REG_OFFSET, > > > > + PAGE_SIZE); > > > > + dt_set_range(&tmp, node, d->arch.vgic.cbase - > > > HIP04_VGIC_REG_OFFSET, > > > > + PAGE_SIZE * 2); > > > > + } > > > > + else > > > > + { > > > > + dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); > > > > + dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * > 2); > > > > + } > > > > > > > > res = fdt_property(fdt, "reg", new_cells, len); > > > > xfree(new_cells); > > > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm- > arm/gic.h > > > > index 3d2b3db..5af8201 100644 > > > > --- a/xen/include/asm-arm/gic.h > > > > +++ b/xen/include/asm-arm/gic.h > > > > @@ -147,6 +147,8 @@ > > > > #define GICH_LR_PENDING 1 > > > > #define GICH_LR_ACTIVE 2 > > > > > > > > +#define HIP04_VGIC_REG_OFFSET 0xe0000000 > > > > + > > > > > > Please move this define in gic-v2.c. The header gic.h should only > > > contains value that needs to be shared with the vgic and/or the > > > other GIC drivers. > > > > > > Also, where does come from the offset? Any pointer to the > documentation? > > > > > > > Well, > > I think I already sent a mail for this problem but got no reply (or > I missed it). > > > > The problem came from how the DTB is in Linux and how Xen override > devices in the DTB. > > > > On Linux I have > > > > / { > > ... > > soc { > > /* It's a 32-bit SoC. */ > > #address-cells = <1>; > > #size-cells = <1>; > > compatible = "simple-bus"; > > interrupt-parent = <&gic>; > > ranges = <0 0 0xe0000000 0x10000000>; > > > > gic: interrupt-controller@c01000 { > > compatible = "hisilicon,hip04-intc"; > > #interrupt-cells = <3>; > > #address-cells = <0>; > > interrupt-controller; > > interrupts = <1 9 0xf04>; > > > > reg = <0xc01000 0x1000>, <0xc02000 0x1000>, > > <0xc04000 0x2000>, <0xc06000 0x2000>; > > }; > > ... > > > > So the address of controller is 0xec01000 (see ranges in /soc and reg > in /soc/interrupt-controller@c01000). > > > > Now Xen compute address as 0xec01000 (which is fine) but then when it > has to provide a virtual gic it just replace the gic entry with one > with reg with 0xec01000 not taking into account the range is putting > the reg into. This lead kernel to think the address is > 0xe0000000+0xec01000 instead of 0xe00000000+0xc01000. Now... the DTB > from Linux is perfectly legal but Xen does not handle it properly. I > mostly consider this as a temporary workaround (I wrote a small comment > on first version). > > > > About solution to this there are different options: > > - put gic always in the root to to have full range without any offset; > > - fix reg according to range. This however could lead to extend the > > range; > > - remove all ranges (and fix all devices' reg) to have always no > offsets. > > - Propagate the host GICC register value literally over, having > mapping the GICV there. i.e. don't translate the value which is > written to the DT. > > None of the other options sound very good to me. > > Ian. Yes, You are right, KISS rule apply! Implemented and working correctly. Regards, Frediano
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index cea9edc..eb8cc19 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct domain *d, return -FDT_ERR_XEN(ENOMEM); tmp = new_cells; - dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); - dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); + + if ( nr_gic_cpu_if == 16 ) + { + dt_set_range(&tmp, node, d->arch.vgic.dbase - HIP04_VGIC_REG_OFFSET, + PAGE_SIZE); + dt_set_range(&tmp, node, d->arch.vgic.cbase - HIP04_VGIC_REG_OFFSET, + PAGE_SIZE * 2); + } + else + { + dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE); + dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2); + } res = fdt_property(fdt, "reg", new_cells, len); xfree(new_cells); diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 3d2b3db..5af8201 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -147,6 +147,8 @@ #define GICH_LR_PENDING 1 #define GICH_LR_ACTIVE 2 +#define HIP04_VGIC_REG_OFFSET 0xe0000000 + #ifndef __ASSEMBLY__ #include <xen/device_tree.h> #include <xen/irq.h>