[PATCH 0/2 v2] xen: arm: Ensure all PTE bits have a known value

[PATCH 0/2 v2] xen: arm: Ensure all PTE bits have a known value

[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: Set all bits in mfn_to_xen_entry()

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

-- 
1.9.1

[PATCH 1/2 v2] 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>
---
 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

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

[Ian Campbell]

On Thu, 2015-08-20 at 16:33 -0700, Chris Brand wrote:
> 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>

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

[Chris Brand]

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 */
             .af = 1,              /* No need for access tracking */
             .ng = 1,              /* Makes TLB flushes easier */
+            .sbz = 0,
+            .contig = 0,          /* Assume non-contiguous */
+            .pxn = 0,             /* Reserved for PL2 stage 1 page table */
             .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 2/2 v2] xen: arm: Set all bits in mfn_to_xen_entry()

[Andrew Cooper]

On 21/08/15 00:33, 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.

This is no functional change, if the compiler is conforming to the C spec.

The spec guarantees that structure initialisation like this causes
unspecified names to gain their default value.  As these are integer
bitfields, the default value is 0.

What compiler is in use?  It would appear that it is buggy, or at least
has buggy scalar replacement optimisations.

~Andrew

>
>  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 */
>              .af = 1,              /* No need for access tracking */
>              .ng = 1,              /* Makes TLB flushes easier */
> +            .sbz = 0,
> +            .contig = 0,          /* Assume non-contiguous */
> +            .pxn = 0,             /* Reserved for PL2 stage 1 page table */
>              .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

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

[Chris (Christopher) Brand]

Hi Andrew,

> On 21/08/15 00:33, 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.
> 
> This is no functional change, if the compiler is conforming to the C spec.
> 
> The spec guarantees that structure initialisation like this causes unspecified
> names to gain their default value.  As these are integer bitfields, the default
> value is 0.
> 
> What compiler is in use?  It would appear that it is buggy, or at least has
> buggy scalar replacement optimisations.

That's right. I'd forgotten about that. This was actually suggested by Julien in
a review of another patch I sent. I haven't seen any problems this fixes.

Chris

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

[Julien Grall]

On 21/08/2015 12:47, Chris (Christopher) Brand wrote:
> Hi Andrew,
>
>> On 21/08/15 00:33, 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.
>>
>> This is no functional change, if the compiler is conforming to the C spec.
>>
>> The spec guarantees that structure initialisation like this causes unspecified
>> names to gain their default value.  As these are integer bitfields, the default
>> value is 0.
>>
>> What compiler is in use?  It would appear that it is buggy, or at least has
>> buggy scalar replacement optimisations.
>
> That's right. I'd forgotten about that. This was actually suggested by Julien in
> a review of another patch I sent. I haven't seen any problems this fixes.

I still think those patches are valid in order to know which value are 
set by default and what does it mean.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

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