Re: [Qemu-devel] [PATCH RFC V4 0/4] Implement GIC-500 from GICv3 family for arm64

[Peter Maydell]

On 17 September 2015 at 19:18, Shlomo Pongratz <address@hidden> wrote:
> On Thursday, September 17, 2015, Peter Maydell <address@hidden>
>> This still seems to have the same issues as were noted in previous
>> rounds:
>>  * you need to get rid of the limitation on number of cores. There
>>    should be no hard limit imposed by the GIC emulation code: not
>>    64, not 128, not anything.
>
>
> The GICv3 spec limits the number of cores to 128.

I don't believe it does (the only limit is the affinity fields
which have a limit up in the millions). Can you point me to the
part of the spec that you think imposes a lower limit?

> I assume you don't expect
> GICv2 to support more than 8 cores.
> As I wrote since the largest "atomic" type is 64 bit supporting more than 64
> cores requires major rewrite using bitops. This can be done but I think this
> can wait for future versions.

No, I don't want to accept a version with a limitation.
Data structures are important and we should get them right from
the start. Don't look at the GICv2 to figure out what data structures
you should have and how they should be arranged, look at the GICv3 spec.

(In particular, you probably want a data structure layout that
says "this GIC has an array of N redistributors, and each redistributor
has such-and-such state", rather than the QEMU GICv2 model's approach
of having a struct per interrupt that tries to track per-CPU state
within that struct.)

> I wonder how can I remove the limit and not crash is someone tries an
> arbitrary large number.
> How can I protect from wrong usage?

You need to allocate your data structures based on the number of CPUs
which the user passes in to you. (We do this in Pavel's patchset for
irq lines, for instance.) If the user passes in something that's so
large we can't allocate memory, then QEMU's memory allocation functions
will fail cleanly.

>>  * patch 2 is over 2000 lines, which is far too big to review. It
>>    needs to be split up. Aim for 200 lines per patch at maximum;
>>    smaller is better.
>
>
> I'm open to suggestions. This is actually a single file (with the necessary
> Makefile addition). Do you suggest that I'll break it for example to
> distributer part an redistributer part e.t.c.?
> Please advice.

Yes, you need to split it up into logical subsections that each
add a small but coherent piece of functionality. The purpose
of splitting up into patches is to make the code easier to
review for correctness. It's impossible to read a single 2000
line patch and keep it all in your head at once. So a split
into multiple patches is about presenting the changes in a
way that allows the reader to assess it in a logical order
and not too much at once.

thanks
-- PMM