[PATCH v3 0/2] xen: arm: Give PTE bits explict values

[PATCH v3 0/2] xen: arm: Give PTE bits explict values

[Chris Brand]

This is more-or-less what Julien requested. He noted that pt.config
was never set to zero. When I looked further, I found other bits that
were also never given a value. Looking at the callsites, they almost
all seem to assume a value of zero, so that's what I went with.

Patch 1 re-orders the assignments to match the declaration, making it
clearer which ones are missing. Patch 2 then adds the missing bits.

Chris

Chris Brand (2):
  xen: arm re-order assignments in mfn_to_xen_entry()
  xen: arm: Be explicit about bit values in mfn_to_xen_entry()

 xen/include/asm-arm/page.h | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] xen: arm re-order assignments in mfn_to_xen_entry()

[Chris Brand]

Shuffle lines around so that the assignments in mfn_to_xen_entry()
occur in the same order as the bits are declared in lpae_pt_t.
This makes it easier to see which ones are never given a value.
No change in behaviour.

Also fix a minor comment typo.

Signed-off-by: Chris Brand <chris.brand@broadcom.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/page.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 5ecfd0705e07..01628f3e96cb 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -197,18 +197,18 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn, unsigned attr)
     paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
     lpae_t e = (lpae_t) {
         .pt = {
-            .xn = 1,              /* No need to execute outside .text */
-            .ng = 1,              /* Makes TLB flushes easier */
-            .af = 1,              /* No need for access tracking */
+            .valid = 1,           /* Mappings are present */
+            .table = 0,           /* Set to 1 for links and 4k maps */
+            .ai = attr,
             .ns = 1,              /* Hyp mode is in the non-secure world */
             .user = 1,            /* See below */
-            .ai = attr,
-            .table = 0,           /* Set to 1 for links and 4k maps */
-            .valid = 1,           /* Mappings are present */
+            .af = 1,              /* No need for access tracking */
+            .ng = 1,              /* Makes TLB flushes easier */
+            .xn = 1,              /* No need to execute outside .text */
         }};;
     /* Setting the User bit is strange, but the ATS1H[RW] instructions
      * don't seem to work otherwise, and since we never run on Xen
-     * pagetables un User mode it's OK.  If this changes, remember
+     * pagetables in User mode it's OK.  If this changes, remember
      * to update the hard-coded values in head.S too */
 
     switch ( attr )
-- 
1.9.1

[PATCH v3 2/2] xen: arm: Be explicit about bit values in mfn_to_xen_entry()

[Chris Brand]

Ensure that every relevant bit is given an explicit value.
This has no effect on the generated code, but makes it
a little easier to follow.

Reported-by: Julien Grall <julien.grall@citrix.com>
Signed-off-by: Chris Brand <chris.brand@broadcom.com>
---
v3 trims down the list of bits given explicit values
v2 adds comments on pxn and avail

 xen/include/asm-arm/page.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 01628f3e96cb..a94e978a9995 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -202,9 +202,12 @@ 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 */
             .af = 1,              /* No need for access tracking */
             .ng = 1,              /* Makes TLB flushes easier */
+            .contig = 0,          /* Assume non-contiguous */
             .xn = 1,              /* No need to execute outside .text */
+            .avail = 0,           /* Reference count for domheap mapping */
         }};;
     /* Setting the User bit is strange, but the ATS1H[RW] instructions
      * don't seem to work otherwise, and since we never run on Xen
-- 
1.9.1

Re: [PATCH v3 2/2] xen: arm: Be explicit about bit values in mfn_to_xen_entry()

[Ian Campbell]

On Thu, 2015-09-10 at 11:56 -0700, Chris Brand wrote:
> Ensure that every relevant bit is given an explicit value.
> This has no effect on the generated code, but makes it
> a little easier to follow.
> 
> Reported-by: Julien Grall <julien.grall@citrix.com>
> Signed-off-by: Chris Brand <chris.brand@broadcom.com>

Acked + applied for 4.7 along with the first one.

I don't think there is any need for either for 4.6, since it's just a code
clarity thing.

> ---
> v3 trims down the list of bits given explicit values
> v2 adds comments on pxn and avail
> 
>  xen/include/asm-arm/page.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 01628f3e96cb..a94e978a9995 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -202,9 +202,12 @@ 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 */
>              .af = 1,              /* No need for access tracking */
>              .ng = 1,              /* Makes TLB flushes easier */
> +            .contig = 0,          /* Assume non-contiguous */
>              .xn = 1,              /* No need to execute outside .text */
> +            .avail = 0,           /* Reference count for domheap mapping
> */
>          }};;
>      /* Setting the User bit is strange, but the ATS1H[RW] instructions
>       * don't seem to work otherwise, and since we never run on Xen