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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 12 Oct 2021, Julien Grall wrote:
> 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?
> 
> > 
> > [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.
> 
> > -        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.
> 
> 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.

This is a good approach and it should be fast.

In the normal case, iterating over inflight_irqs should be fine because
there are typically no more than 2-3 inflight interrupts. So in the
normal case iterating over inflight_irqs will be typically faster but in
the worst case it could be bad because the theoretical max is high: it
is only bounded by the total amount of interrupts assigned to the domU
and by the vgic IRQ limit. So theoretically it is possible that we could
have to walk a list with a hundred elements or more.

Your suggestion has the advantage that the compute time becomes
deterministic. Alternatively, we could #ifdef DEBUG the walk over the
inflight_irqs list. I am fine either way but I prefer your suggestion.