| Message ID | be6a34b3-8404-66be-7535-1631744ccf8c@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 21.06.17 at 17:20, <george.dunlap@citrix.com> wrote: > On 21/06/17 11:25, Jan Beulich wrote: >> Calculate entry PFN and flags just once, making the respective >> variables (and also pg) function wide. Take the opportunity and also >> make the induction variable unsigned. > > Hmm -- I'm a fan of keeping the scope of variables limited to where > they're actually used, to make sure stale values don't end up being > used. pg in particular I think should be kept local to the if() > statements; there's absolutely no benefit to making it function-wide. I > don't really see much benefit in making 'flags' and 'pfn' function-wide > either; it's not easier to read, IMHO, it just might save a tiny bit of > extra code, at the expense of removing some safety. I share your desire to restrict variable scope, with the difference that I think in the common case there shouldn't be multiple identically named variables in the same function. As to flags and pfn - they _are_ actually the same value in both if()-s, so other than for pg there's no risk of using stale values. With that it's hard to see for me why they should be fetched more than once in the function. To me this _is_ making the code easier to read. (In fact you widen the scope of both mentioned variables already anyway, and for pfn further widening would indeed no longer be necessary. For flags, though, I think it should still be pulled out to avoid the recurring l1e_get_flags().) Otoh it's not clear to me why, with your argumentation, other function wide variables (i, p2m_entry, new_entry, and perhaps more) would remain to be such. > If you want to avoid code duplication, it seems like merging the two > if() statements (of which at most one can be true) would be a better > approach. > > Something like the attached (build-tested only)? With the above in mind (as additional adjustments to make), that's certainly also an approach to take. Two cosmetic remarks though: You appear to both leave in place and introduce anew trailing white space, and neither flags nor level have a need to be unsigned long. Jan
On 22/06/17 08:25, Jan Beulich wrote: >>>> On 21.06.17 at 17:20, <george.dunlap@citrix.com> wrote: >> On 21/06/17 11:25, Jan Beulich wrote: >>> Calculate entry PFN and flags just once, making the respective >>> variables (and also pg) function wide. Take the opportunity and also >>> make the induction variable unsigned. >> >> Hmm -- I'm a fan of keeping the scope of variables limited to where >> they're actually used, to make sure stale values don't end up being >> used. pg in particular I think should be kept local to the if() >> statements; there's absolutely no benefit to making it function-wide. I >> don't really see much benefit in making 'flags' and 'pfn' function-wide >> either; it's not easier to read, IMHO, it just might save a tiny bit of >> extra code, at the expense of removing some safety. > > I share your desire to restrict variable scope, with the difference > that I think in the common case there shouldn't be multiple > identically named variables in the same function. As I said, one of my reasons for restricting variable scope is to make sure that stale values don't get reused. Having multiple instances of identically named variables used for different purposes is explicitly a case where the risk of stale values being reused is high. > As to flags and pfn - they _are_ actually the same value in both > if()-s, so other than for pg there's no risk of using stale values. > With that it's hard to see for me why they should be fetched > more than once in the function. To me this _is_ making the code > easier to read. If either `if` clause is executed, then the value of `flags` and `pfn` will be stale after that clause, since p2m_entry will have changed. As it happens, in the current code this doesn't matter because if the first `if` statement is executed, then the second one won't be (since `type` doesn't change); and since `flags` and `pfn` are only used in the two `if` clauses, then they won't be reused. But one could imagine a situation where someone adds some code below the second `if` clause which uses those values, and is is written with the assumption that no superpage is broken up; but if a superpage *is* broken up, would use the stale values. Keeping the scope local would prevent that. > (In fact you widen the scope of both mentioned > variables already anyway, and for pfn further widening would > indeed no longer be necessary. For flags, though, I think it > should still be pulled out to avoid the recurring l1e_get_flags().) > > Otoh it's not clear to me why, with your argumentation, other > function wide variables (i, p2m_entry, new_entry, and perhaps > more) would remain to be such. [snip] > With the above in mind (as additional adjustments to make), that's > certainly also an approach to take. Two cosmetic remarks though: > You appear to both leave in place and introduce anew trailing white > space, and neither flags nor level have a need to be unsigned long. Well I mostly wrote it to make sure the suggestion would actually work, and to make it clear what I was suggesting. I didn't intend to take over this clean-up work. -George
From 68931738e1f555c929cc77d9338043dc38f4610e Mon Sep 17 00:00:00 2001 From: George Dunlap <george.dunlap@citrix.com> Date: Wed, 21 Jun 2017 16:08:31 +0100 Subject: [PATCH] x86/p2m-pt: simplify p2m_next_level() [Insert description here] Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- xen/arch/x86/mm/p2m-pt.c | 77 +++++++++++++++++++++--------------------------- 1 file changed, 33 insertions(+), 44 deletions(-) diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index 06e64b8..4e1028e 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -195,7 +195,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table, l1_pgentry_t *p2m_entry; l1_pgentry_t new_entry; void *next; - int i; + unsigned int i; if ( !(p2m_entry = p2m_find_entry(*table, gfn_remainder, gfn, shift, max)) ) @@ -232,12 +232,10 @@ p2m_next_level(struct p2m_domain *p2m, void **table, } } - ASSERT(l1e_get_flags(*p2m_entry) & (_PAGE_PRESENT|_PAGE_PSE)); - /* split 1GB pages into 2MB pages */ - if ( type == PGT_l2_page_table && (l1e_get_flags(*p2m_entry) & _PAGE_PSE) ) + if ( l1e_get_flags(*p2m_entry) & _PAGE_PSE ) { - unsigned long flags, pfn; + unsigned long flags, pfn, level; struct page_info *pg; pg = p2m_alloc_ptp(p2m, PGT_l2_page_table); @@ -247,54 +245,45 @@ p2m_next_level(struct p2m_domain *p2m, void **table, flags = l1e_get_flags(*p2m_entry); pfn = l1e_get_pfn(*p2m_entry); - l1_entry = __map_domain_page(pg); - for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ ) + if ( type == PGT_l2_page_table ) { - new_entry = l1e_from_pfn(pfn | (i * L1_PAGETABLE_ENTRIES), flags); - p2m_add_iommu_flags(&new_entry, 1, IOMMUF_readable|IOMMUF_writable); - p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 2); + l1_entry = __map_domain_page(pg); + for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ ) + { + new_entry = l1e_from_pfn(pfn | (i * L1_PAGETABLE_ENTRIES), flags); + p2m_add_iommu_flags(&new_entry, 1, IOMMUF_readable|IOMMUF_writable); + p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 2); + } + level = 2; } - unmap_domain_page(l1_entry); - new_entry = l1e_from_pfn(mfn_x(page_to_mfn(pg)), - P2M_BASE_FLAGS | _PAGE_RW); /* disable PSE */ - p2m_add_iommu_flags(&new_entry, 2, IOMMUF_readable|IOMMUF_writable); - p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, 3); - } - - - /* split single 2MB large page into 4KB page in P2M table */ - if ( type == PGT_l1_page_table && (l1e_get_flags(*p2m_entry) & _PAGE_PSE) ) - { - unsigned long flags, pfn; - struct page_info *pg; - - pg = p2m_alloc_ptp(p2m, PGT_l1_page_table); - if ( pg == NULL ) - return -ENOMEM; - - /* New splintered mappings inherit the flags of the old superpage, - * with a little reorganisation for the _PAGE_PSE_PAT bit. */ - flags = l1e_get_flags(*p2m_entry); - pfn = l1e_get_pfn(*p2m_entry); - if ( pfn & 1 ) /* ==> _PAGE_PSE_PAT was set */ - pfn -= 1; /* Clear it; _PAGE_PSE becomes _PAGE_PAT */ else - flags &= ~_PAGE_PSE; /* Clear _PAGE_PSE (== _PAGE_PAT) */ - - l1_entry = __map_domain_page(pg); - for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ ) { - new_entry = l1e_from_pfn(pfn | i, flags); - p2m_add_iommu_flags(&new_entry, 0, 0); - p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 1); + /* New splintered mappings inherit the flags of the old superpage, + * with a little reorganisation for the _PAGE_PSE_PAT bit. */ + if ( pfn & 1 ) /* ==> _PAGE_PSE_PAT was set */ + pfn -= 1; /* Clear it; _PAGE_PSE becomes _PAGE_PAT */ + else + flags &= ~_PAGE_PSE; /* Clear _PAGE_PSE (== _PAGE_PAT) */ + + l1_entry = __map_domain_page(pg); + for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ ) + { + new_entry = l1e_from_pfn(pfn | i, flags); + p2m_add_iommu_flags(&new_entry, 0, 0); + p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 1); + } + level = 1; } - unmap_domain_page(l1_entry); + unmap_domain_page(l1_entry); + new_entry = l1e_from_pfn(mfn_x(page_to_mfn(pg)), P2M_BASE_FLAGS | _PAGE_RW); - p2m_add_iommu_flags(&new_entry, 1, IOMMUF_readable|IOMMUF_writable); - p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, 2); + p2m_add_iommu_flags(&new_entry, level, IOMMUF_readable|IOMMUF_writable); + p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level+1); } + else + ASSERT(l1e_get_flags(*p2m_entry) & _PAGE_PRESENT); next = map_domain_page(_mfn(l1e_get_pfn(*p2m_entry))); if ( unmap ) -- 2.1.4