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

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

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