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

[Peter Crosthwaite]

On Tue, Oct 9, 2012 at 9:41 PM, Peter Maydell <address@hidden> wrote:
> On 4 October 2012 01:16, Peter Crosthwaite
> <address@hidden> wrote:
>> @@ -400,6 +404,20 @@ static void zynq_slcr_write(void *opaque, 
>> target_phys_addr_t offset,
>>                  goto bad_reg;
>>              }
>>              s->reset[(offset - 0x200) / 4] = val;
>> +            if (offset - 0x200 == A9_CPU * 4) { /* CPU Reset */
>> +                for (i = 0; i < NUM_CPUS && s->cpus[i]; ++i) {
>> +                    bool is_rst = val & (1 << (A9_CPU_RST_CTRL_RST_SHIFT + 
>> i));
>> +                    bool is_clkstop = val &
>> +                                    (1 << (A9_CPU_RST_CTRL_CLKSTOP_SHIFT + 
>> i));
>> +                    if (is_rst) {
>> +                        
>> CPU_GET_CLASS(CPU(s->cpus[i]))->reset(CPU(s->cpus[i]));
>> +                        DB_PRINT("resetting cpu %d\n", i);
>> +                    }
>> +                    s->cpus[i]->env.halted = is_rst || is_clkstop;
>> +                    DB_PRINT("%shalting cpu %d\n", s->cpus[i]->env.halted ?
>> +                             "" : "un", i);
>> +                }
>> +            }
>
> 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".

> 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?

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?

(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.

Regards,
Peter

> -- PMM