diff mbox series

[Xen-devel,v3,33/39] ARM: new VGIC: Add preliminary stub implementation

Message ID 20180321163235.12529-34-andre.przywara@linaro.org
State New
Headers show
Series New VGIC(-v2) implementation | expand

Commit Message

Andre Przywara March 21, 2018, 4:32 p.m. UTC 
The ARM arch code requires an interrupt controller emulation to implement
vgic_clear_pending_irqs(), although it is suspected that it is actually
not necessary. Go with a stub for now to make the linker happy.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/vgic/vgic.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Stefano Stabellini March 27, 2018, 10:48 p.m. UTC | #1 
On Wed, 21 Mar 2018, Andre Przywara wrote:
> The ARM arch code requires an interrupt controller emulation to implement
> vgic_clear_pending_irqs(), although it is suspected that it is actually
> not necessary. Go with a stub for now to make the linker happy.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/vgic/vgic.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index 23b8abfc5e..b70fdaaecb 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -791,6 +791,14 @@ void gic_dump_vgic_info(struct vcpu *v)
>      spin_unlock_irqrestore(&v->arch.vgic.ap_list_lock, flags);
>  }
>  
> +void vgic_clear_pending_irqs(struct vcpu *v)
> +{
> +    /*
> +     * TODO: It is unclear whether we really need this, so we might instead
> +     * remove it on the caller site.
> +     */
> +}

This is OK for now.

However, thinking about this issue, is it possible for a vcpu to send an
interrupt to an offline vcpu, maybe an SGI? What would happen in that
case? It looks like that vgic_mmio_write_sgir would allow it. Otherwise,
a vcpu could cause the generation of a physical interrupt, an SPI,
targeting an offline vcpu.

Maybe we should WARN in case ap_list is not empty?


>  /**
>   * arch_move_irqs() - migrate the physical affinity of hardware mapped vIRQs
>   * @v:  the vCPU, already assigned to the new pCPU
> -- 
> 2.14.1
>
Julien Grall April 3, 2018, 1:22 p.m. UTC | #2 
Hi Stefano,

On 27/03/18 23:48, Stefano Stabellini wrote:
> On Wed, 21 Mar 2018, Andre Przywara wrote:
>> The ARM arch code requires an interrupt controller emulation to implement
>> vgic_clear_pending_irqs(), although it is suspected that it is actually
>> not necessary. Go with a stub for now to make the linker happy.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/vgic/vgic.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>> index 23b8abfc5e..b70fdaaecb 100644
>> --- a/xen/arch/arm/vgic/vgic.c
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -791,6 +791,14 @@ void gic_dump_vgic_info(struct vcpu *v)
>>       spin_unlock_irqrestore(&v->arch.vgic.ap_list_lock, flags);
>>   }
>>   
>> +void vgic_clear_pending_irqs(struct vcpu *v)
>> +{
>> +    /*
>> +     * TODO: It is unclear whether we really need this, so we might instead
>> +     * remove it on the caller site.
>> +     */
>> +}
> 
> This is OK for now.
> 
> However, thinking about this issue, is it possible for a vcpu to send an
> interrupt to an offline vcpu, maybe an SGI? What would happen in that
> case? It looks like that vgic_mmio_write_sgir would allow it. Otherwise,
> a vcpu could cause the generation of a physical interrupt, an SPI,
> targeting an offline vcpu.
> 
> Maybe we should WARN in case ap_list is not empty?

The interrupts would be delivered when the vCPU is going online. It does 
not seem against the specification. Actually from the spec, it is valid 
to route an interrupt any vCPU (even non-existent one).

However, as I said on a previous version of this patch, the 
implementation on the current vGIC is just plain wrong for a few reasons:
         - lr_mask is reset but the LRs are not. This means when we 
context switch back, the LR might still be written and injecting 
unexpected interrupt (whoops).
         - both lists (inflight and pending) are cleared which means 
that a physical interrupt pending on that vCPU is lost forever (stay 
active in the physical so never going to fire again).

Furthermore, I don't think that Xen business to reset the GIC on cpu_on. 
If anything should be done, then is it on CPU_off to migrate the current 
interrupts to another vCPU. But IIRC the OS is responsible for that.

One the same note, similar problem would happen because of 
vgic_inject_irq would bail out on offline vCPU. This means that 
interrupt may be lost forever.

We are quite lucky that we don't use PSCI on/off very often in Xen.

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 23b8abfc5e..b70fdaaecb 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -791,6 +791,14 @@  void gic_dump_vgic_info(struct vcpu *v)
     spin_unlock_irqrestore(&v->arch.vgic.ap_list_lock, flags);
 }
 
+void vgic_clear_pending_irqs(struct vcpu *v)
+{
+    /*
+     * TODO: It is unclear whether we really need this, so we might instead
+     * remove it on the caller site.
+     */
+}
+
 /**
  * arch_move_irqs() - migrate the physical affinity of hardware mapped vIRQs
  * @v:  the vCPU, already assigned to the new pCPU