Re: [PATCH 02/16] x86/P2M: introduce p2m_{add,remove}_page()
> On Feb 7, 2022, at 9:38 AM, Jan Beulich wrote: > > On 04.02.2022 23:07, George Dunlap wrote: >> On Mon, Jul 5, 2021 at 5:06 PM Jan Beulich wrote: >> >>> p2m_add_page() is simply a rename from guest_physmap_add_entry(). >>> p2m_remove_page() then is its counterpart, despite rendering >>> guest_physmap_remove_page(). > > First of all: It has been long ago that I noticed that this sentence > misses words. It now ends "... a trivial wrapper." > >>> This way callers can use suitable pairs of >>> functions (previously violated by hvm/grant_table.c). >>> >> >> Obviously this needs some clarification. While we're here, I find this a >> bit confusing; I tend to use the present tense for the way the code is >> before the patch, and the imperative for what the patch does; so Id' say: >> >> Rename guest_physmap_add_entry() to p2m_add_page; make >> guest_physmap_remove_page a wrapper with p2m_remove_page. That way callers >> can use suitable pairs... > > Well, yes, I understand you might word it this way. I'm not convinced > of the fixed scheme you mention for present vs imperative use to be a > universal fit though, requiring to always be followed. When reading > the description with the title in mind (and with the previously missing > words added), I find the use of present tense quite reasonable here. The way you wrote it is ambiguous grammatically; it could either mean, “Right now p2m_add_page() is simply a rename, and so…” or it could mean, “In this patch, p2m_add_page() is simply a rename.” If a reader starts interpreting it the first way, then they’ll read along until it doesn’t make sense any more, then have to re-evaluate the whole paragraph. It seems to me that my proposal is unambiguous. > I'm further slightly puzzled by you keeping the use of present tense in > "That way callers can use ...". I wouldn’t call that the present tense; I’m sure a real linguist would have a name for it. Consider the sentence, “Put the box near the door; that way we can find it easily when we need it.” The second half of the sentence is set in the hypothetical universe in which the imperative has been carried out. -George
Re: [PATCH 02/16] x86/P2M: introduce p2m_{add,remove}_page()
On 04.02.2022 23:07, George Dunlap wrote: > On Mon, Jul 5, 2021 at 5:06 PM Jan Beulich wrote: > >> p2m_add_page() is simply a rename from guest_physmap_add_entry(). >> p2m_remove_page() then is its counterpart, despite rendering >> guest_physmap_remove_page(). First of all: It has been long ago that I noticed that this sentence misses words. It now ends "... a trivial wrapper." >> This way callers can use suitable pairs of >> functions (previously violated by hvm/grant_table.c). >> > > Obviously this needs some clarification. While we're here, I find this a > bit confusing; I tend to use the present tense for the way the code is > before the patch, and the imperative for what the patch does; so Id' say: > > Rename guest_physmap_add_entry() to p2m_add_page; make > guest_physmap_remove_page a wrapper with p2m_remove_page. That way callers > can use suitable pairs... Well, yes, I understand you might word it this way. I'm not convinced of the fixed scheme you mention for present vs imperative use to be a universal fit though, requiring to always be followed. When reading the description with the title in mind (and with the previously missing words added), I find the use of present tense quite reasonable here. I'm further slightly puzzled by you keeping the use of present tense in "That way callers can use ...". Jan
Re: [PATCH 02/16] x86/P2M: introduce p2m_{add,remove}_page()
On Mon, Jul 5, 2021 at 5:06 PM Jan Beulich wrote: > p2m_add_page() is simply a rename from guest_physmap_add_entry(). > p2m_remove_page() then is its counterpart, despite rendering > guest_physmap_remove_page(). This way callers can use suitable pairs of > functions (previously violated by hvm/grant_table.c). > Obviously this needs some clarification. While we're here, I find this a bit confusing; I tend to use the present tense for the way the code is before the patch, and the imperative for what the patch does; so Id' say: Rename guest_physmap_add_entry() to p2m_add_page; make guest_physmap_remove_page a wrapper with p2m_remove_page. That way callers can use suitable pairs... Other than that looks good. -George
Re: [PATCH 02/16] x86/P2M: introduce p2m_{add,remove}_page()
On 05.07.2021 19:47, Paul Durrant wrote: > On 05/07/2021 17:06, Jan Beulich wrote: >> p2m_add_page() is simply a rename from guest_physmap_add_entry(). >> p2m_remove_page() then is its counterpart, despite rendering >> guest_physmap_remove_page(). > > Did some words get dropped in that second sentence? I can't really > understand what it is saying. Oops - this was meant to be "... a trivial wrapper". >> This way callers can use suitable pairs of >> functions (previously violated by hvm/grant_table.c). >> >> In HVM-specific code further avoid going through the guest_physmap_*() >> layer, and instead use the two new/renamed functions directly. >> >> Signed-off-by: Jan Beulich >> > > The code looks fine so... > > Reviewed-by: Paul Durrant Thanks. Jan
Re: [PATCH 02/16] x86/P2M: introduce p2m_{add,remove}_page()
On 05/07/2021 17:06, Jan Beulich wrote: p2m_add_page() is simply a rename from guest_physmap_add_entry(). p2m_remove_page() then is its counterpart, despite rendering guest_physmap_remove_page(). Did some words get dropped in that second sentence? I can't really understand what it is saying. This way callers can use suitable pairs of functions (previously violated by hvm/grant_table.c). In HVM-specific code further avoid going through the guest_physmap_*() layer, and instead use the two new/renamed functions directly. Signed-off-by: Jan Beulich The code looks fine so... Reviewed-by: Paul Durrant
[PATCH 02/16] x86/P2M: introduce p2m_{add,remove}_page()
p2m_add_page() is simply a rename from guest_physmap_add_entry().
p2m_remove_page() then is its counterpart, despite rendering
guest_physmap_remove_page(). This way callers can use suitable pairs of
functions (previously violated by hvm/grant_table.c).
In HVM-specific code further avoid going through the guest_physmap_*()
layer, and instead use the two new/renamed functions directly.
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -174,8 +174,7 @@ static int __init pvh_populate_memory_ra
continue;
}
-rc = guest_physmap_add_page(d, _gfn(start), page_to_mfn(page),
-order);
+rc = p2m_add_page(d, _gfn(start), page_to_mfn(page), order,
p2m_ram_rw);
if ( rc != 0 )
{
printk("Failed to populate memory: [%#lx,%#lx): %d\n",
--- a/xen/arch/x86/hvm/grant_table.c
+++ b/xen/arch/x86/hvm/grant_table.c
@@ -39,9 +39,8 @@ int create_grant_p2m_mapping(uint64_t ad
p2mt = p2m_grant_map_ro;
else
p2mt = p2m_grant_map_rw;
-rc = guest_physmap_add_entry(current->domain,
- _gfn(addr >> PAGE_SHIFT),
- frame, PAGE_ORDER_4K, p2mt);
+rc = p2m_add_page(current->domain, _gfn(addr >> PAGE_SHIFT),
+ frame, PAGE_ORDER_4K, p2mt);
if ( rc )
return GNTST_general_error;
else
@@ -68,7 +67,7 @@ int replace_grant_p2m_mapping(uint64_t a
type, mfn_x(old_mfn), mfn_x(frame));
return GNTST_general_error;
}
-if ( guest_physmap_remove_page(d, _gfn(gfn), frame, PAGE_ORDER_4K) )
+if ( p2m_remove_page(d, _gfn(gfn), frame, PAGE_ORDER_4K) )
{
put_gfn(d, gfn);
return GNTST_general_error;
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -188,8 +188,7 @@ static void hvm_remove_ioreq_gfn(struct
if ( gfn_eq(iorp->gfn, INVALID_GFN) )
return;
-if ( guest_physmap_remove_page(d, iorp->gfn,
- page_to_mfn(iorp->page), 0) )
+if ( p2m_remove_page(d, iorp->gfn, page_to_mfn(iorp->page), 0) )
domain_crash(d);
clear_page(iorp->va);
}
@@ -205,8 +204,7 @@ static int hvm_add_ioreq_gfn(struct iore
clear_page(iorp->va);
-rc = guest_physmap_add_page(d, iorp->gfn,
-page_to_mfn(iorp->page), 0);
+rc = p2m_add_page(d, iorp->gfn, page_to_mfn(iorp->page), 0, p2m_ram_rw);
if ( rc == 0 )
paging_mark_pfn_dirty(d, _pfn(gfn_x(iorp->gfn)));
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -829,15 +829,17 @@ p2m_remove_entry(struct p2m_domain *p2m,
}
int
-guest_physmap_remove_page(struct domain *d, gfn_t gfn,
- mfn_t mfn, unsigned int page_order)
+p2m_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
+unsigned int page_order)
{
struct p2m_domain *p2m = p2m_get_hostp2m(d);
int rc;
-/* IOMMU for PV guests is handled in get_page_type() and put_page(). */
if ( !paging_mode_translate(d) )
-return 0;
+{
+ASSERT_UNREACHABLE();
+return -EPERM;
+}
gfn_lock(p2m, gfn, page_order);
rc = p2m_remove_entry(p2m, gfn, mfn, page_order);
@@ -846,6 +848,17 @@ guest_physmap_remove_page(struct domain
return rc;
}
+int
+guest_physmap_remove_page(struct domain *d, gfn_t gfn,
+ mfn_t mfn, unsigned int page_order)
+{
+/* IOMMU for PV guests is handled in get_page_type() and put_page(). */
+if ( !paging_mode_translate(d) )
+return 0;
+
+return p2m_remove_page(d, gfn, mfn, page_order);
+}
+
#endif /* CONFIG_HVM */
int
@@ -884,14 +897,14 @@ guest_physmap_add_page(struct domain *d,
return 0;
}
-return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
+return p2m_add_page(d, gfn, mfn, page_order, p2m_ram_rw);
}
#ifdef CONFIG_HVM
int
-guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
-unsigned int page_order, p2m_type_t t)
+p2m_add_page(struct domain *d, gfn_t gfn, mfn_t mfn,
+ unsigned int page_order, p2m_type_t t)
{
struct p2m_domain *p2m = p2m_get_hostp2m(d);
unsigned long i;
@@ -2665,7 +2678,7 @@ static int p2m_add_foreign(struct domain
{
if ( is_special_page(mfn_to_page(prev_mfn)) )
/* Special pages are simply unhooked from this phys slot */
-rc = guest_physmap_remove_page(tdom, _gfn(gpfn), prev_mfn, 0);
+rc = p2m_remove_page(tdom, _gfn(gpfn), prev_mfn, 0);
else
/* Normal domain memory is freed, to avoid leaking memory. */
rc = guest_remove_page(tdom, gpfn);
@@ -2673,7 +2686,7 @@ static int p2m_add_foreign(struct domain
goto put_both;
}
/*
- * Create the new mapping. Can't use guest_physmap_add_page() because it
+ *
