[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.