Re: [Xen-devel] [PATCH 51/57] ARM: new VGIC: Add preliminary stub implementation

2018-03-14 Thread Julien Grall 

Hi,

On 03/13/2018 03:55 PM, Andre Przywara wrote:

Hi,

On 09/03/18 18:18, Julien Grall wrote:

Hi Andre,

On 05/03/18 16:04, 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.


The implementation of that function is fundamentally wrong on the
current vGIC 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.

So I would kill that function. Any opinions?


So I guess given that the patch is pretty small, we are good with
keeping that for now, and solve this together with the old VGIC later.


I am ok with that.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 51/57] ARM: new VGIC: Add preliminary stub implementation

2018-03-13 Thread Andre Przywara 
Hi,

On 09/03/18 18:18, Julien Grall wrote:
> Hi Andre,
> 
> On 05/03/18 16:04, 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.
> 
> The implementation of that function is fundamentally wrong on the
> current vGIC 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.
> 
> So I would kill that function. Any opinions?

So I guess given that the patch is pretty small, we are good with
keeping that for now, and solve this together with the old VGIC later.

Cheers,
Andre.

>>
>> Signed-off-by: Andre Przywara 
>> ---
>> Changelog RFC ... v1:
>> - split off from former patch, otherwise unchanged
>>
>>   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 5e767927c0..5d84a4d81a 100644
>> --- a/xen/arch/arm/vgic/vgic.c
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -790,6 +790,14 @@ void gic_dump_vgic_info(struct vcpu *v)
>>   spin_unlock_irqrestore(>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
>>
> 
> Cheers,
> 

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

Re: [Xen-devel] [PATCH 51/57] ARM: new VGIC: Add preliminary stub implementation

2018-03-09 Thread Julien Grall 

Hi Andre,

On 05/03/18 16:04, 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.


The implementation of that function is fundamentally wrong on the 
current vGIC 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.


So I would kill that function. Any opinions?



Signed-off-by: Andre Przywara 
---
Changelog RFC ... v1:
- split off from former patch, otherwise unchanged

  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 5e767927c0..5d84a4d81a 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -790,6 +790,14 @@ void gic_dump_vgic_info(struct vcpu *v)
  spin_unlock_irqrestore(>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



Cheers,

--
Julien Grall

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