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

[Andre Przywara <andre.przywara@linaro.org>]

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

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.

Cheers,
Andre.

>> I changed it now to output:
>> %pv: vGICD: clearing active state not supported (IRQ%u, value: 0x%08lx)
>>
>>> I don't much care about the spam, see why above.
>>
>> Having them on the console between Dom0 messages is really scary, but
>> not helpful if it's *more* than one. Since it's a known limitation of
>> the VGIC emulation, not a real "error" in that sense.
> 
> It is the same as having any Xen messages interleaved with Dom0
> messages. If the user is not happy with that, then it can divert Dom0
> console to another UART.
> 
> Cheers,
> 

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