From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 1/3] xen/arm: Move p2m context save/restore in a separate function Date: Thu, 20 Mar 2014 17:59:48 +0000 Message-ID: <532B2C94.5060400@linaro.org> References: <1395243819-30380-1-git-send-email-julien.grall@linaro.org> <1395243819-30380-2-git-send-email-julien.grall@linaro.org> <20140320172338.GG68664@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WQhFx-0006ze-I8 for xen-devel@lists.xenproject.org; Thu, 20 Mar 2014 17:59:53 +0000 Received: by mail-we0-f182.google.com with SMTP id p61so875315wes.27 for ; Thu, 20 Mar 2014 10:59:51 -0700 (PDT) In-Reply-To: <20140320172338.GG68664@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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 (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