[PATCH] xen: arm: setup sane EL1 state while building domain 0.

[PATCH] xen: arm: setup sane EL1 state while building domain 0.

[Ian Campbell]

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.

Reported-by: Fu Wei <fu.wei@linaro.org>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Fu Wei <fu.wei@linaro.org>
---
Fu Wei, can I add your Tested-by here?
---
 xen/arch/arm/domain_build.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 5ca2f15..ea47af5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1022,6 +1022,13 @@ int construct_dom0(struct domain *d)
 
     /* The following loads use the domain's p2m */
     p2m_load_VTTBR(d);
+    /* Various EL2 operations, such as guest address translations used
+     * part of the domain build, rely on EL1 state (i.e. whether the
+     * guest has paging enabled). Since the bootloader may have left
+     * this state in an arbitrary configuration set it to something
+     * safe here.
+     */
+    WRITE_SYSREG32(SCTLR_GUEST_INIT, SCTLR_EL1);
 #ifdef CONFIG_ARM_64
     d->arch.type = kinfo.type;
     if ( is_pv32_domain(d) )
-- 
1.7.10.4

Re: [PATCH] xen: arm: setup sane EL1 state while building domain 0.

[Julien Grall]

Hi Ian,

On 03/17/2014 03:31 PM, Ian Campbell 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.

I was about to send a similar patch :).

>      /* The following loads use the domain's p2m */
>      p2m_load_VTTBR(d);
> +    /* Various EL2 operations, such as guest address translations used
> +     * part of the domain build, rely on EL1 state (i.e. whether the
> +     * guest has paging enabled). Since the bootloader may have left
> +     * this state in an arbitrary configuration set it to something
> +     * safe here.
> +     */
> +    WRITE_SYSREG32(SCTLR_GUEST_INIT, SCTLR_EL1);

I think it would make more sense to create a new function call
p2m_restore_state which contains:

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_pv32_domain(n->domain) )
        hcr &= ~HCR_RW;
    else
        hcr |= HCR_RW;

    WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
    isb();

    WRITE_SYSREG(hcr, HCR_EL2);
    isb();
}

IHMO, it's more clear than continuing "hardcoding" setup in dom0 code.

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: setup sane EL1 state while building domain 0.

[Ian Campbell]

On Mon, 2014-03-17 at 15:37 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/17/2014 03:31 PM, Ian Campbell 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.
> 
> I was about to send a similar patch :).
> 
> >      /* The following loads use the domain's p2m */
> >      p2m_load_VTTBR(d);
> > +    /* Various EL2 operations, such as guest address translations used
> > +     * part of the domain build, rely on EL1 state (i.e. whether the
> > +     * guest has paging enabled). Since the bootloader may have left
> > +     * this state in an arbitrary configuration set it to something
> > +     * safe here.
> > +     */
> > +    WRITE_SYSREG32(SCTLR_GUEST_INIT, SCTLR_EL1);
> 
> I think it would make more sense to create a new function call
> p2m_restore_state which contains:
> 
> void p2m_restore_state(struct vcpu *n)

I think setup_state might be more indicative of the operation.

> {
>     register_t hcr;
> 
>     hcr = READ_SYSREG(HCR_EL2);
>     WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
>     isb();

I know we do this on context switch now but I think it is unnecessary
and I plan to remove it.

> 
>     p2m_load_VTTBR(n->domain);
>     isb();
> 
>     if ( is_pv32_domain(n->domain) )
>         hcr &= ~HCR_RW;
>     else
>         hcr |= HCR_RW;
> 
>     WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
>     isb();
> 
>     WRITE_SYSREG(hcr, HCR_EL2);
>     isb();
> }
> 
> IHMO, it's more clear than continuing "hardcoding" setup in dom0 code.

Are there any other potential callers?

Ian.

Re: [PATCH] xen: arm: setup sane EL1 state while building domain 0.

[Julien Grall]

On 03/17/2014 03:46 PM, Ian Campbell wrote:
> On Mon, 2014-03-17 at 15:37 +0000, Julien Grall wrote:
>> void p2m_restore_state(struct vcpu *n)
> 
> I think setup_state might be more indicative of the operation.
> 
>> {
>>     register_t hcr;
>>
>>     hcr = READ_SYSREG(HCR_EL2);
>>     WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
>>     isb();
> 
> I know we do this on context switch now but I think it is unnecessary
> and I plan to remove it.

Thanks to confirm, I wasn't sure if it was necessary or not.

>> IHMO, it's more clear than continuing "hardcoding" setup in dom0 code.
> 
> Are there any other potential callers?

Yes ctx_to_switch, so everything related to P2M context switch is
restrict to p2m.c. That why I choose this name, there is also a sister
function which will save the p2m context (p2m_save_state).

-- 
Julien Grall

Re: [PATCH] xen: arm: setup sane EL1 state while building domain 0.

[Fu Wei]

On 03/17/2014 11:31 PM, Ian Campbell 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.
> 
> Reported-by: Fu Wei <fu.wei@linaro.org>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Fu Wei <fu.wei@linaro.org>
> ---
> Fu Wei, can I add your Tested-by here?

Yes, Thanks!
I have tested this with my GRUB binary, it works fine.

> ---
>  xen/arch/arm/domain_build.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 5ca2f15..ea47af5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1022,6 +1022,13 @@ int construct_dom0(struct domain *d)
>  
>      /* The following loads use the domain's p2m */
>      p2m_load_VTTBR(d);
> +    /* Various EL2 operations, such as guest address translations used
> +     * part of the domain build, rely on EL1 state (i.e. whether the
> +     * guest has paging enabled). Since the bootloader may have left
> +     * this state in an arbitrary configuration set it to something
> +     * safe here.
> +     */
> +    WRITE_SYSREG32(SCTLR_GUEST_INIT, SCTLR_EL1);
>  #ifdef CONFIG_ARM_64
>      d->arch.type = kinfo.type;
>      if ( is_pv32_domain(d) )
> 


-- 
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 

Re: [PATCH] xen: arm: setup sane EL1 state while building domain 0.

[Ian Campbell]

On Tue, 2014-03-18 at 11:22 +0800, Fu Wei wrote:
> On 03/17/2014 11:31 PM, Ian Campbell 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.
> > 
> > Reported-by: Fu Wei <fu.wei@linaro.org>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Fu Wei <fu.wei@linaro.org>
> > ---
> > Fu Wei, can I add your Tested-by here?
> 
> Yes, Thanks!
> I have tested this with my GRUB binary, it works fine.

Great. Thanks. It looks like we might rework the approach, if so I'll CC
you on that as well (or Julien will).

Are you also going to change grub to leave EL1 in a known good state?

Ian.

Re: [PATCH] xen: arm: setup sane EL1 state while building domain 0.

[Julien Grall]

Hi Ian,

On 03/18/2014 09:41 AM, Ian Campbell wrote:
 > Great. Thanks. It looks like we might rework the approach, if so I'll CC
> you on that as well (or Julien will).
> 
> Are you also going to change grub to leave EL1 in a known good state?

I don't think we should rely on the EL1 state leave by the bootloader
(even if it's valid for the guest).

We may want to change the guest state when a ELF will be supported.

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: setup sane EL1 state while building domain 0.

[Fu Wei]

On 03/18/2014 05:41 PM, Ian Campbell wrote:
> On Tue, 2014-03-18 at 11:22 +0800, Fu Wei wrote:
>> On 03/17/2014 11:31 PM, Ian Campbell 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.
>>>
>>> Reported-by: Fu Wei <fu.wei@linaro.org>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Fu Wei <fu.wei@linaro.org>
>>> ---
>>> Fu Wei, can I add your Tested-by here?
>>
>> Yes, Thanks!
>> I have tested this with my GRUB binary, it works fine.
> 
> Great. Thanks. It looks like we might rework the approach, if so I'll CC
> you on that as well (or Julien will).
> 
> Are you also going to change grub to leave EL1 in a known good state?

I don't think it's the work of grub to modify EL1 register.
if the booting.txt(xen or linux) doesn't mention about EL1, I think the kernel(xen or linux) should take care of these EL1 registers. 
And at the time, xen has booted up, it can modify EL1 registers.

Do you agree?  :-)

> 
> Ian.
> 
> 


-- 
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