[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [PATCH V2 1/2] ARM: PL061: Clear PL061 device state a
From: |
Wei Huang |
Subject: |
Re: [Qemu-trivial] [PATCH V2 1/2] ARM: PL061: Clear PL061 device state after reset |
Date: |
Wed, 17 Feb 2016 11:34:05 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 |
On 02/16/2016 08:39 AM, Peter Maydell wrote:
> On 16 February 2016 at 14:35, Peter Maydell <address@hidden> wrote:
>> On 1 February 2016 at 20:49, Wei Huang <address@hidden> wrote:
>>> Current QEMU doesn't clear PL061 state after reset. This causes a
>>> weird issue with guest reboot via GPIO. Here is the device state
>>> description with two reboot requests:
>
>>
>> These reset values are all OK...
>>
>>> +
>>> +static void pl061_state_reset(DeviceState *dev)
>>> +{
>>> + PL061State *s = PL061(dev);
>>> +
>>> + pl061_reset(s);
>>> }
>>
>> ...but you don't need to have this wrapper function.
>> You can just do the reset in a function called pl061_reset()
>> with the function signature we need for dc->reset.
>> The only place that currently calls the existing pl061_reset()
>> is the device's init function, and you can delete that call
>> because the Device framework automatically calls the dc->reset
>> function after device initialization.
>
> I know this patch doesn't (by itself) fix the issues with guest
> reboot, but I think it is worth having anyway because not resetting
> the PL061 state is a genuine bug. Can you do a v3 and resend, please?
>
> PS: please could you include a cover letter email next time round,
> since this is a multi patch series?
Done, please review.
>
> Side note: half our "PL061" behaviour is actually specific
> to the TI variant in the Luminary, and for our plain old PL061
> we ought to restrict access to the registers that are Stellaris
> only. But that's a different bug and not a very major one.
Thanks for your suggestion. I was trying to fix it. The plan was to add
a new field rsvd_addr in "struct PL061State". Then in pl061_read() and
pl061_write(), we can check offset against [rsvd_addr, 0xfcc] (ignored
if inside).
While I was working on it, I realized that this is a benign issue. It is
true that PL061 device can access Luminary registers in the reserved
memory area. However QEMU doesn't use these Luminary registers anywhere
else other than pl061_read() and pl061_write(). It basically passes the
read/write requests through. I don't see a malicious driver can damage
device state. Thoughts?
Thanks,
-Wei
>
> thanks
> -- PMM
>