| Message ID | 1469031064-23344-20-git-send-email-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
On 22/07/16 09:07, Sergej Proskurin wrote: > Hi Julien, Hello Sergej, > On 07/20/2016 06:11 PM, Julien Grall wrote: >> p2m_restore_state is the last caller of p2m_load_VTTBR and already check >> if the vCPU does not belong to the idle domain. >> >> Note that it is likely possible to remove some isb in the function >> p2m_restore_state, however this is not the purpose of this patch. So the >> numerous isb have been left. >> > > Right now, I don't see any issues with removing the p2m_load_VTTBR > function in combination with changes applied to flush_tlb_domain in your > patch #18 and #17. However, I am not entirely sure whether it makes > sense to entirely remove the function and replicate the VTTBR loading > functionality across multiple functions. Why don't we just provide a > struct p2m_domain* to p2m_load_VTTBR (potentially with a backpointer to > the associated domain, as it is shown in the arm/altp2m patch) and use > the function inline? Because ideally this function should take a p2m in parameter and a p2m cannot belong to an idle domain. So the function would be: WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); isb(); However, in the case of p2m_restore_state the isb() is not necessary and will impact the performance. Yes, I know the function contains a lots of pointless isb(), this needs to be fixed at some point. So overall, this function is not necessary. Regards,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 015c1e8..c756e0c 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -105,19 +105,6 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr) P2M_ROOT_LEVEL, P2M_ROOT_PAGES); } -static void p2m_load_VTTBR(struct domain *d) -{ - struct p2m_domain *p2m = &d->arch.p2m; - - if ( is_idle_domain(d) ) - return; - - ASSERT(p2m->vttbr); - - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); - isb(); /* Ensure update is visible */ -} - void p2m_save_state(struct vcpu *p) { p->arch.sctlr = READ_SYSREG(SCTLR_EL1); @@ -126,6 +113,7 @@ void p2m_save_state(struct vcpu *p) void p2m_restore_state(struct vcpu *n) { register_t hcr; + struct p2m_domain *p2m = &n->domain->arch.p2m; if ( is_idle_vcpu(n) ) return; @@ -134,7 +122,7 @@ void p2m_restore_state(struct vcpu *n) WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2); isb(); - p2m_load_VTTBR(n->domain); + WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); isb(); if ( is_32bit_domain(n->domain) )
p2m_restore_state is the last caller of p2m_load_VTTBR and already check if the vCPU does not belong to the idle domain. Note that it is likely possible to remove some isb in the function p2m_restore_state, however this is not the purpose of this patch. So the numerous isb have been left. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/p2m.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-)