Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-03-12 at 16:13 +0100, Tamas K Lengyel wrote:

>         and this function is nearly identical to the x86 one too.

> Nearly, but not completely. IMHO consolidation may be possible on some
> of these bits, but I'm not sure if it would make it any easier to
> follow when the code jumps back and forth between common and arch
> specific parts.

It's worse to have two similar bits of code which are supposed to do the
same thing but can get out of sync or behave subtly different.

Anyway, I'm happy for consolidation to come later.

>         
>         
>         Would be nice not to have this static const thing twice
>         in .rodata.
> 
> 
> What do you mean twice? One is converting from p2m_access to
> XENMEM_access, the other is XENMEM_access to p2m_access.

My mistake, they were similar but not similar enough.

>         
>         > +#define ACCESS(ac) [p2m_access_##ac] = XENMEM_access_##ac
>         > +            ACCESS(n),
>         > +            ACCESS(r),
>         > +            ACCESS(w),
>         > +            ACCESS(rw),
>         > +            ACCESS(x),
>         > +            ACCESS(rx),
>         > +            ACCESS(wx),
>         > +            ACCESS(rwx),
>         > +            ACCESS(rx2rw),
>         > +            ACCESS(n2rwx),
>         > +#undef ACCESS
>         > +    };
>         > +
>         > +    /* If no setting was ever set, just return rwx. */
>         > +    if ( !p2m->mem_access_enabled )
>         > +    {
>         > +        *access = XENMEM_access_rwx;
>         > +        return 0;
>         > +    }
>         > +
>         > +    /* If request to get default access */
>         > +    if ( gpfn == ~0ull )
>         
>         We should have a suitable constant for this, I think,
>         INVALID_MFN looks
>         like the one.
> 
> 
> ~0ull is specifically used by the mem_access API for this purpose. If
> anywhere, in the cleanup series it might make sense to have a #define
> added for it.

OK, can be done later then.

Ian.