Re: [Xen-devel] [PATCH v2 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers
Hi Ian, On 08/05/15 11:16, Ian Campbell wrote: > On Sat, 2015-04-25 at 22:16 +0500, Julien Grall wrote: >> Hi Ian, >> >> On 17/04/2015 19:01, Ian Campbell wrote: >>> Signed-off-by: Ian Campbell >>> --- >>> v2: Move last paramter of a handle_ro_raz call to next patch where it >>> belongs. >>> --- >>> xen/arch/arm/traps.c | 52 >>> -- >>> 1 file changed, 33 insertions(+), 19 deletions(-) >>> >>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>> index 8b1846a..b54aef6 100644 >>> --- a/xen/arch/arm/traps.c >>> +++ b/xen/arch/arm/traps.c >>> @@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs, >>> advance_pc(regs, hsr); >>> } >>> >>> +/* Write only + write ignore */ >> >> [..] >> >>> +/* Read only + read as zero */ >> >> I'm not sure if we finished the discussion on those comment on v1 before >> you sent the v2. > > I think we hadn't. > >> The "+" is very confusing for me because it indicates two parts: write >> only and write ignore (same for the read). Both part doesn't really fit >> together. Although this helper clearly choose to implement WO as WI >> (resp. RO as RAZ). > >> I think this should be clearer in order to avoid people think this can >> be used for RO but with a different value than 0. > > For v3 I've made the change I proposed in > <1429266891.25195.260.ca...@citrix.com> > > Specifically "Write only as write ignore" and "Read only as read as > zero" (essentially s/+/as/) > > Is that clear enough do you think? Yes. Thanks. With this changes: Reviewed-by: Julien Grall Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers
On Sat, 2015-04-25 at 22:16 +0500, Julien Grall wrote: > Hi Ian, > > On 17/04/2015 19:01, Ian Campbell wrote: > > Signed-off-by: Ian Campbell > > --- > > v2: Move last paramter of a handle_ro_raz call to next patch where it > > belongs. > > --- > > xen/arch/arm/traps.c | 52 > > -- > > 1 file changed, 33 insertions(+), 19 deletions(-) > > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > > index 8b1846a..b54aef6 100644 > > --- a/xen/arch/arm/traps.c > > +++ b/xen/arch/arm/traps.c > > @@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs, > > advance_pc(regs, hsr); > > } > > > > +/* Write only + write ignore */ > > [..] > > > +/* Read only + read as zero */ > > I'm not sure if we finished the discussion on those comment on v1 before > you sent the v2. I think we hadn't. > The "+" is very confusing for me because it indicates two parts: write > only and write ignore (same for the read). Both part doesn't really fit > together. Although this helper clearly choose to implement WO as WI > (resp. RO as RAZ). > I think this should be clearer in order to avoid people think this can > be used for RO but with a different value than 0. For v3 I've made the change I proposed in <1429266891.25195.260.ca...@citrix.com> Specifically "Write only as write ignore" and "Read only as read as zero" (essentially s/+/as/) Is that clear enough do you think? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers
Hi Ian, On 17/04/2015 19:01, Ian Campbell wrote: Signed-off-by: Ian Campbell --- v2: Move last paramter of a handle_ro_raz call to next patch where it belongs. --- xen/arch/arm/traps.c | 52 -- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 8b1846a..b54aef6 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs, advance_pc(regs, hsr); } +/* Write only + write ignore */ [..] +/* Read only + read as zero */ I'm not sure if we finished the discussion on those comment on v1 before you sent the v2. The "+" is very confusing for me because it indicates two parts: write only and write ignore (same for the read). Both part doesn't really fit together. Although this helper clearly choose to implement WO as WI (resp. RO as RAZ). I think this should be clearer in order to avoid people think this can be used for RO but with a different value than 0. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers
Signed-off-by: Ian Campbell
---
v2: Move last paramter of a handle_ro_raz call to next patch where it
belongs.
---
xen/arch/arm/traps.c | 52 --
1 file changed, 33 insertions(+), 19 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8b1846a..b54aef6 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
advance_pc(regs, hsr);
}
+/* Write only + write ignore */
+static void handle_wo_wi(struct cpu_user_regs *regs,
+ register_t *reg,
+ bool_t read,
+ const union hsr hsr)
+{
+if ( read )
+return inject_undef_exception(regs, hsr);
+/* else: ignore */
+
+advance_pc(regs, hsr);
+}
+
+/* Read only + read as zero */
+static void handle_ro_raz(struct cpu_user_regs *regs,
+ register_t *reg,
+ bool_t read,
+ const union hsr hsr)
+{
+if ( !read )
+return inject_undef_exception(regs, hsr);
+/* else: raz */
+
+*reg = 0;
+
+advance_pc(regs, hsr);
+}
+
static void do_cp15_32(struct cpu_user_regs *regs,
const union hsr hsr)
{
@@ -1737,11 +1765,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const
union hsr hsr)
* Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
* is set to 0, which we emulated below.
*/
-if ( !cp32.read )
-return inject_undef_exception(regs, hsr);
-
-*r = 0;
-break;
+return handle_ro_raz(regs, r, cp32.read, hsr);
case HSR_CPREG32(DBGDSCREXT):
if ( usr_mode(regs) )
@@ -1768,11 +1792,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const
union hsr hsr)
case HSR_CPREG32(DBGOSLAR):
if ( usr_mode(regs) )
return inject_undef_exception(regs, hsr);
-/* WO */
-if ( cp32.read )
-return inject_undef_exception(regs, hsr);
-/* else: ignore */
-break;
+return handle_wo_wi(regs, r, cp32.read, hsr);
default:
gdprintk(XENLOG_ERR,
"%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1857,11 +1877,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
* Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate
that
* register as RAZ/WI above. So RO at both EL0 and EL1.
*/
-if ( !hsr.sysreg.read )
-return inject_undef_exception(regs, hsr);
-
-*x = 0;
-break;
+return handle_ro_raz(regs, x, hsr.sysreg.read, hsr);
/* - Perf monitors */
case HSR_SYSREG_PMUSERENR_EL0:
@@ -1891,10 +1907,8 @@ static void do_sysreg(struct cpu_user_regs *regs,
/* Write only, Write ignore registers: */
case HSR_SYSREG_OSLAR_EL1:
-if ( hsr.sysreg.read )
-return inject_undef_exception(regs, hsr);
-/* else: write ignored */
-break;
+return handle_wo_wi(regs, x, hsr.sysreg.read, hsr);
+
case HSR_SYSREG_CNTP_CTL_EL0:
case HSR_SYSREG_CNTP_TVAL_EL0:
case HSR_SYSREG_CNTP_CVAL_EL0:
--
1.7.10.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
