[PATCH 0/3] xen/arm: setup a sane EL1 state while building domain 0

[PATCH 0/3] xen/arm: setup a sane EL1 state while building domain 0

[Julien Grall]

Hello all,

This patch series is a rework of Ian's patch "EL1 state while building domain 0)
see http://www.gossamer-threads.com/lists/xen/devel/321128.

Sincerely yours,

Julien Grall (3):
  xen/arm: Move p2m context save/restore in a separate function
  xen/arm: Use p2m_restore_state in construct_dom0
  xen/arm: Don't need to export p2m_load_VTTBR

 xen/arch/arm/domain.c       |   21 +++------------------
 xen/arch/arm/domain_build.c |    9 +--------
 xen/arch/arm/p2m.c          |   30 +++++++++++++++++++++++++++++-
 xen/include/asm-arm/p2m.h   |    5 +++--
 4 files changed, 36 insertions(+), 29 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Julien Grall]

Introduce p2m_{save,restore}_state to save/restore p2m context.

The both functions will take care of:
    - VTTBR: contains the pointer to the domain P2M
    - Update HCR_RW if the domain is 64 bit
    - SCTLR: contains bit to know if the MMU is enabled or not

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain.c     |   21 +++------------------
 xen/arch/arm/p2m.c        |   28 ++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h |    4 ++++
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 46ee486..b125857 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -59,11 +59,12 @@ void idle_loop(void)
 
 static void ctxt_switch_from(struct vcpu *p)
 {
+    p2m_save_state(p);
+
     /* CP 15 */
     p->arch.csselr = READ_SYSREG(CSSELR_EL1);
 
     /* Control Registers */
-    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
     p->arch.cpacr = READ_SYSREG(CPACR_EL1);
 
     p->arch.contextidr = READ_SYSREG(CONTEXTIDR_EL1);
@@ -134,14 +135,7 @@ static void ctxt_switch_from(struct vcpu *p)
 
 static void ctxt_switch_to(struct vcpu *n)
 {
-    register_t hcr;
-
-    hcr = READ_SYSREG(HCR_EL2);
-    WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
-    isb();
-
-    p2m_load_VTTBR(n->domain);
-    isb();
+    p2m_restore_state(n);
 
     WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2);
     WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
@@ -189,7 +183,6 @@ static void ctxt_switch_to(struct vcpu *n)
     isb();
 
     /* Control Registers */
-    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
     WRITE_SYSREG(n->arch.cpacr, CPACR_EL1);
 
     WRITE_SYSREG(n->arch.contextidr, CONTEXTIDR_EL1);
@@ -214,14 +207,6 @@ static void ctxt_switch_to(struct vcpu *n)
 
     isb();
 
-    if ( is_32bit_domain(n->domain) )
-        hcr &= ~HCR_RW;
-    else
-        hcr |= HCR_RW;
-
-    WRITE_SYSREG(hcr, HCR_EL2);
-    isb();
-
     /* This is could trigger an hardware interrupt from the virtual
      * timer. The interrupt needs to be injected into the guest. */
     virt_timer_restore(n);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b9d8ca6..979fe5b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -44,6 +44,34 @@ void p2m_load_VTTBR(struct domain *d)
     isb(); /* Ensure update is visible */
 }
 
+void p2m_save_state(struct vcpu *p)
+{
+    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
+}
+
+void p2m_restore_state(struct vcpu *n)
+{
+    register_t hcr;
+
+    hcr = READ_SYSREG(HCR_EL2);
+    WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
+    isb();
+
+    p2m_load_VTTBR(n->domain);
+    isb();
+
+    if ( is_32bit_domain(n->domain) )
+        hcr &= ~HCR_RW;
+    else
+        hcr |= HCR_RW;
+
+    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
+    isb();
+
+    WRITE_SYSREG(hcr, HCR_EL2);
+    isb();
+}
+
 static int p2m_first_level_index(paddr_t addr)
 {
     /*
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 3b39c45..e1013c8 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -75,6 +75,10 @@ int p2m_alloc_table(struct domain *d);
 /* */
 void p2m_load_VTTBR(struct domain *d);
 
+/* Context switch */
+void p2m_save_state(struct vcpu *p);
+void p2m_restore_state(struct vcpu *n);
+
 /* Look up the MFN corresponding to a domain's PFN. */
 paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t);
 
-- 
1.7.10.4

[PATCH 2/3] xen/arm: Use p2m_restore_state in construct_dom0

[Julien Grall]

The address translation functions used while building dom0 rely on certain EL1
state being configured. In particular they are subject to the behaviour of
SCTLR_EL1.M (stage 1 MMU enabled).

The Xen (and Linux) boot protocol require that the kernel be entered with the
MMU disabled but they don't say anything explicitly about exception levels
other than the one which is active when entering the kernels. Arguably the
protocol could be said to apply to all exception levels but in any case we
should cope with this and setup the EL1 state as necessary.

Fu Wei discovered this when booting Xen from grub.efi over UEFI, it's not
clear whether grub or UEFI is responsible for leaving stage 1 MMU enabled.

Use directly the newly created function p2m_restore_state to retrieve a
correct EL1 state to translate an address.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reported-by: Fu Wei <fu.wei@linaro.org>
---
 xen/arch/arm/domain_build.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d3345bf..9eb9f75 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1021,14 +1021,7 @@ int construct_dom0(struct domain *d)
         return rc;
 
     /* The following loads use the domain's p2m */
-    p2m_load_VTTBR(d);
-#ifdef CONFIG_ARM_64
-    d->arch.type = kinfo.type;
-    if ( is_32bit_domain(d) )
-        WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~HCR_RW, HCR_EL2);
-    else
-        WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_RW, HCR_EL2);
-#endif
+    p2m_restore_state(v);
 
     /*
      * kernel_load will determine the placement of the initrd & fdt in
-- 
1.7.10.4

[PATCH 3/3] xen/arm: Don't need to export p2m_load_VTTBR

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/p2m.c        |    2 +-
 xen/include/asm-arm/p2m.h |    3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 979fe5b..403fd89 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -35,7 +35,7 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
     unmap_domain_page(first);
 }
 
-void p2m_load_VTTBR(struct domain *d)
+static void p2m_load_VTTBR(struct domain *d)
 {
     if ( is_idle_domain(d) )
         return;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index e1013c8..bd71abe 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -72,9 +72,6 @@ int relinquish_p2m_mapping(struct domain *d);
  */
 int p2m_alloc_table(struct domain *d);
 
-/* */
-void p2m_load_VTTBR(struct domain *d);
-
 /* Context switch */
 void p2m_save_state(struct vcpu *p);
 void p2m_restore_state(struct vcpu *n);
-- 
1.7.10.4

Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Tim Deegan]

At 15:43 +0000 on 19 Mar (1395240217), Julien Grall wrote:
> Introduce p2m_{save,restore}_state to save/restore p2m context.
> 
> The both functions will take care of:
>     - VTTBR: contains the pointer to the domain P2M
>     - Update HCR_RW if the domain is 64 bit
>     - SCTLR: contains bit to know if the MMU is enabled or not
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain.c     |   21 +++------------------
>  xen/arch/arm/p2m.c        |   28 ++++++++++++++++++++++++++++
>  xen/include/asm-arm/p2m.h |    4 ++++
>  3 files changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 46ee486..b125857 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -59,11 +59,12 @@ void idle_loop(void)
>  
>  static void ctxt_switch_from(struct vcpu *p)
>  {
> +    p2m_save_state(p);
> +
>      /* CP 15 */
>      p->arch.csselr = READ_SYSREG(CSSELR_EL1);
>  
>      /* Control Registers */
> -    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
>      p->arch.cpacr = READ_SYSREG(CPACR_EL1);
>  
>      p->arch.contextidr = READ_SYSREG(CONTEXTIDR_EL1);
> @@ -134,14 +135,7 @@ static void ctxt_switch_from(struct vcpu *p)
>  
>  static void ctxt_switch_to(struct vcpu *n)
>  {
> -    register_t hcr;
> -
> -    hcr = READ_SYSREG(HCR_EL2);
> -    WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
> -    isb();
> -
> -    p2m_load_VTTBR(n->domain);
> -    isb();
> +    p2m_restore_state(n);
>  
>      WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2);
>      WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
> @@ -189,7 +183,6 @@ static void ctxt_switch_to(struct vcpu *n)
>      isb();
>  
>      /* Control Registers */
> -    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
>      WRITE_SYSREG(n->arch.cpacr, CPACR_EL1);
>  
>      WRITE_SYSREG(n->arch.contextidr, CONTEXTIDR_EL1);
> @@ -214,14 +207,6 @@ static void ctxt_switch_to(struct vcpu *n)
>  
>      isb();
>  
> -    if ( is_32bit_domain(n->domain) )
> -        hcr &= ~HCR_RW;
> -    else
> -        hcr |= HCR_RW;
> -
> -    WRITE_SYSREG(hcr, HCR_EL2);
> -    isb();
> -
>      /* This is could trigger an hardware interrupt from the virtual
>       * timer. The interrupt needs to be injected into the guest. */
>      virt_timer_restore(n);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index b9d8ca6..979fe5b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -44,6 +44,34 @@ void p2m_load_VTTBR(struct domain *d)
>      isb(); /* Ensure update is visible */
>  }
>  
> +void p2m_save_state(struct vcpu *p)
> +{
> +    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
> +}
> +
> +void p2m_restore_state(struct vcpu *n)
> +{
> +    register_t hcr;
> +
> +    hcr = READ_SYSREG(HCR_EL2);
> +    WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
> +    isb();
> +
> +    p2m_load_VTTBR(n->domain);
> +    isb();
> +
> +    if ( is_32bit_domain(n->domain) )
> +        hcr &= ~HCR_RW;
> +    else
> +        hcr |= HCR_RW;
> +
> +    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
> +    isb();
> +
> +    WRITE_SYSREG(hcr, HCR_EL2);
> +    isb();
> +}

Are all of these isb()s necessary?  I guess this is only code motion,
so in any case, Acked-by: Tim Deegan <tim@xen.org> (for the whole series)
but it seems like at least the one after the VTTBR write could go?

Tim.

Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Julien Grall]

Hi Tim,

On 03/20/2014 05:23 PM, Tim Deegan wrote:
> At 15:43 +0000 on 19 Mar (1395240217), Julien Grall wrote:
>> Introduce p2m_{save,restore}_state to save/restore p2m context.
>>
>> The both functions will take care of:
>>     - VTTBR: contains the pointer to the domain P2M
>>     - Update HCR_RW if the domain is 64 bit
>>     - SCTLR: contains bit to know if the MMU is enabled or not
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/domain.c     |   21 +++------------------
>>  xen/arch/arm/p2m.c        |   28 ++++++++++++++++++++++++++++
>>  xen/include/asm-arm/p2m.h |    4 ++++
>>  3 files changed, 35 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 46ee486..b125857 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -59,11 +59,12 @@ void idle_loop(void)
>>  
>>  static void ctxt_switch_from(struct vcpu *p)
>>  {
>> +    p2m_save_state(p);
>> +
>>      /* CP 15 */
>>      p->arch.csselr = READ_SYSREG(CSSELR_EL1);
>>  
>>      /* Control Registers */
>> -    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
>>      p->arch.cpacr = READ_SYSREG(CPACR_EL1);
>>  
>>      p->arch.contextidr = READ_SYSREG(CONTEXTIDR_EL1);
>> @@ -134,14 +135,7 @@ static void ctxt_switch_from(struct vcpu *p)
>>  
>>  static void ctxt_switch_to(struct vcpu *n)
>>  {
>> -    register_t hcr;
>> -
>> -    hcr = READ_SYSREG(HCR_EL2);
>> -    WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
>> -    isb();
>> -
>> -    p2m_load_VTTBR(n->domain);
>> -    isb();
>> +    p2m_restore_state(n);
>>  
>>      WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2);
>>      WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
>> @@ -189,7 +183,6 @@ static void ctxt_switch_to(struct vcpu *n)
>>      isb();
>>  
>>      /* Control Registers */
>> -    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
>>      WRITE_SYSREG(n->arch.cpacr, CPACR_EL1);
>>  
>>      WRITE_SYSREG(n->arch.contextidr, CONTEXTIDR_EL1);
>> @@ -214,14 +207,6 @@ static void ctxt_switch_to(struct vcpu *n)
>>  
>>      isb();
>>  
>> -    if ( is_32bit_domain(n->domain) )
>> -        hcr &= ~HCR_RW;
>> -    else
>> -        hcr |= HCR_RW;
>> -
>> -    WRITE_SYSREG(hcr, HCR_EL2);
>> -    isb();
>> -
>>      /* This is could trigger an hardware interrupt from the virtual
>>       * timer. The interrupt needs to be injected into the guest. */
>>      virt_timer_restore(n);
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index b9d8ca6..979fe5b 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -44,6 +44,34 @@ void p2m_load_VTTBR(struct domain *d)
>>      isb(); /* Ensure update is visible */
>>  }
>>  
>> +void p2m_save_state(struct vcpu *p)
>> +{
>> +    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
>> +}
>> +
>> +void p2m_restore_state(struct vcpu *n)
>> +{
>> +    register_t hcr;
>> +
>> +    hcr = READ_SYSREG(HCR_EL2);
>> +    WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
>> +    isb();
>> +
>> +    p2m_load_VTTBR(n->domain);
>> +    isb();
>> +
>> +    if ( is_32bit_domain(n->domain) )
>> +        hcr &= ~HCR_RW;
>> +    else
>> +        hcr |= HCR_RW;
>> +
>> +    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
>> +    isb();
>> +
>> +    WRITE_SYSREG(hcr, HCR_EL2);
>> +    isb();
>> +}
> 
> Are all of these isb()s necessary?  I guess this is only code motion,
> so in any case, Acked-by: Tim Deegan <tim@xen.org> (for the whole series)
> but it seems like at least the one after the VTTBR write could go?

Thanks for the review.

Yes, the isb() right after VTBBR can be removed.

Ian also pointed that unset HCR_VM bit is not useful. I will write an
incremental patch in the next to clean up the function.

Regards,

-- 
Julien Grall

Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Ian Campbell]

On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote:

> > Are all of these isb()s necessary?  I guess this is only code motion,
> > so in any case, Acked-by: Tim Deegan <tim@xen.org> (for the whole series)
> > but it seems like at least the one after the VTTBR write could go?
> 
> Thanks for the review.
> 
> Yes, the isb() right after VTBBR can be removed.

Actually I think there are probably loads of barriers in the context
switch path which can be dropped in favour of a final one at the end,
not all that much stuff there relies on stuff which is reloaded before
it (of course we should keep barriers for cases where there is a
dependence).

Ian.

Re: [PATCH 2/3] xen/arm: Use p2m_restore_state in construct_dom0

[Ian Campbell]

On Wed, 2014-03-19 at 15:43 +0000, Julien Grall wrote:
> The address translation functions used while building dom0 rely on certain EL1
> state being configured. In particular they are subject to the behaviour of
> SCTLR_EL1.M (stage 1 MMU enabled).
> 
> The Xen (and Linux) boot protocol require that the kernel be entered with the
> MMU disabled but they don't say anything explicitly about exception levels
> other than the one which is active when entering the kernels. Arguably the
> protocol could be said to apply to all exception levels but in any case we
> should cope with this and setup the EL1 state as necessary.
> 
> Fu Wei discovered this when booting Xen from grub.efi over UEFI, it's not
> clear whether grub or UEFI is responsible for leaving stage 1 MMU enabled.
> 
> Use directly the newly created function p2m_restore_state to retrieve a
> correct EL1 state to translate an address.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Reported-by: Fu Wei <fu.wei@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

I think this will leave some initial dom0 vcpu state in the idle vcpu
(my patch had the same issue), but I think that is tolerable. It might
just be worth clearing HCR_VM and perhaps VTTBR (more worried about the
VMID than the base address) when scheduling an idle vcpu.

The alternative is to ump through hoops to build dom0 from an actual
dom0 vcpu, but that sounds fiddly.

Re: [PATCH 3/3] xen/arm: Don't need to export p2m_load_VTTBR

[Ian Campbell]

On Wed, 2014-03-19 at 15:43 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/arch/arm/p2m.c        |    2 +-
>  xen/include/asm-arm/p2m.h |    3 ---
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 979fe5b..403fd89 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -35,7 +35,7 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
>      unmap_domain_page(first);
>  }
>  
> -void p2m_load_VTTBR(struct domain *d)
> +static void p2m_load_VTTBR(struct domain *d)
>  {
>      if ( is_idle_domain(d) )
>          return;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index e1013c8..bd71abe 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -72,9 +72,6 @@ int relinquish_p2m_mapping(struct domain *d);
>   */
>  int p2m_alloc_table(struct domain *d);
>  
> -/* */
> -void p2m_load_VTTBR(struct domain *d);
> -
>  /* Context switch */
>  void p2m_save_state(struct vcpu *p);
>  void p2m_restore_state(struct vcpu *n);

Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Ian Campbell]

On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote:
> Ian also pointed that unset HCR_VM bit is not useful. I will write an
> incremental patch in the next to clean up the function. 

It was pretty dubious when the period of the VM bit being cleared was
the entire of the restore function (the original idea, since discounted,
being to avoid oddities while restoring), but it's really very silly to
do it now that it only spans this new function.

Ian.

Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Julien Grall]

Hi Ian,

On 03/21/2014 09:19 AM, Ian Campbell wrote:
> On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote:
> 
>>> Are all of these isb()s necessary?  I guess this is only code motion,
>>> so in any case, Acked-by: Tim Deegan <tim@xen.org> (for the whole series)
>>> but it seems like at least the one after the VTTBR write could go?
>>
>> Thanks for the review.
>>
>> Yes, the isb() right after VTBBR can be removed.
> 
> Actually I think there are probably loads of barriers in the context
> switch path which can be dropped in favour of a final one at the end,
> not all that much stuff there relies on stuff which is reloaded before
> it (of course we should keep barriers for cases where there is a
> dependence).

Sorry for the late answer. Shall I rework this patch and remove the
duplicated isb and HCR_VM?

Regards,

-- 
Julien Grall

Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Ian Campbell]

On Fri, 2014-03-28 at 12:44 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/21/2014 09:19 AM, Ian Campbell wrote:
> > On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote:
> > 
> >>> Are all of these isb()s necessary?  I guess this is only code motion,
> >>> so in any case, Acked-by: Tim Deegan <tim@xen.org> (for the whole series)
> >>> but it seems like at least the one after the VTTBR write could go?
> >>
> >> Thanks for the review.
> >>
> >> Yes, the isb() right after VTBBR can be removed.
> > 
> > Actually I think there are probably loads of barriers in the context
> > switch path which can be dropped in favour of a final one at the end,
> > not all that much stuff there relies on stuff which is reloaded before
> > it (of course we should keep barriers for cases where there is a
> > dependence).
> 
> Sorry for the late answer. Shall I rework this patch and remove the
> duplicated isb and HCR_VM?

If you need to rebase/resend for some other reason then please fold this
in. If not then please send a follow up patch.

I'm not sure why I didn't commit this series already. I think it must
have fallen through the cracks, sorry.

Ian.

Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Julien Grall]

On 03/28/2014 12:47 PM, Ian Campbell wrote:
> On Fri, 2014-03-28 at 12:44 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 03/21/2014 09:19 AM, Ian Campbell wrote:
>>> On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote:
>>>
>>>>> Are all of these isb()s necessary?  I guess this is only code motion,
>>>>> so in any case, Acked-by: Tim Deegan <tim@xen.org> (for the whole series)
>>>>> but it seems like at least the one after the VTTBR write could go?
>>>>
>>>> Thanks for the review.
>>>>
>>>> Yes, the isb() right after VTBBR can be removed.
>>>
>>> Actually I think there are probably loads of barriers in the context
>>> switch path which can be dropped in favour of a final one at the end,
>>> not all that much stuff there relies on stuff which is reloaded before
>>> it (of course we should keep barriers for cases where there is a
>>> dependence).
>>
>> Sorry for the late answer. Shall I rework this patch and remove the
>> duplicated isb and HCR_VM?
> 
> If you need to rebase/resend for some other reason then please fold this
> in. If not then please send a follow up patch.

I will send a separate patch to remove unnecessary code in this function.

-- 
Julien Grall

Re: [PATCH 2/3] xen/arm: Use p2m_restore_state in construct_dom0

[Julien Grall]

On 03/21/2014 04:50 PM, Ian Campbell wrote:
> On Wed, 2014-03-19 at 15:43 +0000, Julien Grall wrote:
>> The address translation functions used while building dom0 rely on certain EL1
>> state being configured. In particular they are subject to the behaviour of
>> SCTLR_EL1.M (stage 1 MMU enabled).
>>
>> The Xen (and Linux) boot protocol require that the kernel be entered with the
>> MMU disabled but they don't say anything explicitly about exception levels
>> other than the one which is active when entering the kernels. Arguably the
>> protocol could be said to apply to all exception levels but in any case we
>> should cope with this and setup the EL1 state as necessary.
>>
>> Fu Wei discovered this when booting Xen from grub.efi over UEFI, it's not
>> clear whether grub or UEFI is responsible for leaving stage 1 MMU enabled.
>>
>> Use directly the newly created function p2m_restore_state to retrieve a
>> correct EL1 state to translate an address.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Reported-by: Fu Wei <fu.wei@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I think this will leave some initial dom0 vcpu state in the idle vcpu
> (my patch had the same issue), but I think that is tolerable. It might
> just be worth clearing HCR_VM and perhaps VTTBR (more worried about the
> VMID than the base address) when scheduling an idle vcpu.

I think it's already the case when idle VPCU are scheduled. We don't
change the VTTBR so it keeps the one used by the previous running VCPU.

-- 
Julien Grall

Re: [PATCH 2/3] xen/arm: Use p2m_restore_state in construct_dom0

[Ian Campbell]

On Fri, 2014-03-28 at 13:26 +0000, Julien Grall wrote:
> On 03/21/2014 04:50 PM, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 15:43 +0000, Julien Grall wrote:
> >> The address translation functions used while building dom0 rely on certain EL1
> >> state being configured. In particular they are subject to the behaviour of
> >> SCTLR_EL1.M (stage 1 MMU enabled).
> >>
> >> The Xen (and Linux) boot protocol require that the kernel be entered with the
> >> MMU disabled but they don't say anything explicitly about exception levels
> >> other than the one which is active when entering the kernels. Arguably the
> >> protocol could be said to apply to all exception levels but in any case we
> >> should cope with this and setup the EL1 state as necessary.
> >>
> >> Fu Wei discovered this when booting Xen from grub.efi over UEFI, it's not
> >> clear whether grub or UEFI is responsible for leaving stage 1 MMU enabled.
> >>
> >> Use directly the newly created function p2m_restore_state to retrieve a
> >> correct EL1 state to translate an address.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> Reported-by: Fu Wei <fu.wei@linaro.org>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > I think this will leave some initial dom0 vcpu state in the idle vcpu
> > (my patch had the same issue), but I think that is tolerable. It might
> > just be worth clearing HCR_VM and perhaps VTTBR (more worried about the
> > VMID than the base address) when scheduling an idle vcpu.
> 
> I think it's already the case when idle VPCU are scheduled. We don't
> change the VTTBR so it keeps the one used by the previous running VCPU.

I expect you are correct.

I don't think we currently rely anywhere on a domain's VMID not being
present in a VTTBR if none of its VCPUs are scheduled or some other case
like that. I'm not sure how likely that scenario is. It's the sort of
thing which might come up if someone was trying to do clever TLB or
cache flush elision. Lets not worry about it now...

Ian.

Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function

[Ian Campbell]

On Fri, 2014-03-28 at 13:23 +0000, Julien Grall wrote:
> On 03/28/2014 12:47 PM, Ian Campbell wrote:
> > On Fri, 2014-03-28 at 12:44 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 03/21/2014 09:19 AM, Ian Campbell wrote:
> >>> On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote:
> >>>
> >>>>> Are all of these isb()s necessary?  I guess this is only code motion,
> >>>>> so in any case, Acked-by: Tim Deegan <tim@xen.org> (for the whole series)
> >>>>> but it seems like at least the one after the VTTBR write could go?
> >>>>
> >>>> Thanks for the review.
> >>>>
> >>>> Yes, the isb() right after VTBBR can be removed.
> >>>
> >>> Actually I think there are probably loads of barriers in the context
> >>> switch path which can be dropped in favour of a final one at the end,
> >>> not all that much stuff there relies on stuff which is reloaded before
> >>> it (of course we should keep barriers for cases where there is a
> >>> dependence).
> >>
> >> Sorry for the late answer. Shall I rework this patch and remove the
> >> duplicated isb and HCR_VM?
> > 
> > If you need to rebase/resend for some other reason then please fold this
> > in. If not then please send a follow up patch.
> 
> I will send a separate patch to remove unnecessary code in this function.

I've applied this with mine and Tim's acks.

Thanks.

Ian.

Re: [PATCH 0/3] xen/arm: setup a sane EL1 state while building domain 0

[Ian Campbell]

Fu Wei,

I've just applied this series from Julien which should eliminate the
need for the SCTLR_EL1 workaround with the grub multiboot stuff.

Ian.

On Wed, 2014-03-19 at 15:43 +0000, Julien Grall wrote:
> Hello all,
> 
> This patch series is a rework of Ian's patch "EL1 state while building domain 0)
> see http://www.gossamer-threads.com/lists/xen/devel/321128.
> 
> Sincerely yours,
> 
> Julien Grall (3):
>   xen/arm: Move p2m context save/restore in a separate function
>   xen/arm: Use p2m_restore_state in construct_dom0
>   xen/arm: Don't need to export p2m_load_VTTBR
> 
>  xen/arch/arm/domain.c       |   21 +++------------------
>  xen/arch/arm/domain_build.c |    9 +--------
>  xen/arch/arm/p2m.c          |   30 +++++++++++++++++++++++++++++-
>  xen/include/asm-arm/p2m.h   |    5 +++--
>  4 files changed, 36 insertions(+), 29 deletions(-)
> 

Re: [PATCH 0/3] xen/arm: setup a sane EL1 state while building domain 0

[Fu Wei]

Hi Ian,
Great thanks for your info!

I will test it and update the wiki page ASAP. :-)


On 04/01/2014 06:53 PM, Ian Campbell wrote:
> Fu Wei,
> 
> I've just applied this series from Julien which should eliminate the
> need for the SCTLR_EL1 workaround with the grub multiboot stuff.
> 
> Ian.
> 
> On Wed, 2014-03-19 at 15:43 +0000, Julien Grall wrote:
>> Hello all,
>>
>> This patch series is a rework of Ian's patch "EL1 state while building domain 0)
>> see http://www.gossamer-threads.com/lists/xen/devel/321128.
>>
>> Sincerely yours,
>>
>> Julien Grall (3):
>>   xen/arm: Move p2m context save/restore in a separate function
>>   xen/arm: Use p2m_restore_state in construct_dom0
>>   xen/arm: Don't need to export p2m_load_VTTBR
>>
>>  xen/arch/arm/domain.c       |   21 +++------------------
>>  xen/arch/arm/domain_build.c |    9 +--------
>>  xen/arch/arm/p2m.c          |   30 +++++++++++++++++++++++++++++-
>>  xen/include/asm-arm/p2m.h   |    5 +++--
>>  4 files changed, 36 insertions(+), 29 deletions(-)
>>
> 
> 


-- 
Best regards,

Fu Wei
LAVA Engineer From Red Hat
LAVA Team
Linaro.org | Open source software for ARM SoCs
Ph: +86 186 2020 4684 (mobile)
IRC: fuwei
Skype: tekkamanninja
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021