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

[Julien Grall <julien.grall@arm.com>]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel