[PATCH] hw/intc/arm_gicv5: Make IRS_IDR0 reads return actual value, not 0

[PATCH] hw/intc/arm_gicv5: Make IRS_IDR0 reads return actual value, not 0

[Peter Maydell]

IN the code for handling reads of IRS_IDR0, we carefully construct
the correct value starting with cs->irs_idr0 and adjusting it.
However, we never actually write the final value into *data, so
the guest will always see zero.

We didn't notice this before, because Linux happens to not care
about this register value except to check for the presence of
virtualization.

Cc: qemu-stable@nongnu.org
Fixes: 405ad2e76f13254 ("hw/intc/arm_gicv5: Implement IRS ID regs")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
index 08878193dfc..ebfb0878af3 100644
--- a/hw/intc/arm_gicv5.c
+++ b/hw/intc/arm_gicv5.c
@@ -1433,6 +1433,7 @@ static bool config_readl(GICv5 *s, GICv5Domain domain, hwaddr offset,
             /* ...which means VIRT_ONE_N is also RES0 */
             v &= ~R_IRS_IDR0_VIRT_ONE_N_MASK;
         }
+        *data = v;
         return true;
 
     case A_IRS_IDR1:
-- 
2.43.0

Re: [PATCH] hw/intc/arm_gicv5: Make IRS_IDR0 reads return actual value, not 0

[Philippe Mathieu-Daudé]

On 8/10/26 11:23, Peter Maydell wrote:
> IN the code for handling reads of IRS_IDR0, we carefully construct
> the correct value starting with cs->irs_idr0 and adjusting it.
> However, we never actually write the final value into *data, so
> the guest will always see zero.
> 
> We didn't notice this before, because Linux happens to not care
> about this register value except to check for the presence of
> virtualization.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 405ad2e76f13254 ("hw/intc/arm_gicv5: Implement IRS ID regs")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv5.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
> index 08878193dfc..ebfb0878af3 100644
> --- a/hw/intc/arm_gicv5.c
> +++ b/hw/intc/arm_gicv5.c
> @@ -1433,6 +1433,7 @@ static bool config_readl(GICv5 *s, GICv5Domain domain, hwaddr offset,
>               /* ...which means VIRT_ONE_N is also RES0 */
>               v &= ~R_IRS_IDR0_VIRT_ONE_N_MASK;
>           }

Looking at the code path it is obvious, I wonder why compilers or
coverity didn't warned 'v' is set but not used...

> +        *data = v;
>           return true;

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

Re: [PATCH] hw/intc/arm_gicv5: Make IRS_IDR0 reads return actual value, not 0

[Jim MacArthur]

On Thu, Oct 08, 2026 at 10:23:27AM +0100, Peter Maydell wrote:
> IN the code for handling reads of IRS_IDR0, we carefully construct
> the correct value starting with cs->irs_idr0 and adjusting it.
> However, we never actually write the final value into *data, so
> the guest will always see zero.
> 
> We didn't notice this before, because Linux happens to not care
> about this register value except to check for the presence of
> virtualization.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 405ad2e76f13254 ("hw/intc/arm_gicv5: Implement IRS ID regs")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv5.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
> index 08878193dfc..ebfb0878af3 100644
> --- a/hw/intc/arm_gicv5.c
> +++ b/hw/intc/arm_gicv5.c
> @@ -1433,6 +1433,7 @@ static bool config_readl(GICv5 *s, GICv5Domain domain, hwaddr offset,
>              /* ...which means VIRT_ONE_N is also RES0 */
>              v &= ~R_IRS_IDR0_VIRT_ONE_N_MASK;
>          }
> +        *data = v;
>          return true;
>  
>      case A_IRS_IDR1:

Reviewed-by: Jim MacArthur <jim.macarthur@linaro.org>

Re: [PATCH] hw/intc/arm_gicv5: Make IRS_IDR0 reads return actual value, not 0

[Jim MacArthur]

On Thu, Oct 08, 2026 at 11:36:59AM +0200, Philippe Mathieu-Daudé wrote:
> On 8/10/26 11:23, Peter Maydell wrote:
> > IN the code for handling reads of IRS_IDR0, we carefully construct
> > the correct value starting with cs->irs_idr0 and adjusting it.
> > However, we never actually write the final value into *data, so
> > the guest will always see zero.
> > 
> > We didn't notice this before, because Linux happens to not care
> > about this register value except to check for the presence of
> > virtualization.
> > 
> > Cc: qemu-stable@nongnu.org
> > Fixes: 405ad2e76f13254 ("hw/intc/arm_gicv5: Implement IRS ID regs")
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/intc/arm_gicv5.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
> > index 08878193dfc..ebfb0878af3 100644
> > --- a/hw/intc/arm_gicv5.c
> > +++ b/hw/intc/arm_gicv5.c
> > @@ -1433,6 +1433,7 @@ static bool config_readl(GICv5 *s, GICv5Domain domain, hwaddr offset,
> >               /* ...which means VIRT_ONE_N is also RES0 */
> >               v &= ~R_IRS_IDR0_VIRT_ONE_N_MASK;
> >           }
> 
> Looking at the code path it is obvious, I wonder why compilers or
> coverity didn't warned 'v' is set but not used...

AFAICT, 'param @= RHS' counts as a use with -Wunused-but-set-variable in GCC up to version 15; GCC 16.1 adds an exception for this and should warn.

Jim

Re: [PATCH] hw/intc/arm_gicv5: Make IRS_IDR0 reads return actual value, not 0

[Philippe Mathieu-Daudé]

On 9/10/26 11:48, Jim MacArthur wrote:
> On Thu, Oct 08, 2026 at 11:36:59AM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/10/26 11:23, Peter Maydell wrote:
>>> IN the code for handling reads of IRS_IDR0, we carefully construct
>>> the correct value starting with cs->irs_idr0 and adjusting it.
>>> However, we never actually write the final value into *data, so
>>> the guest will always see zero.
>>>
>>> We didn't notice this before, because Linux happens to not care
>>> about this register value except to check for the presence of
>>> virtualization.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Fixes: 405ad2e76f13254 ("hw/intc/arm_gicv5: Implement IRS ID regs")
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>    hw/intc/arm_gicv5.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c
>>> index 08878193dfc..ebfb0878af3 100644
>>> --- a/hw/intc/arm_gicv5.c
>>> +++ b/hw/intc/arm_gicv5.c
>>> @@ -1433,6 +1433,7 @@ static bool config_readl(GICv5 *s, GICv5Domain domain, hwaddr offset,
>>>                /* ...which means VIRT_ONE_N is also RES0 */
>>>                v &= ~R_IRS_IDR0_VIRT_ONE_N_MASK;
>>>            }
>>
>> Looking at the code path it is obvious, I wonder why compilers or
>> coverity didn't warned 'v' is set but not used...
> 
> AFAICT, 'param @= RHS' counts as a use with -Wunused-but-set-variable in GCC up to version 15; GCC 16.1 adds an exception for this and should warn.

Oh, thanks for the info!