Re: [PATCH v3 13/16] xen/arm: Add support for GIC v3

[Julien Grall <julien.grall@linaro.org>]

On 04/17/2014 12:00 PM, Vijay Kilari wrote:
> On Thu, Apr 17, 2014 at 3:27 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> On 04/15/2014 12:17 PM, vijay.kilari@gmail.com wrote:
>>> +#define GICD_CTLR       (0x000)
>>> +#define GICD_TYPER      (0x004)
>>> +#define GICD_IIDR       (0x008)
>>> +#define GICD_STATUSR    (0x010)
>>> +#define GICD_SETSPI_NSR (0x040)
>>> +#define GICD_CLRSPI_NSR (0x048)
>>> +#define GICD_SETSPI_SR  (0x050)
>>> +#define GICD_CLRSPI_SR  (0x058)
>>> +#define GICD_IGROUPR    (0x080)
>>> +#define GICD_IGROUPRN   (0x0FC)
>>> +#define GICD_ISENABLER  (0x100)
>>> +#define GICD_ISENABLERN (0x17C)
>>> +#define GICD_ICENABLER  (0x180)
>>> +#define GICD_ICENABLERN (0x1fC)
>>> +#define GICD_ISPENDR    (0x200)
>>> +#define GICD_ISPENDRN   (0x27C)
>>> +#define GICD_ICPENDR    (0x280)
>>> +#define GICD_ICPENDRN   (0x2FC)
>>> +#define GICD_ISACTIVER  (0x300)
>>> +#define GICD_ISACTIVERN (0x37C)
>>> +#define GICD_ICACTIVER  (0x380)
>>> +#define GICD_ICACTIVERN (0x3FC)
>>> +#define GICD_IPRIORITYR (0x400)
>>> +#define GICD_IPRIORITYRN (0x7F8)
>>> +#define GICD_ICFGR      (0xC00)
>>> +#define GICD_ICFGRN     (0xCFC)
>>> +#define GICD_NSACR      (0xE00)
>>> +#define GICD_NSACRN     (0xEFC)
>>> +#define GICD_SGIR       (0xF00)
>>> +#define GICD_CPENDSGIR  (0xF10)
>>> +#define GICD_CPENDSGIRN (0xF1C)
>>> +#define GICD_SPENDSGIR  (0xF20)
>>> +#define GICD_SPENDSGIRN (0xF2C)
>>> +#define GICD_IROUTER    (0x6000)
>>> +#define GICD_IROUTERN   (0x7FF8)
>>> +#define GICD_PIDR0      (0xFFE0)
>>> +#define GICD_PIDR2      (0xFFE8)
>>> +#define GICD_PIDR7      (0xFFDC)
>>
>> Most of this registers are the same as GICv2 except /4 right?
>>
>> If so, it might be interesting to drop the /4 in the GICv2 so we will be
>> able to share most of the VGIC distr code.
>>
> Few registers are common. There are additional registers in V3 like
> IROUTER, PIDR.

You have some strange conception of few :). About 20 registers are
common with GICv2.

> The GICv2 defines are also used by gic-v2 driver as well which expects
> register_address/4.
> So changing this impacts existing GIC v2 driver as well.

I would prefer to modify GIC v2 driver rather than forking too much the
VGIC code. The change in GIC v2 is not so difficult...

It will save us lots of debugging time later. If we keep your solution,
if we find a bug in the vGICv2 distributor we have to check vGICv3
{re,distributor}. That would mean fixing the same bug in 3 places...

With my solution, you will be able to handle in common code the
registers valid on both configuration. Then calling vgic callbacks for
GICv3 specific registers.

It will also help to support vGICv2 on GICv3.

-- 
Julien Grall