Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-04 Thread Julien Grall
Hi Andrii, On 12/3/18 2:36 PM, Andrii Anisov wrote: On 03.12.18 15:53, Juergen Gross wrote: On 03/12/2018 14:46, Andre Przywara wrote: I think PERFCOUNTER is your friend. CONFIG_LOCK_PROFILE and xen-lockprof on tools side? Not sure it is still working, though. Its about 9 years since I

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-03 Thread Andrii Anisov
Hello Andre, On 03.12.18 15:46, Andre Przywara wrote: Well, you should be scared of the old VGIC locking scheme instead ;-) Old VGIC locking is more mazy, indeed. Apart from the vgic_queue_irq_unlock() function, the rest of the new locking scheme is much clearer. I agree, Yes, but

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-03 Thread Andrii Anisov
Hello Juergen, On 03.12.18 15:53, Juergen Gross wrote: On 03/12/2018 14:46, Andre Przywara wrote: I think PERFCOUNTER is your friend. CONFIG_LOCK_PROFILE and xen-lockprof on tools side? Not sure it is still working, though. Its about 9 years since I wrote and used it. It does work. I've

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-03 Thread Juergen Gross
On 03/12/2018 14:46, Andre Przywara wrote: > On 30/11/2018 19:52, Andrii Anisov wrote: >> Hello Andre, >> >> Please see my comments below: >> >> On 23.11.18 14:18, Andre Przywara wrote: >>> Fundamentally there is a semantic difference between edge and level >>> triggered IRQs: When the guest has

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-03 Thread Andre Przywara
On 30/11/2018 19:52, Andrii Anisov wrote: > Hello Andre, > > Please see my comments below: > > On 23.11.18 14:18, Andre Przywara wrote: >> Fundamentally there is a semantic difference between edge and level >> triggered IRQs: When the guest has handled an *edge* IRQ (EOIed so >> the LR's state

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-03 Thread Julien Grall
Hi Andrii, On 03/12/2018 12:58, Andrii Anisov wrote: On 03.12.18 14:17, Julien Grall wrote: No. I meant that I would be happy with that and I think should also suit you. Great! There are no isb() in do_trap_irq(). So did you mean gic_interrupt()? Right you are. But then, I am not sure

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-03 Thread Andrii Anisov
On 03.12.18 14:17, Julien Grall wrote: No. I meant that I would be happy with that and I think should also suit you. Great! There are no isb() in do_trap_irq(). So did you mean gic_interrupt()? Right you are. But then, I am not sure why you want to avoid the isb() in the guest path. Well,

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-03 Thread Julien Grall
Hi Andrii, On 03/12/2018 12:08, Andrii Anisov wrote: On 27.11.18 17:13, Julien Grall wrote: The steps for Xen IRQ is roughly:  -> read_irq  -> local_irq_enable  -> do_IRQ     -> local_irq_enable (via spin_unlock_irq)     -> call handlers     -> local_irq_disable (via

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-12-03 Thread Andrii Anisov
Hello Julien, On 27.11.18 17:13, Julien Grall wrote: Oversubscribing is usually a pretty bad idea if you want to have good latency. There are no promise when the vCPU will get scheduled. Yes, I know and clearly understand that. But we still have requirements. So can you describe how

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-30 Thread Andrii Anisov
Hello Andre, Please see my comments below: On 23.11.18 14:18, Andre Przywara wrote: Fundamentally there is a semantic difference between edge and level triggered IRQs: When the guest has handled an *edge* IRQ (EOIed so the LR's state goes to 0), this is done and dusted, and Xen doesn't need

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-27 Thread Stefano Stabellini
On Thu, 22 Nov 2018, Julien Grall wrote: > > > If you are worried about performance, then I would recommend to try the > > > new vGIC and see whether it improves. > > You know, we are based on XEN 4.10. Initially, when a customer said about > > their dissatisfaction about performance drop in

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-27 Thread Julien Grall
On 11/27/18 1:30 PM, Andrii Anisov wrote: Hello Julien, Hi Andrii, On 23.11.18 15:27, Julien Grall wrote: But we can't use it, because our system is overcommitted. Can you describe how overcommitted your system is? I did mean that we have a requirement to not limit VCPU number with

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-27 Thread Andrii Anisov
Hello Julien, On 23.11.18 15:27, Julien Grall wrote: But we can't use it, because our system is overcommitted. Can you describe how overcommitted your system is? I did mean that we have a requirement to not limit VCPU number with PCPU number. Also we should not use cpu pinning. I guess I

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-23 Thread Julien Grall
On 23/11/2018 12:58, Andrii Anisov wrote: Hello Andre, On 22.11.18 20:04, Andre Przywara wrote: Is that benchmark chosen to put some interrupt load on the system? Or is that what the customer actually uses and she is really suffering from Xen's interrupt handling and emulation? That it the

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-23 Thread Andrii Anisov
On 23.11.18 14:18, Andre Przywara wrote: Fundamentally there is a semantic difference between edge and level triggered IRQs: When the guest has handled an *edge* IRQ (EOIed so the LR's state goes to 0), this is done and dusted, and Xen doesn't need to care about this anymore until the next IRQ

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-23 Thread Andrii Anisov
Hello Andre, On 22.11.18 20:04, Andre Przywara wrote: Is that benchmark chosen to put some interrupt load on the system? Or is that what the customer actually uses and she is really suffering from Xen's interrupt handling and emulation? That it the 3D benchmark used by customer to compare

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-23 Thread Andre Przywara
On Fri, 23 Nov 2018 12:09:41 +0200 Andrii Anisov wrote: Hi, > On 22.11.18 19:22, Julien Grall wrote: > > My biggest worry is you are doing optimization on a vGIC that is > > not fully compliant with how a GIC should behave (e.g edge vs > > level) and with very fragile locking. > Yep, old VGIC

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-23 Thread Andrii Anisov
Hello Julien, On 22.11.18 19:22, Julien Grall wrote: My biggest worry is you are doing optimization on a vGIC that is not fully compliant with how a GIC should behave (e.g edge vs level) and with very fragile locking. Yep, old VGIC locking looks pretty terrible. If you are interested, Andre

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-22 Thread Andre Przywara
On Thu, 22 Nov 2018 18:51:13 +0200 Andrii Anisov wrote: Hi, > On 20.11.18 20:47, Julien Grall wrote: > > > > > > On 20/11/2018 18:10, Andrii Anisov wrote: > >> Hello Julien, > >> > >> > >> On 19.11.18 18:42, Julien Grall wrote: > >>> There are no issue about processing IRQs before the

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-22 Thread Julien Grall
On 11/22/18 4:51 PM, Andrii Anisov wrote: Hello Julien, Hi Andrii, On 20.11.18 20:47, Julien Grall wrote: On 20/11/2018 18:10, Andrii Anisov wrote: Hello Julien, On 19.11.18 18:42, Julien Grall wrote: There are no issue about processing IRQs before the syncs. It is the same as if

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-22 Thread Andrii Anisov
Hello Julien, On 20.11.18 20:47, Julien Grall wrote: On 20/11/2018 18:10, Andrii Anisov wrote: Hello Julien, On 19.11.18 18:42, Julien Grall wrote: There are no issue about processing IRQs before the syncs. It is the same as if an IRQ was raised from ila different pCPUs. So why do you

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-20 Thread Julien Grall
On 20/11/2018 18:10, Andrii Anisov wrote: Hello Julien, On 19.11.18 18:42, Julien Grall wrote: There are no issue about processing IRQs before the syncs. It is the same as if an IRQ was raised from ila different pCPUs. So why do you need that? From my understanding of gic-vgic code (old

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-20 Thread Andrii Anisov
Hello Julien, On 19.11.18 18:42, Julien Grall wrote: There are no issue about processing IRQs before the syncs. It is the same as if an IRQ was raised from ila different pCPUs. So why do you need that? From my understanding of gic-vgic code (old vgic), for the IRQs targeting the `current`

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-19 Thread Julien Grall
On Mon, 19 Nov 2018, 15:57 Andrii Anisov, wrote: > Julien, > > > It's me again about your patch:) > > I've found this patch useful and even can give a motivation to have it > in the mainline. The patch ensures that vgic_sync_from_lrs is performed > on guest to hyp switch prior to any IRQ

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-19 Thread Andrii Anisov
Julien, It's me again about your patch:) I've found this patch useful and even can give a motivation to have it in the mainline. The patch ensures that vgic_sync_from_lrs is performed on guest to hyp switch prior to any IRQ processing. So, do you plan to push it for review? Could I do that

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-09 Thread Stefano Stabellini
On Tue, 23 Oct 2018, Julien Grall wrote: > Devices that expose their interrupt status registers via system > registers (e.g. Statistical profiling, CPU PMU, DynamIQ PMU, arch timer, > vgic (although unused by Linux), ...) rely on a context synchronising > operation on the CPU to ensure that the

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-09 Thread Julien Grall
On 09/11/2018 14:42, Andrii Anisov wrote: Hello Julien, Hi Andrii, I just wonder, do you plan to upstream the patch below? I don't plan to upstream the patch below. Andre and I discussed about it extensively and haven't found potential issue with the 2 vGIC implementation we have in

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-11-09 Thread Andrii Anisov
Hello Julien, I just wonder, do you plan to upstream the patch below? Andrii Anisov On 29/10/2018 10:06, Andrii Anisov wrote: > Hello Julien, Hi, > > Sorry for the previous email sent as html. Don't worry. I didn't notice it :). > > > On 27.10.18 15:14, Andrii Anisov wrote: diff

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-30 Thread Andrii Anisov
Hi, On 29.10.18 18:22, Julien Grall wrote: > What is the issue? Is it just your print or there a latent bug in > current vGIC? Ah, OK, that was the issue for me. > I actually prefer if we re-enable interrupts after > entry_hypervisor_head(). This makes the code working the same way >

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-29 Thread Andrii Anisov
Hello Julien On 29.10.18 15:36, Julien Grall wrote:0 I wrote down an answer yesterday (sent it today) to your previous answer. You may use the LRs information from the previous guest trap as interrupts are re-enabled before storing the LRs. Yes, it is the case. I've overlooked that for some

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-29 Thread Julien Grall
On 29/10/2018 16:16, Andrii Anisov wrote: Hello Julien Hi, On 29.10.18 15:36, Julien Grall wrote:0 I wrote down an answer yesterday (sent it today) to your previous answer. You may use the LRs information from the previous guest trap as interrupts are re-enabled before storing the LRs.

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-29 Thread Andrii Anisov
Hello Julien On 29.10.18 15:36, Julien Grall wrote:0 > I wrote down an answer yesterday (sent it today) to your previous > answer. You may use the LRs information from the previous guest trap as > interrupts are re-enabled before storing the LRs. Yes, it is the case. I've overlooked that for

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-29 Thread Julien Grall
On 29/10/2018 10:06, Andrii Anisov wrote: Hello Julien, Hi, Sorry for the previous email sent as html. Don't worry. I didn't notice it :). On 27.10.18 15:14, Andrii Anisov wrote: diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index f6f6de3..985192b 100644 ---

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-29 Thread Julien Grall
On 10/27/18 1:14 PM, Andrii Anisov wrote: Hello Julien, On 10/27/2018 12:55 AM, Julien Grall wrote: The functions below does not exist in latest Xen. So are you sure this based on mainline? Yep, I've put a wrong diff into the email, it is for XEN 4.10. Please find the patch for XEN

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-29 Thread Andrii Anisov
Hello Julien, Sorry for the previous email sent as html. On 27.10.18 15:14, Andrii Anisov wrote: >>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>> index f6f6de3..985192b 100644 >>> --- a/xen/arch/arm/traps.c >>> +++ b/xen/arch/arm/traps.c >>> @@ -2095,6 +2095,7 @@ static void

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-26 Thread Julien Grall
On 10/26/18 1:49 PM, Andrii Anisov wrote: Hello Julien Hi, On 25.10.18 17:11, Andrii Anisov wrote: I guess I should make a dedicated patch applicable to mainline to reveal the issue. I hope I'll do this nearest days. Please find below the diff applicable to the current xenbits/smoke

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-26 Thread Andrii Anisov
Hello Julien On 25.10.18 17:11, Andrii Anisov wrote: > I guess I should make a dedicated patch applicable to mainline to reveal > the issue. I hope I'll do this nearest days. Please find below the diff applicable to the current xenbits/smoke which exposes the issue. With that diff I see (on

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-25 Thread Andrii Anisov
On 23.10.18 21:17, Julien Grall wrote: > Devices that expose their interrupt status registers via system > registers (e.g. Statistical profiling, CPU PMU, DynamIQ PMU, arch timer, > vgic (although unused by Linux), ...) rely on a context synchronising > operation on the CPU to ensure that the

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-25 Thread Andrii Anisov
On 25.10.18 17:22, Julien Grall wrote: > You misread what I wrote. They are not needed to cover the vGIC for > GICv2 platform. However they are still necessary, for instance, for > the timer as it is accessible via system registers. Ah, OK. > Here the isb() sits between ack() and do_IRQ(). >

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-25 Thread Julien Grall
On 10/25/18 3:11 PM, Andrii Anisov wrote: Hello  Julien, Hi, On 24.10.18 17:41, Julien Grall wrote: vGIC is not only about GICv2. It also covers GICv3 that is accessible via system registers. Yes, I know. But, as you state below, for GICv2 based systems those barriers are not needed.

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-25 Thread Andrii Anisov
Hello Julien, On 24.10.18 17:41, Julien Grall wrote: > vGIC is not only about GICv2. It also covers GICv3 that is accessible > via system registers. Yes, I know. But, as you state below, for GICv2 based systems those barriers are not needed. >>>rely on a context synchronising >>> operation

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-24 Thread Julien Grall
On 10/24/18 10:38 AM, Andrii Anisov wrote: Hello Julien, Hi Andrii, Thank you for the review. On 23.10.18 21:17, Julien Grall wrote: Devices that expose their interrupt status registers via system registers (e.g. Statistical profiling, CPU PMU, DynamIQ PMU, arch timer, vgic (although

Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

2018-10-24 Thread Andrii Anisov
Hello Julien, This patch is very interesting to me. On 23.10.18 21:17, Julien Grall wrote: > Devices that expose their interrupt status registers via system > registers (e.g. Statistical profiling, CPU PMU, DynamIQ PMU, arch timer, > vgic (although unused by Linux), ...) I guess under vgic you