Re: [Xen-devel] [Xen-20] Clearing of GICD_ICACTIVER register at boot time.

[Julien Grall <julien.grall@xxxxxxx>]

Hello,

Please use scripts/get_maintainers.pl to CC all the relevant maintainers 
on your patch.
Regarding the title, Xen is supporting multiple architecture. So it is 
always useful to specify the architecture in the title.
Also the title is quite confusing as for clearing interrupts, you need 
to write 1s in that register.
A better title would be:

xen/arm: gic: Clear active state of IRQs when the GIC is initialized

On 05/06/18 20:20, cdeshpan wrote:
> From: Chaitanya Deshpande <chaitanyagd11@xxxxxxxxx>
Please explain in the commit message why you need to clear the interrupts.

> Signed-off-by: Chaitanya Deshpande <chaitanyagd11@xxxxxxxxx>
> ---
>   xen/arch/arm/gic-v2.c            | 6 +++++-
>   xen/arch/arm/gic-v3.c            | 5 +++++
>   xen/arch/arm/vgic-v2.c           | 5 +++++
>   xen/arch/arm/vgic-v3.c           | 5 +++++
>   xen/arch/arm/vgic/vgic-mmio-v2.c | 5 +++++
>   5 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index d2dcafb..6bd7e43 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -5,7 +5,6 @@
>    *
>    * Tim Deegan <tim@xxxxxxx>
>    * Copyright (c) 2011 Citrix Systems.
> - *
Please don't remove unnecessary line.

>    * This program is free software; you can redistribute it and/or modify
>    * it under the terms of the GNU General Public License as published by
>    * the Free Software Foundation; either version 2 of the License, or
> @@ -1247,6 +1246,11 @@ static int __init gicv2_init(void)
>   {
>       uint32_t aliased_offset = 0;
>
> +    /* Clear GIC register at boot time */
This comment seems incorrect.

> +    uint32_t *gicd_icactiver;
> +    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
> +    *gicd_icactiver = 0;
That code is not going to do anything if you write 0. Per the spec, you 
need to set 1 in order to deactivate interrupts.
Furthermore, this is only covering the first 32 interrupts. You have to 
clear the active state of all the interrupts. The first 32 interrupts 
are banked (i.e per-CPU) the others are shared.
The former should be initialized in gicv{2,3}_cpu_init that will be 
called during each CPU initialization. The latter should be done in 
gicv{2,3}_dist_init().
Lastly, GICD_ICACTIVER is just a register offset in the distributor. You 
have to add the base address in order to find to memory address to 
write. We have an helper for that in the GICv2 driver, see 
writel_gicd(...). For GICv3, you will have to use writel_relaxed(..., 
GICD + GICD_ICACTIVER).
I would also recommend to test your patches as very likely the latest 
error would have been caught.
> +
>       if ( acpi_disabled )
>           gicv2_dt_init();
>       else
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index b2ed0f8..f61d2b3 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1738,6 +1738,11 @@ static int __init gicv3_init(void)
>       uint32_t reg;
>       unsigned int intid_bits;
>
> +    /* Clear GIC register at boot time */
> +    uint32_t *gicd_icactiver;
> +    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
> +    *gicd_icactiver = 0;
See my remark above.

> +
>       if ( !cpu_has_gicv3 )
>       {
>           dprintk(XENLOG_ERR, "GICv3: driver requires system register 
> support\n");
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 646d1f3..d8b2761 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -727,6 +727,11 @@ static const struct vgic_ops vgic_v2_ops = {
>
>   int vgic_v2_init(struct domain *d, int *mmio_count)
>   {
> +    /* Clear GIC register at boot time */
> +    uint32_t *gicd_icactiver;
> +    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
> +    *gicd_icactiver = 0;
I don't understand why you are clearing active register in 
vgic_v{2,3}_init. Those functions are called when creating a domain, so 
there are no need to mess with the physical interrupts here.
> +
>       if ( !vgic_v2_hw.enabled )
>       {
>           printk(XENLOG_G_ERR
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 4b42739..6fe0dac 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1791,6 +1791,11 @@ static const struct vgic_ops v3_ops = {
>
>   int vgic_v3_init(struct domain *d, int *mmio_count)
>   {
> +    /* Clear GIC register at boot time */
> +    uint32_t *gicd_icactiver;
> +    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
> +    *gicd_icactiver = 0;
> +
See above.

>       if ( !vgic_v3_hw.enabled )
>       {
>           printk(XENLOG_G_ERR
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index 2e507b1..863b4d6 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -305,6 +305,11 @@ static const struct vgic_register_region 
> vgic_v2_dist_registers[] = {
>
>   unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev)
>   {
> +    /* Clear GIC register at boot time */
> +    uint32_t *gicd_icactiver;
> +    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
> +    *gicd_icactiver = 0;
See above.

> + >       dev->regions = vgic_v2_dist_registers;
>       dev->nr_regions = ARRAY_SIZE(vgic_v2_dist_registers);
>
> --
> 2.7.4
>
> This email and any attachments are intended for the sole use of the named 
> recipient(s) and contain(s) confidential information that may be proprietary, 
> privileged or copyrighted under applicable law. If you are not the intended 
> recipient, do not read, copy, or forward this email message or any attachments. 
> Delete this email message and any attachments immediately.
I am afraid I am not going to be able to accept any patch containing 
that paragraph in the signature (see [1]). This is a public mailing 
list, so everything is public. Please resend your patch without it if it 
was intended to be merged.
[1] https://www.youtube.com/watch?v=fMeH7wqOwXA&feature=youtu.be&t=13m53s

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel