On 8/16/21 11:36 AM, Peter Maydell wrote:
> On Mon, 16 Aug 2021 at 10:32, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
>>
>> On 8/16/21 11:05 AM, Peter Maydell wrote:
>>> On Sun, 15 Aug 2021 at 17:32, Philippe Mathieu-Daudé <f4...@amsat.org>
>>> wrote:
>>>> I only wonder if we shouldn't check clock_is_enabled() here.
>>>> Maybe not assert, but at least report a GUEST_ERROR?
>>>
>>> Setting the multiplier on a disabled clock doesn't seem like
>>> an error to me. I would expect a common way for the guest to
>>> program a clock-controller would be "first set the divider
>>> and any other parameters; finally, enable the clock".
>>
>> Eh sorry I meant the other way around :/ It is usually either
>> illegal or undefined behavior on real hw to change a clock scale
>> while it is active. Personally I'd be interested to catch guests
>> doing so. I was thinking of:
>>
>> if (clock_is_enabled(clk)) {
>> qemu_log_mask(LOG_GUEST_ERROR,
>> "Changing scale of ENABLED clock '%s'\n'",
>> CLOCK_PATH(clk));
>> }
>
> I think if particular clock-controller hardware has that
> restriction we should be logging guest errors there. (Doing that
> also has the advantage that we can make the error clearer by being
> specific about what guest hardware register/device is being
> mis-programmed.)
OK.
> I don't think we can be certain enough that it's
> always wrong to change the divider on a running clock to put the error
> in the common clock API code.
OK.
> (Among other things, I suspect a warning
> here would be easy to trigger incorrectly when connecting up hard-wired
> clock dividers at startup.)
Yes, probably.
Thanks for the clarifications.
Preferably with a trace-event:
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
