| Message ID | 1395070269-32356-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
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,
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.
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).
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) ) >
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.
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,
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. > >
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) )
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(+)