From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v6 13/31] xen/arm: ITS: implement hw_irq_controller for LPIs Date: Mon, 31 Aug 2015 18:53:48 +0100 Message-ID: <55E494AC.7090907@citrix.com> References: <1441019208-2764-1-git-send-email-vijay.kilari@gmail.com> <1441019208-2764-14-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1441019208-2764-14-git-send-email-vijay.kilari@gmail.com> 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@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, Vijaya Kumar K , Zoltan Kiss , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, On 31/08/2015 12:06, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > Implements hw_irq_controller api's required > to handle LPI's. > > Changed callbacks gic_host_irq_type and gic_guest_irq_type s/Changed/Change the/ > to gic_get_host_irq_type and gic_get_guest_irq_type > in gic_hw_operations, which returns > hw_irq_controller based on irq type (SPI or LPI). > > Signed-off-by: Vijaya Kumar K > CC: Zoltan Kiss > --- > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index f14c0f4..0865a93 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -349,6 +349,19 @@ post: > its_wait_for_range_completion(its, cmd, next_cmd); > } > > +static void its_send_inv(struct its_device *dev, u32 event) See my point of view of about this function living here in my answer to patch #6. [...] > +static void its_host_irq_end(struct irq_desc *desc) > +{ > + /* Lower the priority */ > + gicv3_eoi_irq(desc); > + /* LPIs does not have active state. Do not deactivate */ > +} > + > +static void its_guest_irq_end(struct irq_desc *desc) > +{ > + gicv3_eoi_irq(desc); > +} > + > +static void its_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask) > +{ > + /*TODO: Yet to support */ > + printk(XENLOG_G_WARNING > + "%pv: ITS: Setting Affinity of LPI is not supported\n", current); Printing the current vCPU is hazardous, this function can be called at any time and the current may not be meaningful. > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index e90e0cc..c3b1a7c 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 */ > @@ -54,6 +55,8 @@ static struct { > } gicv3; > > static struct gic_info gicv3_info; > +extern const hw_irq_controller its_host_lpi_type; > +extern const hw_irq_controller its_guest_lpi_type; You should define the variable in gic-its.h in order to let the compiler check that we are using the same as the declaration. > /* per-cpu re-distributor base */ > DEFINE_PER_CPU(struct rdist, rdist); [...] > diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h > index 3599c76..7a46e21 100644 > --- a/xen/include/asm-arm/gic-its.h > +++ b/xen/include/asm-arm/gic-its.h > @@ -279,6 +279,7 @@ void irqdesc_set_lpi_event(struct irq_desc *desc, unsigned id); > unsigned int irqdesc_get_lpi_event(struct irq_desc *desc); > struct its_device *irqdesc_get_its_device(struct irq_desc *desc); > void irqdesc_set_its_device(struct irq_desc *desc, struct its_device *dev); > +bool_t is_valid_collection(struct domain *d, uint32_t col); Where does it come from? I don't see any usage nor implementation of it within the patch. > int its_init(struct rdist_prop *rdists); > int its_cpu_init(void); > int its_add_device(u32 devid, u32 nr_ites, struct dt_device_node *dt_its); > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index d39e1b3..6ece7cc 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -317,10 +317,10 @@ struct gic_hw_operations { > void (*dump_state)(const struct vcpu *); > > /* hw_irq_controller to enable/disable/eoi host irq */ > - hw_irq_controller *gic_host_irq_type; > + const hw_irq_controller *(*gic_get_host_irq_type)(unsigned int irq); > > /* hw_irq_controller to enable/disable/eoi guest irq */ > - hw_irq_controller *gic_guest_irq_type; > + const hw_irq_controller *(*gic_get_guest_irq_type)(unsigned int irq); Actually the const was already embedded in hw_irq_controller (see the typedef in xen/include/xen/irq.h). Sorry for the inconvenience. > > /* End of Interrupt */ > void (*eoi_irq)(struct irq_desc *irqd); Regards, -- Julien Grall