| Message ID | 1469717505-8026-14-git-send-email-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
Hello Tamas, On 28/07/2016 18:29, Tamas K Lengyel wrote: > On Thu, Jul 28, 2016 at 8:51 AM, Julien Grall <julien.grall@arm.com> wrote: [...] >> --- >> xen/arch/arm/p2m.c | 18 ++++-------------- >> 1 file changed, 4 insertions(+), 14 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 8676b9d..9a9c85c 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -398,24 +398,13 @@ out: >> return mfn; >> } >> >> -/* >> - * Lookup the MFN corresponding to a domain's GFN. >> - * >> - * There are no processor functions to do a stage 2 only lookup therefore we >> - * do a a software walk. >> - */ >> -static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) >> -{ >> - return p2m_get_entry(&d->arch.p2m, gfn, t, NULL, NULL); >> -} >> - >> mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) >> { >> mfn_t ret; >> struct p2m_domain *p2m = &d->arch.p2m; >> >> p2m_read_lock(p2m); >> - ret = __p2m_lookup(d, gfn, t); >> + ret = p2m_get_entry(p2m, gfn, t, NULL, NULL); >> p2m_read_unlock(p2m); >> >> return ret; >> @@ -679,7 +668,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn, >> * No setting was found in the Radix tree. Check if the >> * entry exists in the page-tables. >> */ >> - mfn_t mfn = __p2m_lookup(d, gfn, NULL); >> + mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL); >> >> if ( mfn_eq(mfn, INVALID_MFN) ) >> return -ESRCH; >> @@ -1595,6 +1584,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) >> xenmem_access_t xma; >> p2m_type_t t; >> struct page_info *page = NULL; >> + struct p2m_domain *p2m = ¤t->domain->arch.p2m; > > I think this would be a good time to change this and pass p2m as an > input to p2m_mem_access_check_and_get_page. This would help with our > altp2m series as well. This function can only work with the current p2m because of the call to gva_to_ipa. So I don't think it is a good idea to pass the p2m in parameter. If you want to pass the p2m in parameter, you have to context switch properly all the registers (i.e VTTBR_EL2, TTBR{0,1}_EL1 and SCTLR_EL1). Actually, this function is buggy if memaccess has changed the permission on memory holding the stage-1 page-table. Because we are using the hardware to translate the VA -> PA, the translate may fail due to memaccess. Regards,
On 28/07/16 18:36, Tamas K Lengyel wrote: > On Thu, Jul 28, 2016 at 11:29 AM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >> On Thu, Jul 28, 2016 at 8:51 AM, Julien Grall <julien.grall@arm.com> wrote: >>> __p2m_lookup is just a wrapper to p2m_get_entry. >>> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >>> Cc: Tamas K Lengyel <tamas@tklengyel.com> >>> >>> --- >>> It might be possible to rework the memaccess code to take advantage >>> of all the parameters. I will defer this to the memaccess folks. >> >> Could you elaborate on what you mean? >> > > Never mind, I see it. Yes, doing __p2m_get_mem_access and then > p2m_get_entry later duplicates work. I would suggest just replacing > __p2m_get_mem_access with a single call to p2m_get_entry to get both > the type and the mem_access setting on the page in a single run. I am not planning to clean-up the memaccess code. Feel free to send a follow-up patch for that. Regards,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 8676b9d..9a9c85c 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -398,24 +398,13 @@ out: return mfn; } -/* - * Lookup the MFN corresponding to a domain's GFN. - * - * There are no processor functions to do a stage 2 only lookup therefore we - * do a a software walk. - */ -static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) -{ - return p2m_get_entry(&d->arch.p2m, gfn, t, NULL, NULL); -} - mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) { mfn_t ret; struct p2m_domain *p2m = &d->arch.p2m; p2m_read_lock(p2m); - ret = __p2m_lookup(d, gfn, t); + ret = p2m_get_entry(p2m, gfn, t, NULL, NULL); p2m_read_unlock(p2m); return ret; @@ -679,7 +668,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn, * No setting was found in the Radix tree. Check if the * entry exists in the page-tables. */ - mfn_t mfn = __p2m_lookup(d, gfn, NULL); + mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL); if ( mfn_eq(mfn, INVALID_MFN) ) return -ESRCH; @@ -1595,6 +1584,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) xenmem_access_t xma; p2m_type_t t; struct page_info *page = NULL; + struct p2m_domain *p2m = ¤t->domain->arch.p2m; rc = gva_to_ipa(gva, &ipa, flag); if ( rc < 0 ) @@ -1655,7 +1645,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) * We had a mem_access permission limiting the access, but the page type * could also be limiting, so we need to check that as well. */ - mfn = __p2m_lookup(current->domain, gfn, &t); + mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL); if ( mfn_eq(mfn, INVALID_MFN) ) goto err;
__p2m_lookup is just a wrapper to p2m_get_entry. Signed-off-by: Julien Grall <julien.grall@arm.com> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> Cc: Tamas K Lengyel <tamas@tklengyel.com> --- It might be possible to rework the memaccess code to take advantage of all the parameters. I will defer this to the memaccess folks. --- xen/arch/arm/p2m.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)