Re: unhandled word causes Xen crash with recent Linux kernels, was: Re: [PATCH v2 05/11] xen/arm: vgic: Properly emulate the full register

[Julien Grall <julien.grall@citrix.com>]

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 <stefano.stabellini@eu.citrix.com>
> 
> 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