Re: [Xen-devel] [PATCH v2 7/7] xen/arm: Move vGIC registers on Hip04 platform

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

> 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@xxxxxxxxxx>
> > > > Signed-off-by: Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>
> > > > ---
> > > >  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

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