Re: [Qemu-devel] [PATCH 0/8] [PATCH RFC v3] s390 cpu hotplug

[Andreas Färber]

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