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

[Julien Grall <julien.grall@xxxxxxx>]

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 which
> exposes the issue.
The functions below does not exist in latest Xen. So are you sure this 
based on mainline?
> With that diff I see (on my board) outputs like:
>
>
>       (XEN) Stored LR is stale: stored 0xAA025C97 read 0x8A025C97
>       (XEN) Stored LR is stale: stored 0x9A025C97 read 0x8A025C97
>       (XEN) Stored LR is stale: stored 0x9A025C97 read 0x8A025C97
>       (XEN) Stored LR is stale: stored 0x9A025C97 read 0x8A025C97
>       (XEN) Stored LR is stale: stored 0x1A00001B read 0xA00001B
>       (XEN) Stored LR is stale: stored 0x9A025C97 read 0x8A025C97
>       (XEN) Stored LR is stale: stored 0xAA025C97 read 0x8A025C97
How many domain do you have run at that time? Under which condition this 
is hapenning?
Also, what is your Xen command line? I assume you didn't disable 
serrors, right? So we should have plenty of dsb(sy) and isb() in the 
path to there.
So I would rule out a missing barrier here.

> Those prints I treat as LR content change (state PENDING->INVALID)
> without exiting from hyp to guest. Unfortunately, I did not find a kind
> of explanation to this effect in ARM IHI 0048B.b document.
> I have GICv2 on the die.
>
> I appreciate you find some time to look at this and express your opinion.
>
>
> ---
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 511c8d7..0b9aa2d 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -171,6 +171,14 @@ static unsigned int gicv2_cpu_mask(const cpumask_t
> *cpumask)
>        return mask;
>    }
>
> +static void gicv2_store_lrs(void)
> +{
> +    int i;
> +
> +    for ( i = 0; i < gicv2_info.nr_lrs; i++ )
> +        current->arch.gic.v2.lr[i] = readl_gich(GICH_LR + i * 4);
> +}
> +
>    static void gicv2_save_state(struct vcpu *v)
>    {
>        int i;
> @@ -446,11 +454,13 @@ static void gicv2_update_lr(int lr, const struct
> pending_irq *p,
>                                       << GICH_V2_LR_PHYSICAL_SHIFT);
>
>        writel_gich(lr_reg, GICH_LR + lr * 4);
> +    current->arch.gic.v2.lr[lr] = lr_reg;
>    }
>
>    static void gicv2_clear_lr(int lr)
>    {
>        writel_gich(0, GICH_LR + lr * 4);
> +    current->arch.gic.v2.lr[lr] = 0;
>    }
>
>    static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> @@ -458,6 +468,9 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>        uint32_t lrv;
>
>        lrv          = readl_gich(GICH_LR + lr * 4);
> +    if ( lrv != current->arch.gic.v2.lr[lr])
> +        printk("Stored LR is stale: stored 0x%"PRIX32" read 0x%"PRIX32"\n",
> +               current->arch.gic.v2.lr[lr], lrv);
>        lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) &
> GICH_V2_LR_PHYSICAL_MASK;
>        lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) &
> GICH_V2_LR_VIRTUAL_MASK;
>        lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) &
> GICH_V2_LR_PRIORITY_MASK;
> @@ -481,6 +494,7 @@ static void gicv2_write_lr(int lr, const struct
> gic_lr *lr_reg)
>              ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) <<
> GICH_V2_LR_GRP_SHIFT) );
>
>        writel_gich(lrv, GICH_LR + lr * 4);
> +    current->arch.gic.v2.lr[lr] = lrv;
>    }
>
>    static void gicv2_hcr_status(uint32_t flag, bool status)
> @@ -1232,6 +1246,7 @@ const static struct gic_hw_operations gicv2_ops = {
>        .info                = &gicv2_info,
>        .init                = gicv2_init,
>        .secondary_init      = gicv2_secondary_cpu_init,
> +    .store_lrs           = gicv2_store_lrs,
>        .save_state          = gicv2_save_state,
>        .restore_state       = gicv2_restore_state,
>        .dump_state          = gicv2_dump_state,
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ed363f6..a05c518 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -594,6 +594,12 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>        }
>    }
>
> +void gic_store_lrs(void)
> +{
> +    if(gic_hw_ops->store_lrs)
> +        gic_hw_ops->store_lrs();
> +}
> +
>    void gic_clear_lrs(struct vcpu *v)
>    {
>        int i = 0;
> 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 enter_hypervisor_head(struct
> cpu_user_regs *regs)
>            if ( current->arch.hcr_el2 & HCR_VA )
>                current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>
> +        gic_store_lrs();
gic_clear_lrs(...) may rewrite the LRs. To confirm this stall LRs are 
not due by this function, can you move that after gic_clear_lrs()?
>            gic_clear_lrs(current);
>        }
>    }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index d3d7bda..6fe3fdb 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -293,6 +293,7 @@ extern unsigned int gic_number_lines(void);
>    /* IRQ translation function for the device tree */
>    int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>                      unsigned int *out_hwirq, unsigned int *out_type);
> +void gic_store_lrs(void);
>    void gic_clear_lrs(struct vcpu *v);
>
>    struct gic_info {
> @@ -314,6 +315,8 @@ struct gic_hw_operations {
>        /* Initialize the GIC and the boot CPU */
>        int (*init)(void);
>        /* Save GIC registers */
> +    void (*store_lrs)(void);
> +    /* Save GIC registers */
>        void (*save_state)(struct vcpu *);
>        /* Restore GIC registers */
>        void (*restore_state)(const struct vcpu *);
Cheers,

--
Julien Grall

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