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

[Julien Grall <julien@xxxxxxx>]

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?
> [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.
> +        {
> +            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."

> +        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.
That said, the code is quite similar and not trivial. Can we provide an 
helper that can be used for the two GICs?
> +        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.

>           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