[Xen-devel,for-4.12,5/8] xen/arm: p2m: Only use isb() when it is necessary

The EL1 translation regime is out-of-context when running at EL2. This
means the processor cannot speculate memory accesses using the registers
associated to that regime.

An isb() is only need if Xen is going to use the translation regime
before returning to the guest (exception returns will synchronized the
context).

Remove unecessary isb() and document the ones left.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/p2m.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

On 28.11.18 18:49, Julien Grall wrote:
> The EL1 translation regime is out-of-context when running at EL2. This
> means the processor cannot speculate memory accesses using the registers
> associated to that regime.
> 
> An isb() is only need if Xen is going to use the translation regime
> before returning to the guest (exception returns will synchronized the
> context).
> 
> Remove unecessary isb() and document the ones left.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>


Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

On 28.11.18 18:49, Julien Grall wrote:

>       if ( *last_vcpu_ran != INVALID_VCPU_ID && *last_vcpu_ran != n->vcpu_id )
> +    {
>           flush_tlb_local();
> +    }
BTW, missed mentioning that curly braces above are odd by coding style.

On 21/12/2018 14:43, Andrii Anisov wrote:
> 
> 
> On 28.11.18 18:49, Julien Grall wrote:
> 
>>       if ( *last_vcpu_ran != INVALID_VCPU_ID && *last_vcpu_ran != n->vcpu_id )
>> +    {
>>           flush_tlb_local();
>> +    }
> BTW, missed mentioning that curly braces above are odd by coding style.

It is not odd, just unnecessary. I will drop them.

Cheers,

On Wed, 28 Nov 2018, Julien Grall wrote:
> The EL1 translation regime is out-of-context when running at EL2. This
> means the processor cannot speculate memory accesses using the registers
> associated to that regime.
> 
> An isb() is only need if Xen is going to use the translation regime
                   ^ needed


> before returning to the guest (exception returns will synchronized the
                                                        ^ synchronize

> context).
> 
> Remove unecessary isb() and document the ones left.
         ^ unnecessary



> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/p2m.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index e8bacab9d2..844833c4c3 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -112,22 +112,28 @@ void p2m_restore_state(struct vcpu *n)
>          return;
>  
>      WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
> -    isb();
> -
>      WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
> -    isb();
> -
>      WRITE_SYSREG(n->arch.hcr_el2, HCR_EL2);
> -    isb();
>  
>      last_vcpu_ran = &p2m->last_vcpu_ran[smp_processor_id()];
>  
>      /*
> +     * While we are restoring an out-of-context translation regime
> +     * we still need to ensure:
> +     *  - VTTBR_EL2 is synchronized before flushing the TLBs
> +     *  - All registers for EL1 are synchronized before executing an AT
> +     *  instructions targeting S1/S2.
> +     */
> +    isb();

This explanation makes sense to me


> +    /*
>       * Flush local TLB for the domain to prevent wrong TLB translation
>       * when running multiple vCPU of the same domain on a single pCPU.
>       */
>      if ( *last_vcpu_ran != INVALID_VCPU_ID && *last_vcpu_ran != n->vcpu_id )
> +    {
>          flush_tlb_local();
> +    }

Spurious change?


>      *last_vcpu_ran = n->vcpu_id;
>  }
> @@ -153,6 +159,7 @@ static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
>      {
>          local_irq_save(flags);
>          WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
> +        /* Ensure VTTBR_EL2 is synchronized before flushing the TLBs */
>          isb();
>      }
>  
> @@ -161,6 +168,7 @@ static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
>      if ( ovttbr != READ_SYSREG64(VTTBR_EL2) )
>      {
>          WRITE_SYSREG64(ovttbr, VTTBR_EL2);
> +        /* Ensure VTTBR_EL2 is back in place before continuing. */
>          isb();
>          local_irq_restore(flags);
>      }
> @@ -1496,7 +1504,6 @@ static uint32_t __read_mostly vtcr;
>  static void setup_virt_paging_one(void *data)
>  {
>      WRITE_SYSREG32(vtcr, VTCR_EL2);
> -    isb();
>  }
>  
>  void __init setup_virt_paging(void)
> -- 
> 2.11.0
>

Hi Stefano,

On 23/01/2019 23:42, Stefano Stabellini wrote:
> On Wed, 28 Nov 2018, Julien Grall wrote:
>> +    /*
>>        * Flush local TLB for the domain to prevent wrong TLB translation
>>        * when running multiple vCPU of the same domain on a single pCPU.
>>        */
>>       if ( *last_vcpu_ran != INVALID_VCPU_ID && *last_vcpu_ran != n->vcpu_id )
>> +    {
>>           flush_tlb_local();
>> +    }
> 
> Spurious change?

Yes, this was a left-over from a previous try. I will drop them.

Cheers,