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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 03/14/2018 02:30 PM, Andre Przywara wrote:
> Hi,
>
> On 13/03/18 17:42, Julien Grall wrote:
> > Hi,
> >
> > On 13/03/18 17:34, Andre Przywara wrote:
> > > On 13/03/18 17:14, Julien Grall wrote:
> > > > 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:
> > > > I can't see how a knowledgeable admin will be able to know that IRQ 2 is
> > > > active with just the register value.
> > > Well, I was assuming that a really knowledgeable admin would somehow
> > > forward the error message either to the ML or at least to $search_engine.
> > > ...
> > Surely, but it does not mean the message should be clueless for the
> > developer. I would rather no spent 10 min to try to find out what's
> > going on where reading logs...
> >
> > > > > Does that sound OK?
> > > > I would still prefer the one per IRQ and using printk(XENLOG_G_*).
> > > I really don't think one per IRQ is too useful. A developer however can
> > > easily deal with "IRQ45, value: 0x00802000" from a log. And can deduce
> > > from there that it's about IRQ45 and IRQ55. Following the example above
> > > you would either see one "IRQ32, value: 0xffffffff" or "IRQ 45, value:
> > > 0x00002000".
> > I still can't see how the developer would know the IRQ55 is active or
> > not. That's the whole purpose of the per IRQ.
> >
> > > That looks like a good compromise between readability (having the IRQ
> > > number for admins) and brevity.
> > You may save 10 characters on the logs, you likely going to waste 10 min
> > of the developer to understand what that messages really mean.
> It's not about 10 characters, it's about 31 *lines*.
> At the moment a write to I[CS]ACTIVER triggers *one* line in the log:
> %pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d
>
> Now a write to this register would potentially trigger *32* lines:
> %pv: vGICD: IRQ%u: setting active state not supported
Very likely there will 0 lines printed because clearing an active 
interrupt should never happen in Xen guest today. So if there is 32 
lines printed, then having 32 lines in the log is your last of your concern.
> By dumping the line as I proposed, I basically mimic the current line,
> plus give some information about one IRQ affected:
> %pv: vGICD: setting active state not supported (IRQ%u, value: 0x%08lx)
>
> So this is not a regression, but an improvement.
I never said it was a regression. I said your new message and bail out 
is counter-intuitive because the developer can't guess if there are 
other IRQs active with just "value".
If you want to make an improvement, do it properly. In that case, I am 
only asking to drop the counter-intuitive bail_out.
Cheers.

--
Julien Grall

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