[PATCH 0/2] hw/intc/arm_gicv3_kvm: two small fixes about register accesses

[PATCH 0/2] hw/intc/arm_gicv3_kvm: two small fixes about register accesses

[Zenghui Yu]

Zenghui Yu (2):
  hw/intc/arm_gicv3_kvm: Remove writes to ICPENDR registers
  hw/intc/arm_gicv3_kvm: Write all 1's to clear enable/active

 hw/intc/arm_gicv3_kvm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/intc/arm_gicv3_kvm: Remove writes to ICPENDR registers

[Zenghui Yu]

As per the arm-vgic-v3 kernel doc [1]:

    Accesses to GICD_ICPENDR register region and GICR_ICPENDR0 registers
    have RAZ/WI semantics, meaning that reads always return 0 and writes
    are always ignored.

Remove the useless writes to ICPENDR registers in kvm_arm_gicv3_put().

[1] https://docs.kernel.org/virt/kvm/devices/arm-vgic-v3.html

Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
---
 hw/intc/arm_gicv3_kvm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 8ed88e7429..f798a6e28c 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -387,8 +387,6 @@ static void kvm_arm_gicv3_put(GICv3State *s)
         reg = c->level;
         kvm_gic_line_level_access(s, 0, ncpu, &reg, true);
 
-        reg = ~0;
-        kvm_gicr_access(s, GICR_ICPENDR0, ncpu, &reg, true);
         reg = c->gicr_ipendr0;
         kvm_gicr_access(s, GICR_ISPENDR0, ncpu, &reg, true);
 
@@ -445,7 +443,7 @@ static void kvm_arm_gicv3_put(GICv3State *s)
     kvm_gic_put_line_level_bmp(s, s->level);
 
     /* s->pending bitmap -> GICD_ISPENDRn */
-    kvm_dist_putbmp(s, GICD_ISPENDR, GICD_ICPENDR, s->pending);
+    kvm_dist_putbmp(s, GICD_ISPENDR, 0, s->pending);
 
     /* s->active bitmap -> GICD_ISACTIVERn */
     kvm_dist_putbmp(s, GICD_ISACTIVER, GICD_ICACTIVER, s->active);
-- 
2.34.1

[PATCH 2/2] hw/intc/arm_gicv3_kvm: Write all 1's to clear enable/active

[Zenghui Yu]

Writing 0 to GICD_IC{ENABLE,ACTIVE}R architecturally has no effect on
interrupt status (all writes are simply ignored by KVM) and doesn't comply
with the intention of "first write to the clear-reg to clear all bits".

Write all 1's to actually clear the enable/active status.

Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
---
 hw/intc/arm_gicv3_kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index f798a6e28c..6166283cd1 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -295,7 +295,7 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
          * the 1 bits.
          */
         if (clroffset != 0) {
-            reg = 0;
+            reg = ~0;
             kvm_gicd_access(s, clroffset, &reg, true);
             clroffset += 4;
         }
-- 
2.34.1

Re: [PATCH 1/2] hw/intc/arm_gicv3_kvm: Remove writes to ICPENDR registers

[Peter Maydell]

On Tue, 29 Jul 2025 at 17:17, Zenghui Yu <zenghui.yu@linux.dev> wrote:
>
> As per the arm-vgic-v3 kernel doc [1]:
>
>     Accesses to GICD_ICPENDR register region and GICR_ICPENDR0 registers
>     have RAZ/WI semantics, meaning that reads always return 0 and writes
>     are always ignored.
>
> Remove the useless writes to ICPENDR registers in kvm_arm_gicv3_put().
>
> [1] https://docs.kernel.org/virt/kvm/devices/arm-vgic-v3.html

The kernel doesn't implement any state behind these
registers today, but that doesn't inherently mean it
will never do so or that it never did do so.
Since we have the state fields in the GICv3State struct (for the
benefit of TCG) and the kernel defines the accessors for it,
I prefer to leave this code in QEMU, and leave it up to the
kernel whether it provides any state behind them.

thanks
-- PMM

Re: [PATCH 2/2] hw/intc/arm_gicv3_kvm: Write all 1's to clear enable/active

[Peter Maydell]

On Tue, 29 Jul 2025 at 17:17, Zenghui Yu <zenghui.yu@linux.dev> wrote:
>
> Writing 0 to GICD_IC{ENABLE,ACTIVE}R architecturally has no effect on
> interrupt status (all writes are simply ignored by KVM) and doesn't comply
> with the intention of "first write to the clear-reg to clear all bits".
>
> Write all 1's to actually clear the enable/active status.
>
> Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> ---
>  hw/intc/arm_gicv3_kvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index f798a6e28c..6166283cd1 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -295,7 +295,7 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
>           * the 1 bits.
>           */
>          if (clroffset != 0) {
> -            reg = 0;
> +            reg = ~0;
>              kvm_gicd_access(s, clroffset, &reg, true);
>              clroffset += 4;
>          }

I guess given what the kernel has implemented that this
is the correct change, so on that basis
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I don't understand what the kernel ABI is trying to do here,
though...

My expectation for user access for all these registers
where there's a "set" and a "clear" register pair would
be that they behave the same way. But looking at the
implementation, GICD_ICPENDR seems to be implemented
as "reads zero, writes ignored", whereas GICD_ICACTIVER
implements the "write-1-to-clear" semantics.

This seems to match what is documented, but I don't
understand why we implemented and documented that:
https://docs.kernel.org/virt/kvm/devices/arm-vgic-v3.html
rather than a more straightforward "for userspace, you
can just read and write the state".

thanks
-- PMM

Re: [PATCH 1/2] hw/intc/arm_gicv3_kvm: Remove writes to ICPENDR registers

[Peter Maydell]

On Thu, 31 Jul 2025 at 17:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 29 Jul 2025 at 17:17, Zenghui Yu <zenghui.yu@linux.dev> wrote:
> >
> > As per the arm-vgic-v3 kernel doc [1]:
> >
> >     Accesses to GICD_ICPENDR register region and GICR_ICPENDR0 registers
> >     have RAZ/WI semantics, meaning that reads always return 0 and writes
> >     are always ignored.
> >
> > Remove the useless writes to ICPENDR registers in kvm_arm_gicv3_put().
> >
> > [1] https://docs.kernel.org/virt/kvm/devices/arm-vgic-v3.html
>
> The kernel doesn't implement any state behind these
> registers today, but that doesn't inherently mean it
> will never do so or that it never did do so.
> Since we have the state fields in the GICv3State struct (for the
> benefit of TCG) and the kernel defines the accessors for it,
> I prefer to leave this code in QEMU, and leave it up to the
> kernel whether it provides any state behind them.

Ah, having read the second patch I realise I misunderstood
this one. Yes, the ISPENDR registers are special because
the kernel implements them as "just write the state" rather
than with "first clear them via the C register and then write
the set bits via the S register". So this is correct.

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

thanks
-- PMM

Re: [PATCH 0/2] hw/intc/arm_gicv3_kvm: two small fixes about register accesses

[Peter Maydell]

On Tue, 29 Jul 2025 at 17:17, Zenghui Yu <zenghui.yu@linux.dev> wrote:
>
> Zenghui Yu (2):
>   hw/intc/arm_gicv3_kvm: Remove writes to ICPENDR registers
>   hw/intc/arm_gicv3_kvm: Write all 1's to clear enable/active
>
>  hw/intc/arm_gicv3_kvm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Did this cause any visible bugs, or did you just notice it
by code inspection ?

Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH 0/2] hw/intc/arm_gicv3_kvm: two small fixes about register accesses

[Zenghui Yu]

On 2025/8/1 01:11, Peter Maydell wrote:
> On Tue, 29 Jul 2025 at 17:17, Zenghui Yu <zenghui.yu@linux.dev> wrote:
> >
> > Zenghui Yu (2):
> >   hw/intc/arm_gicv3_kvm: Remove writes to ICPENDR registers
> >   hw/intc/arm_gicv3_kvm: Write all 1's to clear enable/active
> >
> >  hw/intc/arm_gicv3_kvm.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> Did this cause any visible bugs, or did you just notice it
> by code inspection ?

They're noticed by code inspection. I agree with that patch #2 is
guest-visible only when doing a system reset.

> 
> Applied to target-arm.next, thanks.

Thanks!

Zenghui

Re: [PATCH 2/2] hw/intc/arm_gicv3_kvm: Write all 1's to clear enable/active

[Zenghui Yu]

On 2025/8/1 00:59, Peter Maydell wrote:
> On Tue, 29 Jul 2025 at 17:17, Zenghui Yu <zenghui.yu@linux.dev> wrote:
> >
> > Writing 0 to GICD_IC{ENABLE,ACTIVE}R architecturally has no effect on
> > interrupt status (all writes are simply ignored by KVM) and doesn't comply
> > with the intention of "first write to the clear-reg to clear all bits".
> >
> > Write all 1's to actually clear the enable/active status.
> >
> > Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> > ---
> >  hw/intc/arm_gicv3_kvm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> > index f798a6e28c..6166283cd1 100644
> > --- a/hw/intc/arm_gicv3_kvm.c
> > +++ b/hw/intc/arm_gicv3_kvm.c
> > @@ -295,7 +295,7 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
> >           * the 1 bits.
> >           */
> >          if (clroffset != 0) {
> > -            reg = 0;
> > +            reg = ~0;
> >              kvm_gicd_access(s, clroffset, &reg, true);
> >              clroffset += 4;
> >          }
> 
> I guess given what the kernel has implemented that this
> is the correct change, so on that basis
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I don't understand what the kernel ABI is trying to do here,
> though...
> 
> My expectation for user access for all these registers
> where there's a "set" and a "clear" register pair would
> be that they behave the same way. But looking at the
> implementation, GICD_ICPENDR seems to be implemented
> as "reads zero, writes ignored", whereas GICD_ICACTIVER
> implements the "write-1-to-clear" semantics.
> 
> This seems to match what is documented, but I don't
> understand why we implemented and documented that:
> https://docs.kernel.org/virt/kvm/devices/arm-vgic-v3.html

I think these 2 paragraphs exactly explain the reason:

    "This is identical to the value returned by a guest read from
     ISPENDR for an edge triggered interrupt, but may differ for level

     [...]

     cannot be deduced from purely the line level and the value of the
     ISPENDR registers)."

Does it help?

Thanks,
Zenghui