Re: [Qemu-trivial] [PATCH V2 1/2] ARM: PL061: Clear PL061 device state after reset

[Peter Maydell <peter.maydell@linaro.org>]

On 17 February 2016 at 19:09, Wei Huang <wei@redhat.com> wrote:
>
>
> On 02/17/2016 11:53 AM, Peter Maydell wrote:
>> On 17 February 2016 at 17:34, Wei Huang <wei@redhat.com> wrote:
>>> On 02/16/2016 08:39 AM, Peter Maydell wrote:
>>>> 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?
>>
>> It's not a "malicious guest can do bad things" bug, it's a "modelled
>> hardware doesn't behave like the real thing" bug. A non-Luminary PL061
>> should act like the hardware, which means that the registers that don't
>> exist should be RAZ/WI (and should log guest-errors if the guest tries
>> to access them), the same way we do in the "default" case of the
>> case statements for other reserved registers.
>
> How about the attached patch? I can write a new patch based on it, or
> you prefer stashing it on top of V3 I just submitted?

Looks OK, but you don't need to check for address being <= 0xfcc
because either we've already handled the ID registers (read)
or we were going to be reserved anyway (write).

thanks
-- PMM