diff mbox

[Xen-devel,2/3] xen/arm: Use p2m_restore_state in construct_dom0

Message ID 1395243819-30380-3-git-send-email-julien.grall@linaro.org
State Accepted, archived
Headers show

Commit Message

Julien Grall March 19, 2014, 3:43 p.m. UTC
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(-)

Comments

Ian Campbell March 21, 2014, 4:50 p.m. UTC | #1
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.
Julien Grall March 28, 2014, 1:26 p.m. UTC | #2
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.
Ian Campbell March 28, 2014, 1:51 p.m. UTC | #3
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.
diff mbox

Patch

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