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

[Andre Przywara <andre.przywara@linaro.org>]

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.

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

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel