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

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

[Chris (Christopher) 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>
---
 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..7a56b2cb463a 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,
             .xn = 1,              /* No need to execute outside .text */
+            .avail = 0,
         }};;
     /* 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] xen: arm: Set all bits in mfn_to_xen_entry()

[Julien Grall]

Hi Chris,

On 14/08/2015 14:42, Chris (Christopher) 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>
> ---
>   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..7a56b2cb463a 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,

I would add a comment to explain that this bit is reserved for PL2 stage 
1 page table.

>               .xn = 1,              /* No need to execute outside .text */
> +            .avail = 0,

I don't think this one is necessary. avail is not used by the hardware 
neither Xen.

>           }};;

what about *t fields (pxnt, xnt, apt,...)?

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

Regards,


-- 
Julien Grall
	

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

[Chris (Christopher) Brand]

Hi Julien,

Thanks for the review.

> > +            .pxn = 0,
> 
> I would add a comment to explain that this bit is reserved for PL2 stage
> 1 page table.

Will do.

> > +            .avail = 0,
> 
> I don't think this one is necessary. avail is not used by the hardware neither
> Xen.

"grep -rF pt.avail xen" gives 7 matches in xen/arch/arm/mm.c, so it is used by
Xen (I'll fully admit that I didn't dig in to the "how and why" of its use).
 
> what about *t fields (pxnt, xnt, apt,...)?

I figured that as we're setting table to 0, these are ignored, and any code setting
table to 1 should then set them. I can obviously easily set them here (I guess all
to zero would make sense) if you think it's worthwhile ...

Chris

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

[Julien Grall]

On 17/08/2015 11:58, Chris (Christopher) Brand wrote:
> Hi Julien,
>
> Thanks for the review.
>
>>> +            .pxn = 0,
>>
>> I would add a comment to explain that this bit is reserved for PL2 stage
>> 1 page table.
>
> Will do.
>
>>> +            .avail = 0,
>>
>> I don't think this one is necessary. avail is not used by the hardware neither
>> Xen.
>
> "grep -rF pt.avail xen" gives 7 matches in xen/arch/arm/mm.c, so it is used by
> Xen (I'll fully admit that I didn't dig in to the "how and why" of its use).

Hmmm right. Sorry I haven't check the Xen code for that.

It's used for domheap mapping to know how many times it has been referenced.

>
>> what about *t fields (pxnt, xnt, apt,...)?
>
> I figured that as we're setting table to 0, these are ignored, and any code setting
> table to 1 should then set them. I can obviously easily set them here (I guess all
> to zero would make sense) if you think it's worthwhile ...

It was just an open question. :) I'm not sure what we should do with them.

Regards,

-- 
Julien Grall