Re: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers access

[Julien Grall <julien@xxxxxxx>]

Hi Hongda,

Title: I would suggest the following title:

xen/arm: vgic: Ignore write access to ICPENDR*

On 20/10/2021 11:10, 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
Typo: s/initilization/initialization/

I would also s/in/during the/

> fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
How about s/enter fatal error/crash/?

> we currently ignore these virtual registers access and print a message
To me 'currently' refers to the existing code base (i.e. without your 
patch). In fact, this seems to be how you use 'currently' in the first 
paragraph. So how about replace "so we currently" with "rework the 
emulation to ignore...".
This seems to suggest the patch will modify both read and write access. 
However, AFAICT, only the write emulation is modified. Can this be 
clarified in the commit message?
> about whether they are already pending instead of returning unhandled.
> More details can be found at [1].
>
> [1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
> cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274
>
> Signed-off-by: Hongda Deng <hongda.deng@xxxxxxx>
> ---
> Changes since v2:
>   *  Avoid to print messages when there is no pending interrupt
>   *  Add helper vgic_check_inflight_irqs_pending to check pending status
>   *  Print a message for each interrupt separately
> Changes since v1:
>   *  Check pending states by going through vcpu->arch.vgic.inflight_irqs
>      instead of checking hardware registers
> ---
>   xen/arch/arm/vgic-v2.c     | 10 ++++++----
>   xen/arch/arm/vgic-v3.c     | 16 ++++++++--------
>   xen/arch/arm/vgic.c        | 36 ++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/vgic.h |  3 ++-
>   4 files changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b2da886adc..7c30da327c 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -481,10 +481,12 @@ 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;
> -        printk(XENLOG_G_ERR
> -               "%pv: vGICD: unhandled word write %#"PRIregister" to 
> ICPENDR%d\n",
> -               v, r, gicd_reg - GICD_ICPENDR);
> -        return 0;
> +        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
> +        if ( rank == NULL ) goto write_ignore;
> +
> +        vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r); > +
> +        goto write_ignore_32;
NIT: We already check the access above. So I would simply use 
"write_ignore" here.
>   
>       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..4913301d22 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -817,10 +817,12 @@ 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;
> -        printk(XENLOG_G_ERR
> -               "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
> -               v, name, r, reg - GICD_ICPENDR);
> -        return 0;
> +        rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> +        if ( rank == NULL ) goto write_ignore;
> +
> +        vgic_check_inflight_irqs_pending(v->domain, v, rank->index, r);
> +
> +        goto write_ignore_32;
NIT: Same remark as the previous write_ignore_32.

>   
>       case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
>           if ( dabt.size != DABT_WORD ) goto bad_width;
> @@ -987,10 +989,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>   
>       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 */
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 8f9400a519..0565557814 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -726,6 +726,42 @@ unsigned int vgic_max_vcpus(unsigned int 
> domctl_vgic_version)
>       }
>   }
>   
> +void vgic_check_inflight_irqs_pending(struct domain *d, struct vcpu *v,
> +                                      unsigned int rank, uint32_t r)
> +{
> +    const unsigned long mask = r;
> +    unsigned int i;
> +    unsigned long flags;
> +    struct pending_irq *p;
> +    bool private = rank == 0;
> +    struct vcpu *v_target;
AFAIC, flags, p, v_target are only used within the loop. So please 
reduce the scope and only declare them in for_each_set_bit().
> +
> +    for_each_set_bit( i, &mask, 32 )
> +    {
> +        unsigned int irq = i + 32 * rank;
> +
> +        if ( private )
> +            v_target = vgic_get_target_vcpu(v, irq);
> +        else
> +            v_target = vgic_get_target_vcpu(d->vcpu[0], irq);
Shared interrupts can be accessed from any vCPU. So you can replace the 
4 lines with:
   v_target = vgic_get_target_vcpu(v, irq);

> +
> +        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> +
> +        p = irq_to_pending(v_target, irq);
> +
> +        if ( unlikely(!p) )
> +        {
> +            spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> +            continue;
> +        }
irq_to_pending() cannot return NULL for non-LPI interrupts. But if you 
still want to check it, then the two if can be combined to something like:
if ( p && !list_empty(&p->inflight) )
  printk(...)

spin_unlock_irqrestore(...);

> +
> +        if ( !list_empty(&p->inflight) )
> +            printk("%pv trying to clear pending interrupt %u.\n", v, irq);
This wants to be a printk(XENLOG_G_WARNING ...) so the message will be 
appropriately rate-limited.
> +
> +        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> +    }
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 62c2ae538d..abcaae2969 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -298,7 +298,8 @@ extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
>                           enum gic_sgi_mode irqmode, int virq,
>                           const struct sgi_target *target);
>   extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int 
> irq);
> -
> +extern void vgic_check_inflight_irqs_pending(struct domain *d, struct vcpu *v,
> +                                             unsigned int rank, uint32_t r);
Please keep the newline before the #endif.

>   #endif /* !CONFIG_NEW_VGIC */
>   
>   /*** Common VGIC functions used by Xen arch code ****/
Cheers,

--
Julien Grall