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/27/18 1:14 PM, Andrii Anisov wrote:
> Hello Julien,
>
>
> On 10/27/2018 12:55 AM, Julien Grall wrote:
> > The functions below does not exist in latest Xen. So are you sure this 
> > based on mainline?
> Yep, I've put a wrong diff into the email, it is for XEN 4.10.
> Please find the patch for XEN 4.12-unstable on my github [1].
>
> > How many domain do you have run at that time?
> Only one domain. In my setup I'm running a BSP release under XEN as Dom0.
>
> > Under which condition this is hapenning?
> Under no specific conditions. During the system boot, during console 
> operations, during glmark2 [2] running.
So the glmark2 is running in Dom0, am I correct?

> > Also, what is your Xen command line?
> `dom0_mem=1G console=dtuart dtuart=serial0 dom0_max_vcpus=4 bootscrub=0 
> loglvl=all cpufreq=none`
> > I assume you didn't disable serrors, right? So we should have plenty 
> > of dsb(sy) and isb() in the path to there.
> I didn't disable serrors.
>
> > 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()?
> Right you are, gic_clear_lrs(), in 4.10 codebase, rewrites LRs. But it 
> does not change PENDING to INVALID under no circumstances from one hand. 
> From another hand, all changes to LRs are made through gic specific 
> operations gic_hw_ops->... which are tracked by me. You can see, in the 
> code above, that I copy all updates to the physical LR issued by 
> hypervisor into the stored LRs. So it not the case. But I'll check on 
> Monday.
Ah, I missed the part where you updated the LRs.

While the interrupt exception path does not re-enable interrupts early. 
Other traps may do it.
On those trap, you would have the following path:
        1) Save GP registers
        2) Enable interrupts
        3) Store lrs

It is possible to receive an interrupts right after 2) and before 3). 
Because the current vGIC is touching the LRs directly (not the case on 
the new one). You may then end up with the information of the previous 
store.
Could you investigate how the guest has exited (i.e sync vs interrupt)?

Cheers,

--
Julien Grall

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