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

[Hongda Deng <Hongda.Deng@xxxxxxx>]

Hi,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年10月13日 5:58
> To: Hongda Deng <Hongda.Deng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>
> Subject: Re: [PATCH v2] xen/arm: vgic to ignore GICD ICPENRn registers access
> 
> Hi,
> 
> On 12/10/2021 07:24, 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 initialization code, these virtual registers will be accessed. And
> > zephyr guest will get an IO data abort in initilization stage and enter
> > 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].
> 
> The link you provide only states that I am happy with the warning. This
> doesn't seem relevant for a future reader. Did you intend to point to
> something different?
> 

Yes, I would attach this link [1] then, which explains how zephyr accesses 
ICPENDR at its initialization. ( Though I still don't understand why zephyr 
would clear this register at initialization while linux wouldn't )

> >
> > [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 | 26 +++++++++++++++++++++++++-
> >   xen/arch/arm/vgic-v3.c | 40 +++++++++++++++++++++++++++++++---------
> >   2 files changed, 56 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index b2da886adc..d7ffaeeb65 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -480,11 +480,35 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
> mmio_info_t *info,
> >           return 1;
> >
> >       case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> > +    {
> > +        struct pending_irq *iter;
> > +        unsigned int irq_start;
> > +        unsigned int irq_end;
> > +        uint32_t irq_pending = 0;
> > +
> >           if ( dabt.size != DABT_WORD ) goto bad_width;
> >           printk(XENLOG_G_ERR
> >                  "%pv: vGICD: unhandled word write %#"PRIregister" to
> ICPENDR%d\n",
> >                  v, r, gicd_reg - GICD_ICPENDR);
> 
> As I wrote in v1, we should avoid to print a message when we know there
> is no pending interrupts.
> 

These are not the modifications made in this patch, and same codes also exist
for ICACTIVER. I didn't delete them for that I think they are used to give some
useful and coherent messages to user for reference. Should we delete it for both
or only for ICPENDR?

> > -        return 0;
> > +
> > +        irq_start = (gicd_reg - GICD_ICPENDR) * 32;
> > +        irq_end = irq_start + 31;
> > +        /* go through inflight_irqs and print specified pending irqs */
> > +        list_for_each_entry(iter, &v->arch.vgic.inflight_irqs, inflight)
> You need to hold v->arch.vgic.lock (with interrupt disabled) to go
> through the list of inflight irqs. Otherwise, the list may be modified
> while you are walking it.
> 

Ack.

> However, I am a little bit concerned with this approached (I noticed
> Stefano suggested). The list may in theory contains a few hundreds
> interrupts (a malicious OS May decide to never read IAR). So we are
> potentially doing more work than necessary (we need to think about the
> worse case scenario).
> 
> Instead, I think it would be better to go through the 32 interrupts and
> for each of them:
>    1) find the pending_irq() using irq_to_pending()
>    2) check if the IRQ in the inflight list with list_empty(&p->inflight)
> 
> In addition to that, you want to check that the rank exists so we don't
> do any extra work if the guest is trying to clear an interrupts above
> the number of interrupts we support.
> 

Agreed, and that's quite helpful.

> > +        {
> > +            if ( iter->irq < irq_start || irq_end < iter->irq )
> > +                continue;
> > +
> > +            if ( test_bit(GIC_IRQ_GUEST_QUEUED, &iter->status) )
> > +                irq_pending = irq_pending | (1 << (iter->irq - irq_start));
> > +        }
> > +
> > +        if ( irq_pending != 0 )
> > +            printk(XENLOG_G_ERR
> > +                   "%pv: vGICD: ICPENDR%d=0x%08x\n",
> > +                   v, gicd_reg - GICD_ICPENDR, irq_pending);
> 
> This message is a bit confusing. I think it would be worth to print a
> message for every interrupt not cleared. Maybe something like:
> 
> "%pv trying to clear pending interrupt %u."
> 

I was thinking that there may be too many interrupts there, to print each with
one message line would be too superfluous. 
But that worst case scenario should not be usual, and print a message for each 
interrupt would be much clearer.

Ack.

> > +        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..243b24e496 100644
> > --- a/xen/arch/arm/vgic-v3.c
> > +++ b/xen/arch/arm/vgic-v3.c
> > @@ -816,11 +816,35 @@ static int
> __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
> >           return 1;
> >
> >       case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> > +    {
> > +        struct pending_irq *iter;
> > +        unsigned int irq_start;
> > +        unsigned int irq_end;
> > +        uint32_t irq_pending = 0;
> > +
> >           if ( dabt.size != DABT_WORD ) goto bad_width;
> >           printk(XENLOG_G_ERR
> >                  "%pv: %s: unhandled word write %#"PRIregister" to 
> > ICPENDR%d\n",
> >                  v, name, r, reg - GICD_ICPENDR);
> > -        return 0;
> > +
> > +        irq_start = (reg - GICD_ICPENDR) * 32;
> > +        irq_end = irq_start + 31;
> > +        /* go through inflight_irqs and print specified pending irqs */
> > +        list_for_each_entry(iter, &v->arch.vgic.inflight_irqs, inflight)
> > +        {
> > +            if ( iter->irq < irq_start || irq_end < iter->irq )
> > +                continue;
> > +
> > +            if ( test_bit(GIC_IRQ_GUEST_QUEUED, &iter->status) )
> > +                irq_pending = irq_pending | (1 << (iter->irq - irq_start));
> > +        }
> > +
> > +        if ( irq_pending != 0 )
> > +            printk(XENLOG_G_ERR
> > +                   "%pv: %s: ICPENDR%d=0x%08x\n",
> > +                   v, name, reg - GICD_ICPENDR, irq_pending);
> 
> My remarks apply for GICv3 as well. Note that in the case of GICv3 v may
> not be current.
> 

I am a bit confused why v may not be current for GICv3? Does that mean 
that for SPI we should use vgic_get_target_vcpu() to get its correct vcpu
on GICv3 and multi cores?

> That said, the code is quite similar and not trivial. Can we provide an
> helper that can be used for the two GICs?
> 

Yes, to provide a helper function in vgic.c would be more helpful.

> > +        goto write_ignore_32;
> > +    }
> >
> >       case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> >           if ( dabt.size != DABT_WORD ) goto bad_width;
> > @@ -978,19 +1002,17 @@ 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
> > +        */
> 
> This looks like a spurious change.
> 

I moved this comment to the left by one space to apply format style
to be coherent with others.
I will undo this modification and write another patch to fix it if needed.

> >           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;
> > +        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 */
> >
> 
> Cheers,
> 
> --
> Julien Grall

This is quite a good approach as Stefano said and helps a lot to me.

But I am still confused about the difference between vGICv2 and
vGICv3 on checking inflight irqs.
I will send patch version3 ASAP.

[1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274

Cheers,
Hongda