| Message ID | 1395243819-30380-2-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 <julien.grall@linaro.org> > --- > 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 <tim@xen.org> (for the whole series) but it seems like at least the one after the VTTBR write could go? Tim.
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 <julien.grall@linaro.org> >> --- >> 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 <tim@xen.org> (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,
On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote: > > Are all of these isb()s necessary? I guess this is only code motion, > > so in any case, Acked-by: Tim Deegan <tim@xen.org> (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. Actually I think there are probably loads of barriers in the context switch path which can be dropped in favour of a final one at the end, not all that much stuff there relies on stuff which is reloaded before it (of course we should keep barriers for cases where there is a dependence). Ian.
On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote: > 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. It was pretty dubious when the period of the VM bit being cleared was the entire of the restore function (the original idea, since discounted, being to avoid oddities while restoring), but it's really very silly to do it now that it only spans this new function. Ian.
Hi Ian, On 03/21/2014 09:19 AM, Ian Campbell wrote: > On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote: > >>> Are all of these isb()s necessary? I guess this is only code motion, >>> so in any case, Acked-by: Tim Deegan <tim@xen.org> (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. > > Actually I think there are probably loads of barriers in the context > switch path which can be dropped in favour of a final one at the end, > not all that much stuff there relies on stuff which is reloaded before > it (of course we should keep barriers for cases where there is a > dependence). Sorry for the late answer. Shall I rework this patch and remove the duplicated isb and HCR_VM? Regards,
On Fri, 2014-03-28 at 12:44 +0000, Julien Grall wrote: > Hi Ian, > > On 03/21/2014 09:19 AM, Ian Campbell wrote: > > On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote: > > > >>> Are all of these isb()s necessary? I guess this is only code motion, > >>> so in any case, Acked-by: Tim Deegan <tim@xen.org> (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. > > > > Actually I think there are probably loads of barriers in the context > > switch path which can be dropped in favour of a final one at the end, > > not all that much stuff there relies on stuff which is reloaded before > > it (of course we should keep barriers for cases where there is a > > dependence). > > Sorry for the late answer. Shall I rework this patch and remove the > duplicated isb and HCR_VM? If you need to rebase/resend for some other reason then please fold this in. If not then please send a follow up patch. I'm not sure why I didn't commit this series already. I think it must have fallen through the cracks, sorry. Ian.
On 03/28/2014 12:47 PM, Ian Campbell wrote: > On Fri, 2014-03-28 at 12:44 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 03/21/2014 09:19 AM, Ian Campbell wrote: >>> On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote: >>> >>>>> Are all of these isb()s necessary? I guess this is only code motion, >>>>> so in any case, Acked-by: Tim Deegan <tim@xen.org> (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. >>> >>> Actually I think there are probably loads of barriers in the context >>> switch path which can be dropped in favour of a final one at the end, >>> not all that much stuff there relies on stuff which is reloaded before >>> it (of course we should keep barriers for cases where there is a >>> dependence). >> >> Sorry for the late answer. Shall I rework this patch and remove the >> duplicated isb and HCR_VM? > > If you need to rebase/resend for some other reason then please fold this > in. If not then please send a follow up patch. I will send a separate patch to remove unnecessary code in this function.
On Fri, 2014-03-28 at 13:23 +0000, Julien Grall wrote: > On 03/28/2014 12:47 PM, Ian Campbell wrote: > > On Fri, 2014-03-28 at 12:44 +0000, Julien Grall wrote: > >> Hi Ian, > >> > >> On 03/21/2014 09:19 AM, Ian Campbell wrote: > >>> On Thu, 2014-03-20 at 17:59 +0000, Julien Grall wrote: > >>> > >>>>> Are all of these isb()s necessary? I guess this is only code motion, > >>>>> so in any case, Acked-by: Tim Deegan <tim@xen.org> (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. > >>> > >>> Actually I think there are probably loads of barriers in the context > >>> switch path which can be dropped in favour of a final one at the end, > >>> not all that much stuff there relies on stuff which is reloaded before > >>> it (of course we should keep barriers for cases where there is a > >>> dependence). > >> > >> Sorry for the late answer. Shall I rework this patch and remove the > >> duplicated isb and HCR_VM? > > > > If you need to rebase/resend for some other reason then please fold this > > in. If not then please send a follow up patch. > > I will send a separate patch to remove unnecessary code in this function. I've applied this with mine and Tim's acks. Thanks. Ian.
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(); +} + static int p2m_first_level_index(paddr_t addr) { /* diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 3b39c45..e1013c8 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -75,6 +75,10 @@ int p2m_alloc_table(struct domain *d); /* */ void p2m_load_VTTBR(struct domain *d); +/* Context switch */ +void p2m_save_state(struct vcpu *p); +void p2m_restore_state(struct vcpu *n); + /* Look up the MFN corresponding to a domain's PFN. */ paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t);
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 <julien.grall@linaro.org> --- 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(-)