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

Re: [Xen-devel] [RFC PATCH 30/49] ARM: new VGIC: Add ENABLE registers handlers





On 27/02/18 13:54, Andre Przywara wrote:
Hi,

On 19/02/18 14:13, Julien Grall wrote:


On 19/02/18 12:41, Andre Przywara wrote:
Hi,

Hi,

On 16/02/18 16:57, Julien Grall wrote:
On 09/02/18 14:39, Andre Przywara wrote:
+    spin_lock_irqsave(&desc->lock, flags);
+    if ( enable )
+    {
+        gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ?
+                 IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING);

Indentation and I would prefer a helper to convert between the vgic
value and the IRQ_TYPE. This would make the code easier to read.

Also, this code does not replicate correctly the current vGIC.
gic_set_irq_type is only allowed to be used when
irq_set_type_by_domain(d) returns true. If you consider this change
valid, then I would like to know why.

So what is/was the rationale for not allowing IRQ type changes for
non-privileged guests? If you allow to pass through an hardware IRQ to a
guest (which is the case this function handles), then I don't see why a
guest would not be allowed to change the configuration? It seems rather
odd, I guess it's up to the guest to know which type of IRQ this is?

If you can answer the question on top of irq_type_set_by_domain (i.e
"See whether it is possible to let any domain configure the type) then
we can remove it. We decided to only allow for the hardware domain
because we trust it.

But why would you mistrust a DomU in this respect?
The only point I see is that a guest has *some* influence on a hardware
access, but I fail to see how a single MMIO read-modify-write sequence
would actually impact the host. Especially since we do it only on
enabling an IRQ.
Looking more closely at the existing VGIC code we might want to check if
the hardware IRQ was already enabled before entering the
"if ( p->desc != NULL )" branch, btw.

That's not an issue here. You can only enter in vgic_enable_irqs if the virtual interrupt was previously disabled. Because the physical interrupt is routed to the guest, it will also be disabled at that time.


So is this the concern? Commit b0003bdd690 wasn't really enlightening in
this respect.

It was not really clear if it would be an issue when I wrote the patch. We trust the hardware domain so it is fine to let him configure the interrupt. For the guests, this will be taken from the DT (see gic_route_irq_to_guest). So this is likely to get configured correctly for the guest.

What I was worry about is whether we need to sanitize the ICFGR when the interrupt is routed to another domain. But if you can clear that, then I guess it should be ok.

However, I would prefer to do this in a separate patch and keep irq_type_set_by_domain around. This is to match the current vGIC and not changing too much Xen behavior.

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®.