| Message ID | fc340862-6842-3db4-1093-d2df44c1aa9c@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: large parts of P2M code and struct p2m_domain are HVM-only | expand |
On Mon, Jul 5, 2021 at 5:05 PM Jan Beulich <jbeulich@suse.com> wrote: > This is in preparation to re-using the original name. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Hey Jan, This series overall looks good; thanks for taking this on. Functionally this patch looks good; just one question... --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -788,8 +788,8 @@ void p2m_final_teardown(struct domain *d > #ifdef CONFIG_HVM > > static int __must_check > -p2m_remove_page(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, > - unsigned int page_order) > +p2m_remove_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, > + unsigned int page_order) > One question that's naturally raised for both this and the following patch is, what is the new naming "scheme" for these renamed functions, and how do they relate to the old scheme? Overall it seems like the intention is that "guest_physmap_..." can be called on a domain which may be PV or HVM, while "p2m_..." should only be called on HVM domains. There's also "..._entry" vs "..._page". Is the p2m_remove_page / p2m_remove_entry distinction have a meaning, and is it the same meaning as guest_physmap_add_page / guest_physmap_add_entry? Or is it similar to p2m_init_nestedp2m / p2m_nestedp2m_init -- we need both functions and don't want to make the names longer? -George
On 04.02.2022 22:54, George Dunlap wrote: > On Mon, Jul 5, 2021 at 5:05 PM Jan Beulich <jbeulich@suse.com> wrote: > --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -788,8 +788,8 @@ void p2m_final_teardown(struct domain *d >> #ifdef CONFIG_HVM >> >> static int __must_check >> -p2m_remove_page(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, >> - unsigned int page_order) >> +p2m_remove_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, >> + unsigned int page_order) >> > > One question that's naturally raised for both this and the following patch > is, what is the new naming "scheme" for these renamed functions, and how do > they relate to the old scheme? > > Overall it seems like the intention is that "guest_physmap_..." can be > called on a domain which may be PV or HVM, while "p2m_..." should only be > called on HVM domains. Yes. I think by the end of the series all p2m_...() named functions pertain to HVM domains only. > There's also "..._entry" vs "..._page". Is the p2m_remove_page / > p2m_remove_entry distinction have a meaning, and is it the same meaning as > guest_physmap_add_page / guest_physmap_add_entry? In the next patch a pair p2m_{add,remove}_page() is introduced. p2m_remove_entry() remains a static helper for the latter of the two, assuming the GFN is already locked. I've used the "page" vs "entry" in the names just like it was used prior to patch 2; I'd be happy to take suggestions on what else could be used in place of "entry" (but I'd like to stick to "page"). Jan
--- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -788,8 +788,8 @@ void p2m_final_teardown(struct domain *d #ifdef CONFIG_HVM static int __must_check -p2m_remove_page(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, - unsigned int page_order) +p2m_remove_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, + unsigned int page_order) { unsigned long i; p2m_type_t t; @@ -840,7 +840,7 @@ guest_physmap_remove_page(struct domain return 0; gfn_lock(p2m, gfn, page_order); - rc = p2m_remove_page(p2m, gfn, mfn, page_order); + rc = p2m_remove_entry(p2m, gfn, mfn, page_order); gfn_unlock(p2m, gfn, page_order); return rc; @@ -1009,7 +1009,7 @@ guest_physmap_add_entry(struct domain *d P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n", gfn_x(ogfn) , mfn_x(omfn)); if ( mfn_eq(omfn, mfn_add(mfn, i)) && - (rc = p2m_remove_page(p2m, ogfn, omfn, 0)) ) + (rc = p2m_remove_entry(p2m, ogfn, omfn, 0)) ) goto out; } } @@ -2382,7 +2382,7 @@ int p2m_change_altp2m_gfn(struct domain { mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL); rc = mfn_valid(mfn) - ? p2m_remove_page(ap2m, old_gfn, mfn, PAGE_ORDER_4K) + ? p2m_remove_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K) : 0; goto out; }
This is in preparation to re-using the original name. Signed-off-by: Jan Beulich <jbeulich@suse.com>