Re: [Xen-devel] [PATCH v6 13/31] xen/arm: ITS: implement hw_irq_controller for LPIs

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 31/08/2015 12:06, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>
> 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 <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> CC: Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>
> ---
> 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 <asm/device.h>
>   #include <asm/gic.h>
>   #include <asm/gic_v3_defs.h>
> +#include <asm/gic-its.h>
>   #include <asm/cpufeature.h>
>
>   /* 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel