| Message ID | 20180305160415.16760-52-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
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 <andre.przywara@linaro.org> > --- > 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(&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 > Cheers,
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 <andre.przywara@linaro.org> >> --- >> 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(&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 >> > > Cheers, >
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,
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(&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
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> --- Changelog RFC ... v1: - split off from former patch, otherwise unchanged xen/arch/arm/vgic/vgic.c | 8 ++++++++ 1 file changed, 8 insertions(+)