Am 05.09.2013 15:10, schrieb Alexander Graf: > On 05.09.2013, at 15:05, Andreas Färber wrote: >> Am 05.09.2013 14:54, schrieb Alexander Graf: >>> Very simple and clean patch set. I don't think it deserves the RFC tag. >> >> Negative, see my review. If you want to fix up and queue patches 1-2 >> that's fine with me, but the others need a respin. No major blocker >> though, just some more footwork mostly related to QOM and Jason's >> shifted focus on cpu-add rather than device_add. > > Yeah, that's what I'm referring to. I've seen a lot worse patch sets at v8 > than this RFC :). > > I don't think we should apply it as is, and I'm very happy to see your review > and comment on the modeling bits :). But I try to never apply or cherry pick > RFC patches - and this set looks like he sent it with the intent of getting > it merged.
Agreed, we can continue with "PATCH v4". I was more upset about the "very simple and clean" bit after I commented on a number of unclean things to improve - mostly about doing things in different places. If you could find some time to review my two model string patches then I could supply Jason with a branch or even a pull to base on: http://patchwork.ozlabs.org/patch/272511/ http://patchwork.ozlabs.org/patch/272509/ I would also volunteer to provide a base patch for the link<> issue if there is agreement. Apart from the QOM API question this depends on the contradictory modelling of whether we allow CPU addresses 0..max_cpus as seen in this series or 0..somemax with <= max_cpus non-NULL as discussed on #zkvm. (child<s390-cpu> properties would allow to model the latter sparse address space very well, but an object can only have one parent in the hot-add case. We could of course add cpu[n] link<s390-cpu> properties as CPUs get added, but that doesn't strike me as very clean. My underlying thought is to offload the error handling to QOM so that we don't start hardcoding s/smp_cpus/max_cpus/g (or some max_cpu_address) all around ipi_states.) Btw an unanswered question: ipi_states is just pointers to CPUs currently, no further state. So what's "ipi" in the name? Will that array need to carry state beyond S390CPU someday? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
