[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 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

 


Rackspace

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