Re: [Xen-devel] [PATCH 39/57] ARM: new VGIC: Add ACTIVE registers handlers

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 13/03/18 17:02, Andre Przywara wrote:
> On 08/03/18 15:39, Julien Grall wrote:
> > On 05/03/18 16:03, Andre Przywara wrote:
> > > +/*
> > > + * We don't actually support clearing the active state of an IRQ (yet).
> > > + * However there is a chance that most guests use this for
> > > initialization.
> > > + * We check whether this MMIO access would actually affect any active
> > > IRQ,
> > > + * and only print our warning in this case. So clearing already
> > > non-active
> > > + * IRQs would not be moaned about in the logs.
> > > + */
> > > +void vgic_mmio_write_cactive(struct vcpu *vcpu,
> > > +                             paddr_t addr, unsigned int len,
> > > +                             unsigned long val)
> > > +{
> > > +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> > > +    unsigned int i;
> > > +    bool bail_out = false;
> > > +
> > > +    for_each_set_bit( i, &val, len * 8 )
> > > +    {
> > > +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid
> > > + i);
> > > +
> > > +        /*
> > > +         * If we know that the IRQ is active or we can't be sure about
> > > +         * it (because it is currently in a CPU), log the not properly
> > > +         * emulated MMIO access.
> > > +         */
> > > +        if ( irq->active || irq->vcpu )
> > > +        {
> > > +            gdprintk(XENLOG_ERR,
> > > +                     "%pv: vGICD: IRQ%d: clearing active state not
> > > supported\n",
> > s/%d/%u/
> >
> > > +                     vcpu, irq->intid);
> > gdprintk will always print the vCPU. Thought it is the current which
> > might be different from vcpu (mostly in the re-dist case).
> Ah, thanks. I always get confused about what which version of *printk does.
>
> > So I would use dprintk(XENLOG_G_ERR, "%pv: ..."). I would even be tempt
> > to use printk(....) so we can spot potential issue on non-debug build.
> Well, in the true spirit of Xen paranoia ;-) I wanted to avoid a guest
> spamming the console.
The guests messages are rate limited.

> And in the end there is nothing a administrator
> could really do about it. In my experience those messages tend to really
> scare users ("I could boot Dom0 but I see those error messages ....").
Xen message are not only here for the administrator, they are also here 
to help for the developer to get log to dissect.
I think that particular message should be printed in non-debug build 
because if the interrupt was active and can't clear it. Then something
will go wrong later on.

> > > +            bail_out = true;
> > I admit the bailout is a bit weird here. You would only print the
> > warning for the first activated IRQ and give the impression it is fine
> > for the rest. So maybe you want to drop IRQ%d?
> For the above reasons I wanted to keep them concise, so that we see that
> the issue has happened, but avoid getting tons of error messages about
> the same problem (as this may affect up to 32 IRQs).
> But for debugging it might be good to know which IRQ was affected. I see
> two use cases for a guest:
> - (De-)activating a single IRQ: we get one message and know which IRQ it
> was, so an admin can chase this down to a certain device (driver).
> - (De-)activating *every* IRQ in this range (~0): we still get one
> message per 32 IRQs, but can see whether it covers SPIs only (IRQ>=32)
> and which ones.
>
> So what about a compromise: I use dprintk(XENLOG_G_ERR, "%pv ...), print
> the (first) IRQ and the *value* to be written. So a knowledgeable admin
> can tell whether it's a single IRQ or a "clear/set-all" case. That
> should also give enough info for debugging, but keeps it short.
I can't see how a knowledgeable admin will be able to know that IRQ 2 is 
active with just the register value.
> Does that sound OK?
I would still prefer the one per IRQ and using printk(XENLOG_G_*). I 
don't much care about the spam, see why above.
Cheers,

--
Julien Grall

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