[Xen-devel] xen: arm: setup sane EL1 state while building domain 0.

Message ID 1395070269-32356-1-git-send-email-ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell March 17, 2014, 3:31 p.m.
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(+)

Comments

Julien Grall March 17, 2014, 3:37 p.m. | #1
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,
Ian Campbell March 17, 2014, 3:46 p.m. | #2
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.
Julien Grall March 17, 2014, 3:50 p.m. | #3
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).
Fu Wei Fu March 18, 2014, 3:22 a.m. | #4
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) )
>
Ian Campbell March 18, 2014, 9:41 a.m. | #5
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.
Julien Grall March 18, 2014, 12:16 p.m. | #6
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,
Fu Wei Fu March 18, 2014, 12:37 p.m. | #7
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.
> 
>

Patch

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