Re: [Xen-devel] [PATCH] arch: arm: vgic-v3: fix GICD_ISACTIVER range

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 9 Nov 2019, Julien Grall wrote:
> On Sat, 9 Nov 2019, 04:27 Stefano Stabellini, <sstabellini@xxxxxxxxxx> wrote:
>       On Thu, 7 Nov 2019, Peng Fan wrote:
>       > The end should be GICD_ISACTIVERN not GICD_ISACTIVER.
>       >
>       > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> 
>       Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> 
> To be honest, I am not sure the code is correct. A read to those registers 
> should tell you the list of interrupts active. As we always
> return 0, this will not return the correct state of the GIC.
> 
> I know that returning the list of actives interrupts is complicated with the 
> old vGIC, but I don't think silently ignoring it is a good
> idea.
> The question here is why the guest accessed those registers? What is it 
> trying to figure out?

We are not going to solve the general problem at this stage. At the
moment the code:

- ignore the first register only
- print an error and return an IO_ABORT error for the other regs

For the inconsistency alone the second option is undesirable. Also it
doesn't match the write implementation, which does the same thing for
all the GICD_ISACTIVER* regs instead of having a special treatment for
the first one only. It looks like a typo in the original patch to me.

The proposed patch switches the behavior to:

- silently ignore all the GICD_ISACTIVER* regs (as proposed)

is an improvement.


>       Juergen, I think this fix should be in the release (and also
>       backported to stable trees.)
> 
> 
> Without an understanding of the problem, I disagree with this request (see 
> above).
> 
> As an aside, the range ISPENDR  has the same issue.

You meant GICD_ICPENDR, right? Yep, that one is suffering from the same
typo mistake too.

 
>       > ---
>       >  xen/arch/arm/vgic-v3.c | 2 +-
>       >  1 file changed, 1 insertion(+), 1 deletion(-)
>       >
>       > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>       > index 422b94f902..e802f2055a 100644
>       > --- a/xen/arch/arm/vgic-v3.c
>       > +++ b/xen/arch/arm/vgic-v3.c
>       > @@ -706,7 +706,7 @@ static int __vgic_v3_distr_common_mmio_read(const 
> char *name, struct vcpu *v,
>       >          goto read_as_zero;
>       > 
>       >      /* Read the active status of an IRQ via GICD/GICR is not 
> supported */
>       > -    case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER):
>       > +    case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
>       >      case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
>       >          goto read_as_zero;
>       > 
>       > --
>       > 2.16.4
>       >
> 
>       _______________________________________________
>       Xen-devel mailing list
>       Xen-devel@xxxxxxxxxxxxxxxxxxxx
>       https://lists.xenproject.org/mailman/listinfo/xen-devel
> 
> 
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel