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

[Julien Grall <julien.grall@xxxxxxx>]

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