Re: [PATCH 2/2 v2] xen: arm: Set all bits in mfn_to_xen_entry()

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

On Thu, 2015-08-20 at 16:33 -0700, Chris Brand wrote:
> Ensure that every bit has a specific value.
> 
> Reported-by: Julien Grall <julien.grall@citrix.com>
> Signed-off-by: Chris Brand <chris.brand@broadcom.com>
> ---
> v2 adds comments on pxn and avail.
> 
>  xen/include/asm-arm/page.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 01628f3e96cb..4f430a5ff4fa 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -202,9 +202,14 @@ static inline lpae_t mfn_to_xen_entry(unsigned long 
> mfn, unsigned attr)
>              .ai = attr,
>              .ns = 1,              /* Hyp mode is in the non-secure world 
> */
>              .user = 1,            /* See below */
> +            .ro = 0,              /* Assume read-write */

OK
>              .af = 1,              /* No need for access tracking */
>              .ng = 1,              /* Makes TLB flushes easier */
> +            .sbz = 0,

I think this one can/should be omitted, it's basically a padding field and
it will be zero by C specs.

> +            .contig = 0,          /* Assume non-contiguous */

OK

> +            .pxn = 0,             /* Reserved for PL2 stage 1 page table */

Do you mean "Reserved in ..." or maybe "Reserved at ..."?

"Reserved for ..." doesn't make much sense since this function is
constructing a PL2 stage 1 PT and this bit is not reserved for (i.e. "set
aside for") use there.

I think in this context pxn is essentially a padding bit and should be just
omitted as with .sbz.

>              .xn = 1,              /* No need to execute outside .text */
> +            .avail = 0,           /* Reference count for domheap mapping 


OK

> */
>          }};;
>      /* Setting the User bit is strange, but the ATS1H[RW] instructions
>       * don't seem to work otherwise, and since we never run on Xen