From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v5 15/22] xen/arm: ITS: implement hw_irq_controller for LPIs Date: Thu, 6 Aug 2015 11:05:02 +0100 Message-ID: <55C3314E.3040304@citrix.com> References: <1437995524-19772-1-git-send-email-vijay.kilari@gmail.com> <1437995524-19772-16-git-send-email-vijay.kilari@gmail.com> <55C0C1E8.10407@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Vijay Kilari Cc: Ian Campbell , Stefano Stabellini , Prasun Kapoor , manish.jaggi@caviumnetworks.com, Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , Vijaya Kumar K List-Id: xen-devel@lists.xenproject.org On 06/08/15 09:15, Vijay Kilari wrote: > On Tue, Aug 4, 2015 at 7:15 PM, Julien Grall wrote: >> Hi Vijay, >> >> On 27/07/15 12:11, vijay.kilari@gmail.com wrote: >>> + >>> +static const hw_irq_controller its_host_lpi_type = { >>> + .typename = "gic-its", >>> + .startup = its_irq_startup, >>> + .shutdown = its_irq_shutdown, >>> + .enable = its_irq_enable, >>> + .disable = its_irq_disable, >>> + .ack = its_irq_ack, >>> + .end = gicv3_host_irq_end, >>> + .set_affinity = its_irq_set_affinity, >>> +}; >>> + >>> +static const hw_irq_controller its_guest_lpi_type = { >>> + .typename = "gic-its", >>> + .startup = its_irq_startup, >>> + .shutdown = its_irq_shutdown, >>> + .enable = its_irq_enable, >>> + .disable = its_irq_disable, >>> + .ack = its_irq_ack, >>> + .end = gicv3_guest_irq_end, >>> + .set_affinity = its_irq_set_affinity, >>> +}; >>> + >>> +hw_irq_controller *its_get_host_lpi_type(void) >>> +{ >>> + return &its_host_lpi_type; >>> +} >>> + >>> +hw_irq_controller *its_get_guest_lpi_type(void) >>> +{ >>> + return &its_guest_lpi_type; >>> +} >>> + >> >> Please directly export the variables. > > These API's are called to assign hw_irq_handler to LPI > by generic code You didn't understand what I meant. I meant dropping static from the 2 variables and directly use them in the GICv3 code. The functions are pointless here and add another indirection for nothing. > >> >>> /* >>> * How we allocate LPIs: >>> * >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c >>> index 98d45bc..58e878e 100644 >>> --- a/xen/arch/arm/gic-v3.c >>> +++ b/xen/arch/arm/gic-v3.c >>> @@ -40,6 +40,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> >>> /* Global state */ >>> @@ -1033,15 +1034,19 @@ static void gicv3_irq_ack(struct irq_desc *desc) >>> /* No ACK -- reading IAR has done this for us */ >>> } >>> >>> -static void gicv3_host_irq_end(struct irq_desc *desc) >>> +void gicv3_host_irq_end(struct irq_desc *desc) >>> { >>> /* Lower the priority */ >>> gicv3_eoi_irq(desc); >>> - /* Deactivate */ >>> - gicv3_dir_irq(desc); >>> + /* >>> + * LPIs does not have active state. Do do not deactivate, >>> + * when EOI mode is set to 1. >>> + */ >>> + if ( !gic_is_lpi(desc->irq) ) >>> + gicv3_dir_irq(desc); >> >> I already told you that the goal of the hw_irq_controller is to avoid >> checking the interrupt for every callback. >> >> If the current function doesn't work for you, introduce a new one. But >> don't put an if inside. > > This is what I have done in patch v4 > > http://lists.xen.org/archives/html/xen-devel/2015-07/msg02128.html > > But Ian suggested to export gicv3_host_irq_end instead of calling > gicv3_eoi_irq() And Ian said "Exposing those two gicv3 functions is a bit unfortunate, but I think it will do for now.". Which means he was opposed to the previous solution. Anyway, I think it's a fair trade compare to the overhead you had for everytime you received an IRQ used by Xen. This is more true given that we don't even support LPI for Xen... Regards, -- Julien Grall