See inline. On Thursday, September 17, 2015, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 17 September 2015 at 19:18, Shlomo Pongratz <shlomopongr...@gmail.com > <javascript:;>> wrote: > > On Thursday, September 17, 2015, Peter Maydell <peter.mayd...@linaro.org > <javascript:;>> > >> 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 don't have the GICv3 spec at home only the GIC 500 which is based on it and is what implemented also in HW and it states: 1.1 About the GIC-500 The GIC-500 is a build-time configurable interrupt controller that supports up to 128 cores. I'll send the GICv3 reference next week when I'll come back to work (I have it only at work). > 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 >
