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

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   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.

> +                         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 */

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.

> +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, 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).

Regards,


-- 
Julien Grall