RE: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access

[Hongda Deng <Hongda.Deng@xxxxxxx>]

Hi,

Thanks for your great and detailed advice, I did some investigations about vgic 
especially inflight_irqs in the last few days.

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Sent: 2021年9月24日 4:54
> To: Julien Grall <julien@xxxxxxx>
> Cc: Hongda Deng <Hongda.Deng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei
> Chen <Wei.Chen@xxxxxxx>
> Subject: Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
> 
> On Thu, 23 Sep 2021, Julien Grall wrote:
> > Hi,
> >
> > On 23/09/2021 11:14, Hongda Deng wrote:
> > > Currently, Xen will return IO unhandled when guests access GICD ICPENRn
> > > registers. This will raise a data abort inside guest. For Linux Guest,
> > > these virtual registers will not be accessed. But for Zephyr, in its
> > > GIC initilization code, these virtual registers will be accessed. And
> > > zephyr guest will get an IO dataabort in initilization stage and enter
> >
> > s/dataabort/data abort/
> > s/initilization/initialization/
> >

Ack.

> > > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
> > > we currently ignore these virtual registers access and print a message
> > > about whether they are already pending instead of returning unhandled.
> > > More details can be found at [1].
> > >
> > > [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
> > > msg00744.html
> > >
> > > Signed-off-by: Hongda Deng <hongda.deng@xxxxxxx>
> > > ---
> > >   xen/arch/arm/vgic-v2.c     | 10 +++++++---
> > >   xen/arch/arm/vgic-v3.c     | 29 +++++++++++++++++------------
> > >   xen/arch/arm/vgic.c        | 37 +++++++++++++++++++++++++++++++++++++
> > >   xen/include/asm-arm/vgic.h |  2 ++
> > >   4 files changed, 63 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > > index b2da886adc..644c62757c 100644
> > > --- a/xen/arch/arm/vgic-v2.c
> > > +++ b/xen/arch/arm/vgic-v2.c
> > > @@ -481,10 +481,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
> > > mmio_info_t *info,
> > >         case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> > >           if ( dabt.size != DABT_WORD ) goto bad_width;
> > > +        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, 
> > > DABT_WORD);
> > > +        if ( rank == NULL ) goto write_ignore;
> >
> >
> >
> > > +
> > >           printk(XENLOG_G_ERR
> > > -               "%pv: vGICD: unhandled word write %#"PRIregister" to
> > > ICPENDR%d\n",
> > > -               v, r, gicd_reg - GICD_ICPENDR);
> > > -        return 0;
> > > +               "%pv: vGICD: unhandled word write %#"PRIregister" to
> > > ICPENDR%d, and current pending state is: %s\n",
> > > +               v, r, gicd_reg - GICD_ICPENDR,
> > > +               vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> >
> > Each register contain the information for multiple pending interrupts. So it
> > is a bit confusing to say whether the state is on/off. Instead, it would be
> > better to state which interrupt is pending.
> >
> > Also, I would rather avoid printing a message if there are no interrupts
> > pending because there are no issues if this is happening.

I will fix it in the next version patch.

> >
> > > +        goto write_ignore_32;
> > >         case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> > >           if ( dabt.size != DABT_WORD ) goto bad_width;
> > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > > index cb5a70c42e..c94e33ff4f 100644
> > > --- a/xen/arch/arm/vgic-v3.c
> > > +++ b/xen/arch/arm/vgic-v3.c
> > > @@ -817,10 +817,14 @@ static int
> __vgic_v3_distr_common_mmio_write(const
> > > char *name, struct vcpu *v,
> > >         case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> > >           if ( dabt.size != DABT_WORD ) goto bad_width;
> > > +        rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> > > +        if ( rank == NULL ) goto write_ignore;
> > > +
> > >           printk(XENLOG_G_ERR
> > > -               "%pv: %s: unhandled word write %#"PRIregister" to
> > > ICPENDR%d\n",
> > > -               v, name, r, reg - GICD_ICPENDR);
> > > -        return 0;
> > > +               "%pv: %s: unhandled word write %#"PRIregister" to 
> > > ICPENDR%d,
> > > and current pending state is: %s\n",
> > > +               v, name, r, reg - GICD_ICPENDR,
> > > +               vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> > > +        goto write_ignore_32;
> > >         case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> > >           if ( dabt.size != DABT_WORD ) goto bad_width;
> > > @@ -978,19 +982,20 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct
> vcpu
> > > *v, mmio_info_t *info,
> > >       case VREG32(GICR_ICFGR1):
> > >       case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
> > >       case VREG32(GICR_ISPENDR0):
> > > -         /*
> > > -          * Above registers offset are common with GICD.
> > > -          * So handle common with GICD handling
> > > -          */
> > > +        /*
> > > +         * Above registers offset are common with GICD.
> > > +         * So handle common with GICD handling
> > > +         */
> > >           return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
> > >                                                    info, gicr_reg, r);
> > >         case VREG32(GICR_ICPENDR0):
> > > -        if ( dabt.size != DABT_WORD ) goto bad_width;
> > > -        printk(XENLOG_G_ERR
> > > -               "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to
> > > ICPENDR0\n",
> > > -               v, r);
> > > -        return 0;
> > > +        /*
> > > +         * Above registers offset are common with GICD.
> > > +         * So handle common with GICD handling
> > > +         */
> > > +        return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
> > > +                                                 info, gicr_reg, r);
> > >         case VREG32(GICR_IGRPMODR0):
> > >           /* We do not implement security extensions for guests, write
> > > ignore */
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 8f9400a519..29a1aa5056 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -470,6 +470,43 @@ void vgic_set_irqs_pending(struct vcpu *v, uint32_t
> r,
> > > unsigned int rank)
> > >       }
> > >   }
> > >   +bool vgic_get_irqs_pending(struct vcpu *v, uint32_t r, unsigned int 
> > > rank)
> > > +{
> > > +    const unsigned long mask = r;
> > > +    unsigned int i;
> > > +    /* The first rank is always per-vCPU */
> > > +    bool private = rank == 0;
> > > +    bool is_pending = false;
> > > +
> > > +    /* LPIs status will never be retrieved via this function */
> > > +    ASSERT(!is_lpi(32 * rank + 31));
> > > +
> > > +    for_each_set_bit( i, &mask, 32 )
> > > +    {
> > > +        unsigned int irq = i + 32 * rank;
> > > +
> > > +        if ( !private )
> >
> > It is not clear to me why you not handling PPIs/SGIs and ...
> >
> > > +        {
> > > +            struct pending_irq *p = spi_to_pending(v->domain, irq);
> > > +
> > > +            if ( p->desc != NULL )
> >
> > ... emulated SPIs (e.g. PL011).
> >
> > > +            {
> > > +                unsigned long flags;
> > > +
> > > +                spin_lock_irqsave(&p->desc->lock, flags);
> > > +                is_pending = gic_read_pending_state(p->desc);
> > > +                spin_unlock_irqrestore(&p->desc->lock, flags);
> >
> > What you are reading here is the pending state from the HW. This is not the
> > same as the pending state from the VM PoV. In fact, in the most common case,
> > the interrupt will be pending from the VM PoV, but simply active from the HW
> > PoV (it is deactivated once the interrupt has been handled by the guest).
> >
> > I think what you want to check is whether the flag GIC_IRQ_GUEST_QUEUED
> is set
> > in p->status (Stefano ?).
> 
> Yeah, that's right. In fact, there is no need for checking the hardware
> registers. You can just go through the inflight_irqs list and print all
> of them (the list is sync on hyp entry on the cpu you are running on,
> not the others of course).
> 
> 
> > This is technically still a bit racy as Xen may still think the interrupt is
> > pending while the it may be actually active in the guest. AFAIK, the other 
> > way
> > around (i.e. not pending in Xen but pending in the guest) cannot happen.
> >
> > Anyway, this is just a message, so it is still better than crashing :).
> 
> +1

Thanks again for your advice. 
Based on that, I wrote a new patch to go through vcpu->arch.vgic.inflight_irqs 
to check the pending
states and print them if there are in the next version patch. I will send it 
for review later.

Cheers,
Hongda