Re: [PATCH v4 17/19] arm64: KVM: Dynamically compute the HYP VA mask

2018-02-15 Thread Marc Zyngier 
On 18/01/18 20:28, Christoffer Dall wrote:
> On Thu, Jan 04, 2018 at 06:43:32PM +, Marc Zyngier wrote:
>> As we're moving towards a much more dynamic way to compute our
>> HYP VA, let's express the mask in a slightly different way.
>>
>> Instead of comparing the idmap position to the "low" VA mask,
>> we directly compute the mask by taking into account the idmap's
>> (VA_BIT-1) bit.
>>
>> No functionnal change.
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>>  arch/arm64/kvm/va_layout.c | 17 ++---
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
>> index aee758574e61..75bb1c6772b0 100644
>> --- a/arch/arm64/kvm/va_layout.c
>> +++ b/arch/arm64/kvm/va_layout.c
>> @@ -21,24 +21,19 @@
>>  #include 
>>  #include 
>>  
>> -#define HYP_PAGE_OFFSET_HIGH_MASK   ((UL(1) << VA_BITS) - 1)
>> -#define HYP_PAGE_OFFSET_LOW_MASK((UL(1) << (VA_BITS - 1)) - 1)
>> -
>>  static u64 va_mask;
>>  
>>  static void compute_layout(void)
>>  {
>>  phys_addr_t idmap_addr = __pa_symbol(__hyp_idmap_text_start);
>> -unsigned long mask = HYP_PAGE_OFFSET_HIGH_MASK;
>> +u64 region;
> 
> the naming here really confused me.  Would it make sense to call this
> 'hyp_va_msb' or something like that instead?
> 
>>  
>> -/*
>> - * Activate the lower HYP offset only if the idmap doesn't
>> - * clash with it,
>> - */
>> -if (idmap_addr > HYP_PAGE_OFFSET_LOW_MASK)
>> -mask = HYP_PAGE_OFFSET_HIGH_MASK;
> 
> Ah, the series was tested, it was just that this code only existed for a
> short while.  Amusingly, I think this ephemeral bug goes against the "No
> function change" statement in the commit message.
> 
>> +/* Where is my RAM region? */
>> +region  = idmap_addr & BIT(VA_BITS - 1);
>> +region ^= BIT(VA_BITS - 1);
>>  
>> -va_mask = mask;
>> +va_mask  = BIT(VA_BITS - 1) - 1;
> 
> nit: This could also be written as GENMASK_ULL(VA_BITS - 2, 0) --- and
> now I'm not sure which one I prefer.

Good point. I think GENMASK makes it clearer what the intent is, and
assigning a mask to a mask has certain degree of consistency (/me fondly
remembers dimensional analysis...).

> 
>> +va_mask |= region;
>>  }
>>  
>>  static u32 compute_instruction(int n, u32 rd, u32 rn)
>> -- 
>> 2.14.2
>>
> Otherwise looks good:
> 
> Reviewed-by: Christoffer Dall 
> 

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 17/19] arm64: KVM: Dynamically compute the HYP VA mask

2018-01-18 Thread Christoffer Dall 
On Thu, Jan 04, 2018 at 06:43:32PM +, Marc Zyngier wrote:
> As we're moving towards a much more dynamic way to compute our
> HYP VA, let's express the mask in a slightly different way.
> 
> Instead of comparing the idmap position to the "low" VA mask,
> we directly compute the mask by taking into account the idmap's
> (VA_BIT-1) bit.
> 
> No functionnal change.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  arch/arm64/kvm/va_layout.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
> index aee758574e61..75bb1c6772b0 100644
> --- a/arch/arm64/kvm/va_layout.c
> +++ b/arch/arm64/kvm/va_layout.c
> @@ -21,24 +21,19 @@
>  #include 
>  #include 
>  
> -#define HYP_PAGE_OFFSET_HIGH_MASK((UL(1) << VA_BITS) - 1)
> -#define HYP_PAGE_OFFSET_LOW_MASK ((UL(1) << (VA_BITS - 1)) - 1)
> -
>  static u64 va_mask;
>  
>  static void compute_layout(void)
>  {
>   phys_addr_t idmap_addr = __pa_symbol(__hyp_idmap_text_start);
> - unsigned long mask = HYP_PAGE_OFFSET_HIGH_MASK;
> + u64 region;

the naming here really confused me.  Would it make sense to call this
'hyp_va_msb' or something like that instead?

>  
> - /*
> -  * Activate the lower HYP offset only if the idmap doesn't
> -  * clash with it,
> -  */
> - if (idmap_addr > HYP_PAGE_OFFSET_LOW_MASK)
> - mask = HYP_PAGE_OFFSET_HIGH_MASK;

Ah, the series was tested, it was just that this code only existed for a
short while.  Amusingly, I think this ephemeral bug goes against the "No
function change" statement in the commit message.

> + /* Where is my RAM region? */
> + region  = idmap_addr & BIT(VA_BITS - 1);
> + region ^= BIT(VA_BITS - 1);
>  
> - va_mask = mask;
> + va_mask  = BIT(VA_BITS - 1) - 1;

nit: This could also be written as GENMASK_ULL(VA_BITS - 2, 0) --- and
now I'm not sure which one I prefer.

> + va_mask |= region;
>  }
>  
>  static u32 compute_instruction(int n, u32 rd, u32 rn)
> -- 
> 2.14.2
> 
Otherwise looks good:

Reviewed-by: Christoffer Dall 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 17/19] arm64: KVM: Dynamically compute the HYP VA mask

2018-01-04 Thread Marc Zyngier 
As we're moving towards a much more dynamic way to compute our
HYP VA, let's express the mask in a slightly different way.

Instead of comparing the idmap position to the "low" VA mask,
we directly compute the mask by taking into account the idmap's
(VA_BIT-1) bit.

No functionnal change.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/va_layout.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
index aee758574e61..75bb1c6772b0 100644
--- a/arch/arm64/kvm/va_layout.c
+++ b/arch/arm64/kvm/va_layout.c
@@ -21,24 +21,19 @@
 #include 
 #include 
 
-#define HYP_PAGE_OFFSET_HIGH_MASK  ((UL(1) << VA_BITS) - 1)
-#define HYP_PAGE_OFFSET_LOW_MASK   ((UL(1) << (VA_BITS - 1)) - 1)
-
 static u64 va_mask;
 
 static void compute_layout(void)
 {
phys_addr_t idmap_addr = __pa_symbol(__hyp_idmap_text_start);
-   unsigned long mask = HYP_PAGE_OFFSET_HIGH_MASK;
+   u64 region;
 
-   /*
-* Activate the lower HYP offset only if the idmap doesn't
-* clash with it,
-*/
-   if (idmap_addr > HYP_PAGE_OFFSET_LOW_MASK)
-   mask = HYP_PAGE_OFFSET_HIGH_MASK;
+   /* Where is my RAM region? */
+   region  = idmap_addr & BIT(VA_BITS - 1);
+   region ^= BIT(VA_BITS - 1);
 
-   va_mask = mask;
+   va_mask  = BIT(VA_BITS - 1) - 1;
+   va_mask |= region;
 }
 
 static u32 compute_instruction(int n, u32 rd, u32 rn)
-- 
2.14.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm