From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: unhandled word causes Xen crash with recent Linux kernels, was: Re: [PATCH v2 05/11] xen/arm: vgic: Properly emulate the full register Date: Mon, 30 Nov 2015 12:18:57 +0000 Message-ID: <565C3EB1.4070505@citrix.com> References: <1447867686-19371-1-git-send-email-julien.grall@citrix.com> <1447867686-19371-6-git-send-email-julien.grall@citrix.com> <56557EE0.6010507@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a3NRl-0007mh-QK for xen-devel@lists.xenproject.org; Mon, 30 Nov 2015 12:20:45 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini , Shannon Zhao Cc: xen-devel@lists.xenproject.org, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org Hi Stefano, On 25/11/15 12:15, Stefano Stabellini wrote: > Hi Shannon, > > On Wed, 25 Nov 2015, Shannon Zhao wrote: >> Upstream Linux kernel applies below patch which will write >> GICD_ICACTIVER. But since Xen doesn't support it, so it will cause Dom0 >> initializes GIC failed. >> >> 0eece2b22849c90b730815c893425a36b9d10fd5 (irqchip/gic: Make sure all >> interrupts are deactivated at boot) >> >> (XEN) d0v0: vGICD: unhandled word write 0xffffffff to ICACTIVER4 >> (XEN) traps.c:2447:d0v0 HSR=0x93860046 pc=0xffffffc0008d63f0 >> gva=0xffffff8000004384 gpa=0x0000002f000384 >> (XEN) DOM0: Unhandled fault: ttbr address size fault (0x96000000) at >> 0xffffff8000004384 >> (XEN) DOM0: Internal error: : 96000000 [#1] PREEMPT SMP >> (XEN) DOM0: Modules linked in: >> (XEN) DOM0: CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.0-rc2+ #364 >> (XEN) DOM0: Hardware name: (null) (DT) >> (XEN) DOM0: task: ffffffc000969970 ti: ffffffc00095c000 task.ti: >> ffffffc00095c000 >> (XEN) DOM0: PC is at gic_dist_config+0x78/0xa0 >> (XEN) DOM0: LR is at __gic_init_bases+0x240/0x2bc >> >> Do we have a plan to fix this? > > Thanks for the reporting the issue, I can reproduce the problem. Given > that this is a very serious regression and that we cannot really "fix" > the Linux side because Linux is not doing anything wrong, I think we > have to go with a very simple change, something we can easily backport > to all past Xen releases. > > I suggest we turn the "unhandled word write" into a write_ignore, see > below: > > --- > > xen/arm: ignore GICD_ICACTIVER writes This need more rational in the commit message to explain why you decided to implement write ignore. > > Signed-off-by: Stefano Stabellini > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > index f7d784b..8585c44 100644 > --- a/xen/arch/arm/vgic-v2.c > +++ b/xen/arch/arm/vgic-v2.c > @@ -332,11 +332,8 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info, > return 0; > > case GICD_ICACTIVER ... GICD_ICACTIVERN: > - if ( dabt.size != DABT_WORD ) goto bad_width; > - printk(XENLOG_G_ERR > - "%pv: vGICD: unhandled word write %#"PRIregister" to ICACTIVER%d\n", > - v, r, gicd_reg - GICD_ICACTIVER);implementing write ignore is fine. > - return 0; I would prefer if you retain the printk, it helps the guest developer to know that we don't support GICD_I*ACTIVER registers. Maybe you can turn it to a XENLOG_G_DEBUG. > + /* we should really be implementing this */ > + goto write_ignore_32; > > case GICD_ITARGETSR ... GICD_ITARGETSR + 7: > /* SGI/PPI target is read only */ > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index b5249ff..6d77373 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -421,11 +421,8 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v, > return 0; > > case GICD_ICACTIVER ... GICD_ICACTIVERN: > - if ( dabt.size != DABT_WORD ) goto bad_width; > - printk(XENLOG_G_ERR > - "%pv: %s: unhandled word write %#"PRIregister" to ICACTIVER%d\n", > - v, name, r, reg - GICD_ICACTIVER); Ditto > - return 0; > + /* we should really be implementing this */ > + goto write_ignore_32; > > case GICD_IPRIORITYR ... GICD_IPRIORITYRN: > if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; > Regards, -- Julien Grall