Message ID | 20200131201046.44996-1-jeff.kubascik@dornerworks.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v2] xen/arm: Handle unimplemented VGICv3 dist registers as RAZ/WI | expand |
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: >
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
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:
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(-)