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

[Andre Przywara <andre.przywara@xxxxxxx>]

On Mon, 11 Nov 2019 11:01:07 -0800 (PST)
Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:

Hi,

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

I see Linux querying the active state (IRQCHIP_STATE_ACTIVE) at two relevant 
points for ARM:
- In kernel/irq/manage.c, in __synchronize_hardirq().
- In KVM's arch timer emulation code.

I think the latter is of no concern (yet), but the first might actually 
trigger. At the moment it's beyond me what it actually does, but maybe some IRQ 
changes (RT, threaded IRQs?) trigger this now?
 
> 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.

Yeah, I agree. Getting the actual active state of a virtual IRQ is at least 
expensive, as you have to bring all VCPUs back, to sync the LR content back 
into something Xen can access.
This is an architectural property of the GIC virtualisation, as normally the 
acknowledge happens without exiting, also the EOI, so Xen cannot know which 
state an IRQ is in while the VCPU is running. Think: Schrödinger ;-)

Regarding this patch: The original code looks indeed like a typo to me: On the 
read side both ISACTIVERx and ICACTIVERx behave the same, so they should be 
handled the same.
And returning 0 is probably the best approximation we can do at the moment. The 
other solution is to add GICv3 support to the new VGIC ;-), as this at least 
solves the case when we deliberately inject an active IRQ. We could extend this 
logic to find out which IRQs in this block *could* possibly be active, then 
bring those VCPUs back to Xen.

Cheers,
Andre.

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