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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 19 Nov 2019, Julien Grall wrote:
> Hi Andre,
> 
> On 12/11/2019 12:46, Andre Przywara wrote:
> > On Mon, 11 Nov 2019 11:01:07 -0800 (PST)
> > Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> > > 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?\
> It happens I am sitting right next to Marc now, so I had a chat with him about
> this :). Let me try to summarize the discussion here.
> 
> __synchronize_hardirq() is used to ensure that all active interrupts have been
> handled before continuing. When sync_chip == false, we only ensure that all in
> progress interrupts (from Linux PoV) are handled before continue.
> 
> sync_chip == true will additionally ensure that any interrupt that were
> acknowledge but not yet marked as in progress by the kernel are also handled.
> The assumption is this is called after the interrupt has been masked/disabled.
> 
> The call to __synchronize_hardirq() in free_irq() (as reported by Peng stack
> trace) was introduced recently (see [1]). It is not entirely clear whether
> this would affect anyone using Linux 5.4 or just a limited subset of users.
> 
> Anyhow, this is a genuine bug and I think returning 0 is only papering over
> the bug with no long-term resolution. As Marc pointed out, Even the old vGIC
> in KVM was not spec compliant and thankfully this was fixed in the new vGIC.
> 
> As I said in a different sub-thread, I would reluctanly be ok to see returning
> 0 as long as we have add a warning for *every* access. Long-term, the current
> vGIC should really get killed.

I appreciate your intention of raising awareness and getting help in
fixing things in the community, which is the right thing to do. However,
I am doubtful of the usefulness of a warning to achieve the goal. Maybe
it would be best to curate an "open issues" section somewhere, even just
as an email after every release or as part of the release notes, or as a
jira ticket, or a wikipage, or a document under docs/.

Actually, an "open issues" document under docs/ could be a good idea for
this and other similar items.

A warning is a blunt instrument that lacks the subtleties necessary to
raise the attention in the right way and achieve the goal of getting
help and contributions. Especially it has the risk of causing problems
and confusion with less knowledgeable users.  Maybe a dprintk warning
message (only DEBUG builds) could avoid some of the issues, while still
gaining attention of savvy developers who could understand what it means.
But I think that the "open issues" document would be more effective.

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