[v2] xen/arm: Handle unimplemented VGICv3 dist registers as RAZ/WI

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

Posted by Jeff Kubascik 4 years, 2 months ago

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
default case for GICD registers as read_as_zero/write_ignore.

Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
---
 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 */
+        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;
     }
 
 bad_width:
-- 
2.17.1


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

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

Posted by Julien Grall 4 years, 2 months ago

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@dornerworks.com>
> ---
>   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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Posted by Jeff Kubascik 4 years, 2 months ago

Hey Julien,

On 2/1/2020 6:45 AM, Julien Grall wrote:
> 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

Good catch, I will correct.

>> 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.

Should be easy enough to make the same change for the redist.

> I will look to write a patch similar for GICv2 as well.

Great!

>>
>> Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
>> ---
>>   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.

Sure thing, I'll drop the comment.

> 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.

I'll add the printk back in.

> As an aside, the coding style for multi-lines comment on Xen is:
> 
> /*
>   * Foo
>   * Bar
>   */

Thanks for pointing this out.

>> +        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.

Understood :)

>>       }
>>
>>   bad_width:
>>
> 
> --
> Julien Grall
> 

Sincerely,
Jeff Kubascik

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