[Xen-devel,RFC,43/49] ARM: new VGIC: Add preliminary stub implementations

Message ID 20180209143937.28866-44-andre.przywara@linaro.org
State New
Headers show
Series
  • New VGIC(-v2) implementation
Related show

Commit Message

Andre Przywara Feb. 9, 2018, 2:39 p.m.
The Xen core code requires an interrupt controller emulation to implement
arch_move_irqs(), to move the affinity of an hardware mapped virtual IRQ
to another core. In the moment we don't implement this
physical-follow-virtual regime in our new VGIC, so just provide an empty
stub implementation to make the linker happy.
Similarily vgic_clear_pending_irqs() is required by the ARM code,
although it is suspected that it is actually not necessary. Go with a
stub for now.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/vgic/vgic.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Julien Grall Feb. 19, 2018, 12:34 p.m. | #1
Hi,

On 09/02/18 14:39, Andre Przywara wrote:
> The Xen core code requires an interrupt controller emulation to implement
> arch_move_irqs(), to move the affinity of an hardware mapped virtual IRQ
> to another core. In the moment we don't implement this
> physical-follow-virtual regime in our new VGIC, so just provide an empty
> stub implementation to make the linker happy.

physical-follow-virtual is a must feature for the new vGIC. This has 
shown better interrupt latency.

> Similarily vgic_clear_pending_irqs() is required by the ARM code,
> although it is suspected that it is actually not necessary. Go with a
> stub for now.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/vgic/vgic.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index d91028bd43..77fa756329 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -770,6 +770,19 @@ void gic_dump_vgic_info(struct vcpu *v)
>                  irq->active ? "" : "not ", irq->enabled ? "" : "not ");
>   }
>   
> +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.
> +     */

I remember some issue with the current vGIC when not removing pending 
interrupts on PSCI CPU ON. But that was a while ago. I will have another 
try and see if we can drop it.

> +}
> +
> +void arch_move_irqs(struct vcpu *v)
> +{
> +    /* TODO: implement this (?) */

Here you would need to go through the interrupt and modify the affinity 
of the physical IRQs routed to that vCPU.

> +}
> +
>   struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
>                                         unsigned int virq)
>   {
> 

Cheers,
Andre Przywara Feb. 27, 2018, 5:05 p.m. | #2
Hi,

On 19/02/18 12:34, Julien Grall wrote:
> Hi,
> 
> On 09/02/18 14:39, Andre Przywara wrote:
>> The Xen core code requires an interrupt controller emulation to implement
>> arch_move_irqs(), to move the affinity of an hardware mapped virtual IRQ
>> to another core. In the moment we don't implement this
>> physical-follow-virtual regime in our new VGIC, so just provide an empty
>> stub implementation to make the linker happy.
> 
> physical-follow-virtual is a must feature for the new vGIC. This has
> shown better interrupt latency.
> 
>> Similarily vgic_clear_pending_irqs() is required by the ARM code,
>> although it is suspected that it is actually not necessary. Go with a
>> stub for now.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   xen/arch/arm/vgic/vgic.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>> index d91028bd43..77fa756329 100644
>> --- a/xen/arch/arm/vgic/vgic.c
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -770,6 +770,19 @@ void gic_dump_vgic_info(struct vcpu *v)
>>                  irq->active ? "" : "not ", irq->enabled ? "" : "not ");
>>   }
>>   +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.
>> +     */
> 
> I remember some issue with the current vGIC when not removing pending
> interrupts on PSCI CPU ON. But that was a while ago. I will have another
> try and see if we can drop it.

So that's what I came up with:
CPU_ON can be called from two states:
- The initial state of a core before it's being brought up for the first
time.
- After the OS has called CPU_OFF.
 The PSCI spec says that before calling CPU_OFF an OS all IRQs must have
been migrated away from that core. I take this as no IRQs are allowed,
hence we don't have to clear anything on CPU_ON.

In both cases the CPU is expected to enter in a defined state, which
includes all interrupts masked on the CPU level (SPSR.ELx.[DAIF] = 1).
The GIC defaults to ISPENDR being 0.

So I take we should not be held responsible for clearing the pending
state upon CPU_ON.

What is your opinion?

>> +}
>> +
>> +void arch_move_irqs(struct vcpu *v)
>> +{
>> +    /* TODO: implement this (?) */
> 
> Here you would need to go through the interrupt and modify the affinity
> of the physical IRQs routed to that vCPU.

Since this is apparently the next best thing after sliced bread, I
implemented this now. Coming to a theatre near you any time soon.

> 
>> +}
>> +
>>   struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
>>                                         unsigned int virq)
>>   {
>>
> 
> Cheers,
>

Patch

diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index d91028bd43..77fa756329 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -770,6 +770,19 @@  void gic_dump_vgic_info(struct vcpu *v)
                irq->active ? "" : "not ", irq->enabled ? "" : "not ");
 }
 
+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.
+     */
+}
+
+void arch_move_irqs(struct vcpu *v)
+{
+    /* TODO: implement this (?) */
+}
+
 struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
                                       unsigned int virq)
 {