Re: [Xen-devel] [PATCH v2] xen/arm: Handle unimplemented VGICv3 dist registers as RAZ/WI

[Julien Grall <julien@xxxxxxx>]

Hi,

On 31/01/2020 20:10, Jeff Kubascik wrote:
> Per the ARM Generic Interrupt Controller Architecture Specification (ARM
> IHI 0069E), reserved registers should generally be treated as RAZ/WI.
> To simplify the VGICv3 design and improve guest compatability, treat the
Typo: compatibility

> default case for GICD registers as read_as_zero/write_ignore.
I would prefer if we try to keep the emulation of all the registers the 
same way. I.e if GICD default case is now RAZ/WI, then all the other 
regions (e.g GICR) should do the same.
I will look to write a patch similar for GICv2 as well.

> Signed-off-by: Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>
> ---
>   xen/arch/arm/vgic-v3.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 422b94f902..8d0856ac33 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1250,9 +1250,9 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>            goto read_impl_defined;
>   
>       default:
> -        printk(XENLOG_G_ERR "%pv: vGICD: unhandled read r%d offset %#08x\n",
> -               v, dabt.reg, gicd_reg);
> -        return 0;
> +        /* Since reserved registers should be read-as-zero, make this the
> +         * default case */
This comment is misleading because the default case doesn't only handle 
reserved registers. A good example is GICD_IGRPMODR will use the default 
label. Yet it is not a reserved registers. Some of the reserved 
registers may also be allocated in the future (i.e with GICv4). So I 
would drop the comment here.
I would also like to keep a print (at least in debug build) as it could 
be helpful for an OS developper (or even Xen one) to detect any register 
we implement as RAZ/WI but should not.
As an aside, the coding style for multi-lines comment on Xen is:

/*
 * Foo
 * Bar
 */

> +        goto read_as_zero;
>       }
>   
>   bad_width:
> @@ -1435,10 +1435,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>            goto write_impl_defined;
>   
>       default:
> -        printk(XENLOG_G_ERR
> -               "%pv: vGICD: unhandled write r%d=%"PRIregister" offset %#08x\n",
> -               v, dabt.reg, r, gicd_reg);
> -        return 0;
> +        /* Since reserved registers should be write-ignore, make this the
> +         * default case */
> +        goto write_ignore;
Same comments.

>       }
>   
>   bad_width:
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel