Re: [PATCH v2 14/15] xen/arm: Add vgic support for GIC v3

[Vijay Kilari <vijay.kilari@gmail.com>]

On Thu, Apr 10, 2014 at 4:21 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-04-10 at 16:13 +0530, Vijay Kilari wrote:
>> On Thu, Apr 10, 2014 at 3:53 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Fri, 2014-04-04 at 17:26 +0530, vijay.kilari@gmail.com wrote:
>> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> >>
>> >> Add vgic driver support for GIC v3 version.
>> >
>> > There is an awful lot of code duplication with the v2 stuff here,
>> > especially with the distributor, but even the redistrubotr stuff looks
>> > like it could share common implementation with the v2 distro for at
>> > least some functionality.
>> >
>> > I think we should investigate a sinlge driver here, or at least
>> > refactoring anything which is common into a library of helper functions
>> > which both can use.
>>
>> Yes, only distributor code is common which is just 2 functions i.e
>> read & write functions.
>> But the whole complexity is v2 gic & vgic driver is it runs with byte
>> offset that is the reason
>> why in gic_v2_defs all the register addresses definitions are divided by 4.
>> So this make it quite difficult to make common code.
>
> It sounds trivial to make this work with appropriate accessor macros to
> me.
>
> In fact I'm not sure why readl(&GICD[GICD_FOO]) won't work for you just
> as it does for v2 (e.g. type of GICD is uint32_t and GICD_FOO is /4).
>
>> With GICv2 HW constraint, I could not take it up. May be we can take
>> this as separate task.
>
> What do you mean?
>
I mean, I don't have V2 hardware to test any trivial changes that I do to
GIC v2. In all my patches, I am not making any changes to functionality of
v2 except code movement and changing names

>> >> +    case GICR_SETLPIR:
>> > I think you should either register two mmio handlers or maybe just
>> > handle this all at once in a single callback.
>> >
>>    There is only one address rdistributore base and size extracted from dt.
>> So we register one handle for this entire region and this single callback will
>> take access either GICR or SGI's GICR region.
>
> Having a single region in DT doesn't stop you from doing the same
> calculation you do now in the callback at init time and registering two
> regions.
>
SGI GICR region is per cpu, there is no single address with which
we can register a callback. So we register one callback for entire
re-distributor region and if it falls in that region we consider it
re-distributor
access.

This callback will take re-distributor stride offset and finds offset.

Also, this reduces the additional check for sgi re-distributor region

>> > I didn't see any system register handling, I suppose that is all handled
>> > by the hardware GICV stuff?
>>    For all system register accesses in Dom's, HW treats are virtual access.
>> The memory mapped GICD & GICR registers are virtualized by VGIC
>
> I was looking for some register configuration which causes EL1 accesses
> to the system registers to go to the "GICV" equivalent, but I didn't see
> it. Can you point me to it please?
>
You mean ICC_SRE_EL2 register.  GICv3 by default enables SRE bit.
> Ian.
>
>