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

[Andreas Färber]

Am 13.09.2013 17:01, schrieb Jason J. Herne:
> On 09/05/2013 10:06 AM, Andreas Färber wrote:
>> 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.
> 
> According to http://wiki.qemu.org/Features/CPUHotplug:
> 
> "adding CPUs should be done in successive order from lower to higher IDs
> in [0..max-cpus) range.
> It's possible to add arbitrary CPUs in random order, however that would
> cause migration to fail on its target side."
> 
> Considering that, in a virtual environment, it rarely (if ever) makes
> sense to define out of order cpu ids maybe we should keep the patch as
> is and only allow consecutive cpu ids to be used.

Your previous series tried to make -device work in place of -smp. This
series now only seems to focus on cpu-add, including you referencing its
current limitations above.

As I tried to explain, x86 needed cpu-add because unlike s390x its CPU
is not yet a fully initialize'able QOM object and thus can't use
device_add yet (and long-term we want to use containers instead of
today's *-x86_64-cpu).

So I would very much prefer to see s390x continuing to use -smp but
using device_add for CPU hot-add and in a way where we don't have to
change semantics and ABI again when we implement hot-unplug. I am fine
with you implementing a cpu-add wrapper, but on top of a working
implementation please rather than limiting your implementation by it
upfront.

> By extension, hot-unplug would require that the highest id be unplugged.
> This is probably not acceptable in any type of "mixed cpu" environment
> because the greatest id may not be the cpu type you want to remove.  I'm
> not sure if S390 will implement mixed cpu types.
> 
>> (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.)
>>
> 
> I'm not sure I understand. What is meant by: "an object can only have
> one parent in the hot-add case."
> 
> What is the difference between "child<s390-cpu>" and "cpu[n]
> link<s390-cpu>"?  And why do you feel the link case would be unclean?

child<> properties determine the canonical path of an object. Each
object only has one canonical path. When using link<> properties, the
linked-to objects still need a canonical path, which becomes the string
value of the property on QMP level (pointer on C level). device_add
assigns canonical paths. Realizing a device creates a canonical path in
/machine/unassigned as fallback, but that won't work long-term as the
composition tree will be our source to find devices to realize, so
either the machine code or the S390CPU code should take care of assuring
CPUs have canonical paths before manually realizing them.

> 
>> 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?
>>
> 
> Quoting Jens Freimann:
> "
> The ipi_states array holds all our S390CPU states. The position of a cpu
> within this array equals its "cpu address". See section "CPU address
> identification"
> in the Principles of Operation. This cpu address is used for
> cpu signalling (inter-processor interrupts->ipi) via the sigp instruction.
> The cpu address does not contain information about which book this cpu is
> plugged into, nor does it hold any other topology information.
> 
> When we intercept a sigp instruction in qemu the cpu address is passed
> to us in a field of the SIE control block. We then use the cpu address
> as an index to the ipi_states field, get hold of the S390CPU and then
> for example do an vcpu ioctl on the chosen cpu.
> "
> 
> Based on this explanation I do not think this array will ever be
> anything more than what it is today.

Thanks, seems I overread that.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg