Re: [Xen-devel] [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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 <ian.campbell@xxxxxxxxxx>
> > ---
> >   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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel