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 <[email protected]> > Sent: 2021年9月24日 4:54 > To: Julien Grall <[email protected]> > Cc: Hongda Deng <[email protected]>; [email protected]; > [email protected]; Bertrand Marquis <[email protected]>; Wei > Chen <[email protected]> > 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 <[email protected]> > > > --- > > > 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
