Re: [Qemu-devel] [PATCH v2 4/4] zynq_slcr: Implement CPU reset and halting

[Peter Crosthwaite]

On Tue, Oct 9, 2012 at 11:52 PM, Peter Maydell <address@hidden> wrote:
> On 9 October 2012 14:38, Peter Crosthwaite
> <address@hidden> wrote:
>> On Tue, Oct 9, 2012 at 9:41 PM, Peter Maydell <address@hidden> wrote:
>>> We don't support per-CPU reset right now, and I don't think this is the
>>> right approach. This external device shouldn't be reaching into the
>>> implementation of the CPU object like this.
>>>
>>
>> cpu reset is part of the QOM interface for TYPE_CPU. Theres no
>> privatisation of that so disagree that this is "reaching into the
>> implementation".
>
> You're modifying s->cpus[i]->env.halted ...
>

My understanding is this is tangled with andreas's CPU refactorings:

http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg02882.html

And once that issue is surmounted we should be able to add APIs to do
this on the TYPE_CPU level rather than the TYPE_ARM_CPU level.

> Also, the QOM cpu reset function just does an instantaneous reset
> of the CPU. What you actually want is "while I assert this then
> please hold the CPU in reset indefinitely", which is functionality
> we don't expose at the moment.
>

I implemented this my just holding the halt while the reset bit is
asserted. Is this inaccurate?

>>> My suggestion would be to expose an inbound GPIO line on the CPU object
>>> (corresponding to the hardware's per-CPU reset lines) and then have an
>>> appropriate implementation inside the CPU.
>>
>> This would be awkward, considering the standard GPIO functionality
>> comes from TYPE_DEVICE, which is not a parent class of TYPE_ARM_CPU.
>> Would have to add GPIOs directly as properties. Does
>> object_property_set_gpio of something usable for that even exist?
>>

This is still my bigger concern. Can we actually do the GPIO thing
without major infrastructural developments?

>> Modelling internal signal as a GPIO is a slipperly slope as well. All
>> over QEMU there are assumptions made about what device connects to
>> what which abstract away the signalling layer, and I dont see how a
>> reset connection is different. We dont model an I2C bus as a wiggling
>> wire, so why do we have to model CPU resets like that?
>
> We do (or should) model an I2C bus as a connection between two
> QEMU objects, where the connection is made by the board model
> as part of its initialisation. Similarly, we should model CPU
> reset as a connection between two QEMU objects (the power controller
> and the CPU) which is made by the board model as part of its
> initialisation.
>

But the machine model does make the connection in this case as well.
Just using QOM links rather than GPIOs. The Machine model explicitly
sets the link (using object_property_set_link()) from the Zynq SLCR to
the CPU. The SLCR can be connected to any CPU. This also means that
when we come to implement the halting functionality, the interconnect
is handled as one link an not 2 GPIOs (or N gpios depending on how
many CPU control/status signals you want to hookup).

Regards,
Peter

> I2C is more complicated and so you want to bundle things up
> into a more abstracted connection rather than modelling things
> at the single-wire level. CPU reset really is a single wire though.
>
>>> (In general QEMU doesn't really
>>> handle reset very cleanly IMHO.)
>>>
>>> Not sure how much sense it makes to model the 'stop the CPU clock' stuff
>>> within QEMU -- probably depends whether there are any programmer-visible
>>> consequences.
>>>
>>
>> Theres a race condition in my test vectors if the CPU immediately runs
>> after the reset. CPU0 holds CPU1 in reset, then rewrites the vector
>> table while holding reset. If CPU1 kicks immediately then it will use
>> out an out of date reset vector.
>
> Hmm, shouldn't CPU1 only start executing from the reset vector
> when we come out of reset? (There is an issue like this for M
> profile cores I expect, since they read the PC from the vector
> table rather than just jumping into the vector table like A
> profile cores.)
>
> But either way the implementation of the reset gpio input
> should be that while it's asserted we hold the CPU in reset,
> and it doesn't execute until the GPIO line goes low.
>
> -- PMM