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

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

[Wei Huang]

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:

  (PL061State fields)           data   old_in_data   istate
VM boot                         0      0             0
After 1st ACPI reboot request   8      8             8
After VM PL061 driver ACK       8      8             0
After VM reboot                 8      8             0
------------------------------------------------------------
2nd ACPI reboot request         8

In the second reboot request above, because old_in_data field is 8,
QEMU decides that there is a pending edge IRQ already (see
pl061_update()) in input; so it doesn't raise up IRQ again. As a result
the second reboot request is lost. The correct way is to clear PL061
device state after reset.

NOTE: The reset state is found from the following documentation:
 - PL061 Technical Reference Manual
 - Stellaris LM3S8962 Microcontroller Data Sheet
 - Stellaris LM3S5P31 Microcontroller Data Sheet

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/gpio/pl061.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index e5a696e..342a70d 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -284,8 +284,35 @@ static void pl061_write(void *opaque, hwaddr offset,
 
 static void pl061_reset(PL061State *s)
 {
-  s->locked = 1;
-  s->cr = 0xff;
+    /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */
+    s->data = 0;
+    s->old_out_data = 0;
+    s->old_in_data = 0;
+    s->dir = 0;
+    s->isense = 0;
+    s->ibe = 0;
+    s->iev = 0;
+    s->im = 0;
+    s->istate = 0;
+    s->afsel = 0;
+    s->dr2r = 0xff;
+    s->dr4r = 0;
+    s->dr8r = 0;
+    s->odr = 0;
+    s->pur = 0;
+    s->pdr = 0;
+    s->slr = 0;
+    s->den = 0;
+    s->locked = 1;
+    s->cr = 0xff;
+    s->amsel = 0;
+}
+
+static void pl061_state_reset(DeviceState *dev)
+{
+    PL061State *s = PL061(dev);
+
+    pl061_reset(s);
 }
 
 static void pl061_set_irq(void * opaque, int irq, int level)
@@ -343,6 +370,7 @@ static void pl061_class_init(ObjectClass *klass, void *data)
 
     k->init = pl061_initfn;
     dc->vmsd = &vmstate_pl061;
+    dc->reset = &pl061_state_reset;
 }
 
 static const TypeInfo pl061_info = {
-- 
1.8.3.1

[Qemu-devel] [PATCH V2 2/2] ARM: PL061: Cleaning field of PL061 device state

[Wei Huang]

This patch removes the float_high field of PL061State, which doesn't
seem to be used anywhere. Because this changes the device state, the
version ID is also bumped up for the reason of compatiblity.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/gpio/pl061.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 342a70d..5e2abe6 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -56,7 +56,6 @@ typedef struct PL061State {
     uint32_t slr;
     uint32_t den;
     uint32_t cr;
-    uint32_t float_high;
     uint32_t amsel;
     qemu_irq irq;
     qemu_irq out[8];
@@ -65,8 +64,8 @@ typedef struct PL061State {
 
 static const VMStateDescription vmstate_pl061 = {
     .name = "pl061",
-    .version_id = 3,
-    .minimum_version_id = 3,
+    .version_id = 4,
+    .minimum_version_id = 4,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(locked, PL061State),
         VMSTATE_UINT32(data, PL061State),
@@ -88,7 +87,6 @@ static const VMStateDescription vmstate_pl061 = {
         VMSTATE_UINT32(slr, PL061State),
         VMSTATE_UINT32(den, PL061State),
         VMSTATE_UINT32(cr, PL061State),
-        VMSTATE_UINT32(float_high, PL061State),
         VMSTATE_UINT32_V(amsel, PL061State, 2),
         VMSTATE_END_OF_LIST()
     }
-- 
1.8.3.1

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

[Shannon Zhao]

Hi Wei,

This still has a problem as I said before.

If we execute "virsh reboot xxx" during VM booting(i.e. before the PL061
driver loaded), then after VM booting, the VM will not have any reaction
to the "virsh reboot xxx".

On 2016/2/2 4:49, Wei Huang 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:
> 
>   (PL061State fields)           data   old_in_data   istate
> VM boot                         0      0             0
> After 1st ACPI reboot request   8      8             8
> After VM PL061 driver ACK       8      8             0
> After VM reboot                 8      8             0
> ------------------------------------------------------------
> 2nd ACPI reboot request         8
> 
> In the second reboot request above, because old_in_data field is 8,
> QEMU decides that there is a pending edge IRQ already (see
> pl061_update()) in input; so it doesn't raise up IRQ again. As a result
> the second reboot request is lost. The correct way is to clear PL061
> device state after reset.
> 
> NOTE: The reset state is found from the following documentation:
>  - PL061 Technical Reference Manual
>  - Stellaris LM3S8962 Microcontroller Data Sheet
>  - Stellaris LM3S5P31 Microcontroller Data Sheet
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/gpio/pl061.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
> index e5a696e..342a70d 100644
> --- a/hw/gpio/pl061.c
> +++ b/hw/gpio/pl061.c
> @@ -284,8 +284,35 @@ static void pl061_write(void *opaque, hwaddr offset,
>  
>  static void pl061_reset(PL061State *s)
>  {
> -  s->locked = 1;
> -  s->cr = 0xff;
> +    /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */
> +    s->data = 0;
> +    s->old_out_data = 0;
> +    s->old_in_data = 0;
> +    s->dir = 0;
> +    s->isense = 0;
> +    s->ibe = 0;
> +    s->iev = 0;
> +    s->im = 0;
> +    s->istate = 0;
> +    s->afsel = 0;
> +    s->dr2r = 0xff;
> +    s->dr4r = 0;
> +    s->dr8r = 0;
> +    s->odr = 0;
> +    s->pur = 0;
> +    s->pdr = 0;
> +    s->slr = 0;
> +    s->den = 0;
> +    s->locked = 1;
> +    s->cr = 0xff;
> +    s->amsel = 0;
> +}
> +
> +static void pl061_state_reset(DeviceState *dev)
> +{
> +    PL061State *s = PL061(dev);
> +
> +    pl061_reset(s);
>  }
>  
>  static void pl061_set_irq(void * opaque, int irq, int level)
> @@ -343,6 +370,7 @@ static void pl061_class_init(ObjectClass *klass, void *data)
>  
>      k->init = pl061_initfn;
>      dc->vmsd = &vmstate_pl061;
> +    dc->reset = &pl061_state_reset;
>  }
>  
>  static const TypeInfo pl061_info = {
> 

-- 
Shannon

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

[Peter Maydell]

On 1 February 2016 at 20:49, Wei Huang <wei@redhat.com> 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:
>
>   (PL061State fields)           data   old_in_data   istate
> VM boot                         0      0             0
> After 1st ACPI reboot request   8      8             8
> After VM PL061 driver ACK       8      8             0
> After VM reboot                 8      8             0
> ------------------------------------------------------------
> 2nd ACPI reboot request         8
>
> In the second reboot request above, because old_in_data field is 8,
> QEMU decides that there is a pending edge IRQ already (see
> pl061_update()) in input; so it doesn't raise up IRQ again. As a result
> the second reboot request is lost. The correct way is to clear PL061
> device state after reset.
>
> NOTE: The reset state is found from the following documentation:
>  - PL061 Technical Reference Manual
>  - Stellaris LM3S8962 Microcontroller Data Sheet
>  - Stellaris LM3S5P31 Microcontroller Data Sheet
>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/gpio/pl061.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
> index e5a696e..342a70d 100644
> --- a/hw/gpio/pl061.c
> +++ b/hw/gpio/pl061.c
> @@ -284,8 +284,35 @@ static void pl061_write(void *opaque, hwaddr offset,
>
>  static void pl061_reset(PL061State *s)
>  {
> -  s->locked = 1;
> -  s->cr = 0xff;
> +    /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */
> +    s->data = 0;
> +    s->old_out_data = 0;
> +    s->old_in_data = 0;
> +    s->dir = 0;
> +    s->isense = 0;
> +    s->ibe = 0;
> +    s->iev = 0;
> +    s->im = 0;
> +    s->istate = 0;
> +    s->afsel = 0;
> +    s->dr2r = 0xff;
> +    s->dr4r = 0;
> +    s->dr8r = 0;
> +    s->odr = 0;
> +    s->pur = 0;
> +    s->pdr = 0;
> +    s->slr = 0;
> +    s->den = 0;
> +    s->locked = 1;
> +    s->cr = 0xff;
> +    s->amsel = 0;
> +}

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.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V2 2/2] ARM: PL061: Cleaning field of PL061 device state

[Peter Maydell]

On 1 February 2016 at 20:49, Wei Huang <wei@redhat.com> wrote:
> This patch removes the float_high field of PL061State, which doesn't
> seem to be used anywhere. Because this changes the device state, the
> version ID is also bumped up for the reason of compatiblity.
>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/gpio/pl061.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
> index 342a70d..5e2abe6 100644
> --- a/hw/gpio/pl061.c
> +++ b/hw/gpio/pl061.c
> @@ -56,7 +56,6 @@ typedef struct PL061State {
>      uint32_t slr;
>      uint32_t den;
>      uint32_t cr;
> -    uint32_t float_high;
>      uint32_t amsel;
>      qemu_irq irq;
>      qemu_irq out[8];
> @@ -65,8 +64,8 @@ typedef struct PL061State {
>
>  static const VMStateDescription vmstate_pl061 = {
>      .name = "pl061",
> -    .version_id = 3,
> -    .minimum_version_id = 3,
> +    .version_id = 4,
> +    .minimum_version_id = 4,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(locked, PL061State),
>          VMSTATE_UINT32(data, PL061State),
> @@ -88,7 +87,6 @@ static const VMStateDescription vmstate_pl061 = {
>          VMSTATE_UINT32(slr, PL061State),
>          VMSTATE_UINT32(den, PL061State),
>          VMSTATE_UINT32(cr, PL061State),
> -        VMSTATE_UINT32(float_high, PL061State),
>          VMSTATE_UINT32_V(amsel, PL061State, 2),
>          VMSTATE_END_OF_LIST()
>      }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

On 16 February 2016 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 February 2016 at 20:49, Wei Huang <wei@redhat.com> 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?

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

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

[Wei Huang]

On 02/16/2016 08:39 AM, Peter Maydell wrote:
> On 16 February 2016 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 1 February 2016 at 20:49, Wei Huang <wei@redhat.com> 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
> 

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Wei Huang]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]



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?

Thanks,
-Wei

> 
> thanks
> -- PMM
> 

[-- Attachment #2: pl061_boundary_check.txt --]
[-- Type: text/plain, Size: 2368 bytes --]

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 5ece8b0..03a6351 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -60,6 +60,7 @@ typedef struct PL061State {
     qemu_irq irq;
     qemu_irq out[8];
     const unsigned char *id;
+    uint32_t rsvd_start; /* reserved area: [rsvd_start, 0xfcc] */
 } PL061State;
 
 static const VMStateDescription vmstate_pl061 = {
@@ -158,6 +159,9 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
     if (offset < 0x400) {
         return s->data & (offset >> 2);
     }
+    if (offset >= s->rsvd_start && offset <= 0xfcc) {
+        goto err_out;
+    }
     switch (offset) {
     case 0x400: /* Direction */
         return s->dir;
@@ -198,10 +202,12 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
     case 0x528: /* Analog mode select */
         return s->amsel;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "pl061_read: Bad offset %x\n", (int)offset);
-        return 0;
+        break;
     }
+err_out:
+    qemu_log_mask(LOG_GUEST_ERROR,
+                  "pl061_read: Bad offset %x\n", (int)offset);
+    return 0;    
 }
 
 static void pl061_write(void *opaque, hwaddr offset,
@@ -216,6 +222,9 @@ static void pl061_write(void *opaque, hwaddr offset,
         pl061_update(s);
         return;
     }
+    if (offset >= s->rsvd_start && offset <= 0xfcc) {
+        goto err_out;
+    }
     switch (offset) {
     case 0x400: /* Direction */
         s->dir = value & 0xff;
@@ -274,10 +283,14 @@ static void pl061_write(void *opaque, hwaddr offset,
         s->amsel = value & 0xff;
         break;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "pl061_write: Bad offset %x\n", (int)offset);
+        goto err_out;
     }
     pl061_update(s);
+    return;
+
+err_out:
+    qemu_log_mask(LOG_GUEST_ERROR,
+                  "pl061_write: Bad offset %x\n", (int)offset);
 }
 
 static void pl061_reset(DeviceState *dev)
@@ -347,6 +360,7 @@ static void pl061_luminary_init(Object *obj)
     PL061State *s = PL061(obj);
 
     s->id = pl061_id_luminary;
+    s->rsvd_start = 0x52c;
 }
 
 static void pl061_init(Object *obj)
@@ -354,6 +368,7 @@ static void pl061_init(Object *obj)
     PL061State *s = PL061(obj);
 
     s->id = pl061_id;
+    s->rsvd_start = 0x424;
 }
 
 static void pl061_class_init(ObjectClass *klass, void *data)

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

[Peter Maydell]

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