Re: [Qemu-devel] [PATCH 2/2] pseries: Use new hook to correct reset sequence

[Andreas Färber]

Am 08.08.2012 03:45, schrieb David Gibson:
> On Wed, Aug 08, 2012 at 12:32:39AM +0200, Andreas Färber wrote:
>> Am 08.08.2012 00:02, schrieb Benjamin Herrenschmidt:
>>> On Fri, 2012-08-03 at 17:01 +0200, Andreas Färber wrote:
>>>>
>>>> I have posted a suggestion where CPU reset is triggered by "the
>>>> machine
>>>> as an abstract concept" (needs a bit of tweaking still, but the
>>>> general
>>>> idea is there).
>>>> Based on that, shouldn't it be rather easy to add a Notifier similar
>>>> to
>>>> "machine init done" that lets individual machines do post-reset setup?
>>>> I.e. not have QEMUMachine trigger and control the reset.
>>>>
>>>
>>> Note that we really want pre and post reset vs the device reset.
>>>
>>> That's why the machine should be the one in charge. The top level of the
>>> reset sequencing is -not- the CPU, it's the machine. All machines (or
>>> SoCs) have some kind of reset controller and provide facilities for
>>> resetting individual devices, busses, processor cores.... the global
>>> "system" reset (when it exists) itself might have interesting ordering
>>> or sequencing requirements.
>>>
>>> Now, to fix our immediate problem on ppc for 1.2 the hook proposed by
>>> Anthony for which David sent a patch does the job just fine, it allows
>>> us to clean out all our iommu tables before the device-reset, meaning
>>> that in-flights DMA cannot overwrite the various "files" (SLOF image
>>> etc.... that are auto-loaded via reset handlers implicitely created by
>>> load_image_targphys), and we can then do some post-initializations as
>>> well to get things ready for a restart (rebuild the device-tree, etc...)
>>
>> That's all good, except for embedded machines without such implicit
>> reset handling. It does contradict the "a machine is just a config file,
>> setting up QOM objects" concept, but I was not the one to push that! :)
>>
>> What I was thinking about however were those mentioned individual cores
>> being reset using cpu_reset(). If we want to piggy-back some
>> machine-specific register initialization for individual CPUStates then
>> QEMUMachine::reset is not going to be enough because it only gets
>> triggered for complete system reset. My suggestion was thus to just call
>> cpu_reset() in your QEMUMachine::reset and have cpu_reset() take care of
>> its initialization wherever called from. Any of these solutions are easy
>> to implement for 1.2 if agreement is reached what people want.
> 
> So, I more or less reaslied that myself and my new version of the
> reset patch (which I expect to send out later today) kind of does
> that.  I no longer do the machine specific CPU state setup from the
> QEMUMachine::reset, it's done from the per-cpu reset handler.  The
> QEMUMachine::reset just does the special setup that's only for the
> CPU0 entry conditions, which *is* specific to a full system reset (not
> that I think we can get an individual CPU reset on pseries, anyway).
> 
>> What I am missing from Anthony's side is some communication to machine
>> maintainers on the course to adopt before applying random patches. Right
>> now x86 and ppc are moving into opposite directions and arm, mips, etc.
>> maintainers may not even be aware of ongoing changes, and there's a
>> pending uc32 machine that should be reviewed in this light.
> 
> So.. having the CPU reset at the top of the tree definitely makes no
> sense - if nothing else, *which* cpu when there's more than one.

Maybe let me restate clearly what I am looking for in this discussion:

I would like a clear definition of
* what is the "normal" case, and
* what is the special case.

The special case sPAPR seems uncontroversial.

So, a bonus would be if we can have a default implementation (of
QEMUMachine::reset or whatever we end up doing) so that the average
machine does not need to fiddle with reset callbacks in
QEMUMachine::init. For example, have a machine_default_reset() as
fallback for QEMUMachine::reset == NULL that resets all CPUs (in order
of the singly linked list) and then does qemu_devices_reset()? sPAPR
would then override that default implementation by specifying its own
implementation and we could get rid of reset callbacks in an estimated
70% of QEMUMachine::init. (The less people fiddle at that level the
easier to refactor for me.) That could well be a later follow-up to your
v2, which looked okay on brief sight.

Cheers,
Andreas

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