From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers Date: Thu, 16 Apr 2015 17:22:30 +0100 Message-ID: <1429201350.25195.175.camel@citrix.com> References: <1427796446.2115.34.camel@citrix.com> <1427796462-24376-5-git-send-email-ian.campbell@citrix.com> <551E8CD5.3080407@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <551E8CD5.3080407@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, julien.grall@linaro.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Fri, 2015-04-03 at 14:51 +0200, Julien Grall wrote: > Hi Ian, > > On 31/03/2015 12:07, Ian Campbell wrote: > > Signed-off-by: Ian Campbell > > --- > > 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..ebc09f9 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, > > This helper handles WO and WI which doesn't require to modify the register. > > I would pass a register_t rather than register_t* in order to make clear > that the register of the guest won't change. I deliberately made handle_* all have the same prototype for consistency. > > +/* Read only + read as zero */ > > This comment may confuse developer who wants to implement RO register > which another value than 0. > > I got confuse too. It would be nice to expand the comment for the RO case. I'm afraid I don't understand the confusion so I'm not sure how to clarify. What did you think this comment was saying? > > + return handle_ro_raz(regs, r, cp32.read, hsr, 1); > > The function call has too many argument. > > I guess the last argument is the minimum level exception, we should be > part of the next patch (#6). Fixed, thanks. Ian.