* [Xen-devel] [PATCH v2] xen/arm: Handle unimplemented VGICv3 dist registers as RAZ/WI
@ 2020-01-31 20:10 Jeff Kubascik
2020-02-01 11:45 ` Julien Grall
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Kubascik @ 2020-01-31 20:10 UTC (permalink / raw)
To: xen-devel; +Cc: Volodymyr Babchuk, Stefano Stabellini, Julien Grall
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Xen-devel] [PATCH v2] xen/arm: Handle unimplemented VGICv3 dist registers as RAZ/WI
2020-01-31 20:10 [Xen-devel] [PATCH v2] xen/arm: Handle unimplemented VGICv3 dist registers as RAZ/WI Jeff Kubascik
@ 2020-02-01 11:45 ` Julien Grall
2020-02-04 19:26 ` Jeff Kubascik
0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2020-02-01 11:45 UTC (permalink / raw)
To: Jeff Kubascik, xen-devel; +Cc: Stefano Stabellini, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Xen-devel] [PATCH v2] xen/arm: Handle unimplemented VGICv3 dist registers as RAZ/WI
2020-02-01 11:45 ` Julien Grall
@ 2020-02-04 19:26 ` Jeff Kubascik
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Kubascik @ 2020-02-04 19:26 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Stefano Stabellini, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-04 19:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 20:10 [Xen-devel] [PATCH v2] xen/arm: Handle unimplemented VGICv3 dist registers as RAZ/WI Jeff Kubascik
2020-02-01 11:45 ` Julien Grall
2020-02-04 19:26 ` Jeff Kubascik
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.