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

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

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