[v5,2/7] x86: introduce a new set of APIs to manage Xen page tables

Commit Message

Xia, Hongyan Jan. 7, 2020, 12:06 p.m. UTC

From: Wei Liu <[e-mail]>

We are going to switch to using domheap page for page tables.
A new set of APIs is introduced to allocate, map, unmap and free pages
for page tables.

The allocation and deallocation work on mfn_t but not page_info,
because they are required to work even before frame table is set up.

Implement the old functions with the new ones. We will rewrite, site
by site, other mm functions that manipulate page tables to use the new
APIs.

Note these new APIs still use xenheap page underneath and no actual
map and unmap is done so that we don't break xen half way. They will
be switched to use domheap and dynamic mappings when usage of old APIs
is eliminated.

No functional change intended in this patch.

Signed-off-by: Wei Liu <[e-mail]>
Signed-off-by: Hongyan Xia <[e-mail]>
Reviewed-by: Julien Grall <[e-mail]>

---
Changed since v4:
- properly handle INVALID_MFN.
- remove the _new suffix for map/unmap_xen_pagetable because they do not
  have old alternatives.

Changed since v3:
- const qualify unmap_xen_pagetable_new().
- remove redundant parentheses.
---
 xen/arch/x86/mm.c        | 44 +++++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/mm.h | 11 +++++++++++
 2 files changed, 50 insertions(+), 5 deletions(-)

Comments

Jan Beulich Jan. 16, 2020, 12:04 p.m. UTC | #1

On 07.01.2020 13:06, Hongyan Xia wrote:
> @@ -4992,22 +4993,55 @@ int mmcfg_intercept_write(
>  }
>  
>  void *alloc_xen_pagetable(void)
> +{
> +    mfn_t mfn = alloc_xen_pagetable_new();
> +
> +    return mfn_eq(mfn, INVALID_MFN) ? NULL : mfn_to_virt(mfn_x(mfn));
> +}

Isn't it more dangerous to do the mapping this way round than the
opposite (new calls old)? Done the opposite way the new functions
could be switched to their new implementations ahead of the
removal of the old ones, and - if suitably isolated - perhaps
some of their users. Anyway, perhaps more a theoretical remark.

> +void free_xen_pagetable(void *v)
> +{
> +    mfn_t mfn = v ? virt_to_mfn(v) : INVALID_MFN;
> +
> +    if ( system_state != SYS_STATE_early_boot )
> +        free_xen_pagetable_new(mfn);

The condition is (partly) redundant with what
free_xen_pagetable_new() does. Therefore I'd like to ask that
either the if() be dropped here, or be completed by also
checking v to be non-NULL, at which point this would likely
become just

void free_xen_pagetable(void *v)
{
    if ( v && system_state != SYS_STATE_early_boot )
        free_xen_pagetable_new(virt_to_mfn(v));
}

> +/* v can point to an entry within a table or be NULL */
> +void unmap_xen_pagetable(const void *v)

Why "entry" in the comment?

Jan

Xia, Hongyan Jan. 27, 2020, 2:33 p.m. UTC | #2

On Thu, 2020-01-16 at 13:04 +0100, Jan Beulich wrote:
> ...
> 
> > +void free_xen_pagetable(void *v)
> > +{
> > +    mfn_t mfn = v ? virt_to_mfn(v) : INVALID_MFN;
> > +
> > +    if ( system_state != SYS_STATE_early_boot )
> > +        free_xen_pagetable_new(mfn);
> 
> The condition is (partly) redundant with what
> free_xen_pagetable_new() does. Therefore I'd like to ask that
> either the if() be dropped here, or be completed by also
> checking v to be non-NULL, at which point this would likely
> become just
> 
> void free_xen_pagetable(void *v)
> {
>     if ( v && system_state != SYS_STATE_early_boot )
>         free_xen_pagetable_new(virt_to_mfn(v));
> }

You are right. Will change in the next revision.

> > +/* v can point to an entry within a table or be NULL */
> > +void unmap_xen_pagetable(const void *v)
> 
> Why "entry" in the comment?

I think the comment originally meant pointing to the entry in its
parent table, which then meant pointing to this table. It's a bit
confusing. Will reword.

Reflecting back to your comment in v3 about whether the new Xen page
table mapping APIs (map/unmap_xen_pagetable) are really necessary, I
agree in the end they will just do the same thing as
map/unmap_domain_page, although one thing is that the latter suggests
it is trying to map a `domain' page, whose definition probably does not
include Xen page tables, so the name could be a bit confusing (well, we
could argue that Xen page tables are just idle `domain' pages so the
name still holds). If we are happy with using map_domain_page on Xen
PTE tables then I am okay with dropping the new mapping APIs and only
include the new alloc APIs.

Hongyan

Jan Beulich Jan. 27, 2020, 2:43 p.m. UTC | #3

On 27.01.2020 15:33, Xia, Hongyan wrote:
> On Thu, 2020-01-16 at 13:04 +0100, Jan Beulich wrote:
>> ...
>>
>>> +void free_xen_pagetable(void *v)
>>> +{
>>> +    mfn_t mfn = v ? virt_to_mfn(v) : INVALID_MFN;
>>> +
>>> +    if ( system_state != SYS_STATE_early_boot )
>>> +        free_xen_pagetable_new(mfn);
>>
>> The condition is (partly) redundant with what
>> free_xen_pagetable_new() does. Therefore I'd like to ask that
>> either the if() be dropped here, or be completed by also
>> checking v to be non-NULL, at which point this would likely
>> become just
>>
>> void free_xen_pagetable(void *v)
>> {
>>     if ( v && system_state != SYS_STATE_early_boot )
>>         free_xen_pagetable_new(virt_to_mfn(v));
>> }
> 
> You are right. Will change in the next revision.
> 
>>> +/* v can point to an entry within a table or be NULL */
>>> +void unmap_xen_pagetable(const void *v)
>>
>> Why "entry" in the comment?
> 
> I think the comment originally meant pointing to the entry in its
> parent table, which then meant pointing to this table. It's a bit
> confusing. Will reword.
> 
> Reflecting back to your comment in v3 about whether the new Xen page
> table mapping APIs (map/unmap_xen_pagetable) are really necessary, I
> agree in the end they will just do the same thing as
> map/unmap_domain_page, although one thing is that the latter suggests
> it is trying to map a `domain' page, whose definition probably does not
> include Xen page tables, so the name could be a bit confusing (well, we
> could argue that Xen page tables are just idle `domain' pages so the
> name still holds). If we are happy with using map_domain_page on Xen
> PTE tables then I am okay with dropping the new mapping APIs and only
> include the new alloc APIs.

Anyone on the To: list - opinions?

Thanks, Jan

Xia, Hongyan Jan. 27, 2020, 3:07 p.m. UTC | #4

On Mon, 2020-01-27 at 15:43 +0100, Jan Beulich wrote:
> On 27.01.2020 15:33, Xia, Hongyan wrote:
> > ...
> > 
> > Reflecting back to your comment in v3 about whether the new Xen
> > page
> > table mapping APIs (map/unmap_xen_pagetable) are really necessary,
> > I
> > agree in the end they will just do the same thing as
> > map/unmap_domain_page, although one thing is that the latter
> > suggests
> > it is trying to map a `domain' page, whose definition probably does
> > not
> > include Xen page tables, so the name could be a bit confusing
> > (well, we
> > could argue that Xen page tables are just idle `domain' pages so
> > the
> > name still holds). If we are happy with using map_domain_page on
> > Xen
> > PTE tables then I am okay with dropping the new mapping APIs and
> > only
> > include the new alloc APIs.
> 
> Anyone on the To: list - opinions?

There is one argument. We already use map_domain_page on non-domain
pages and outside the domheap. For example, the trap handler walks page
tables and print traces via map_domain_page regardless of where the
underlying page is from. The mapped page could just be from Xen.

Hongyan

George Dunlap Jan. 27, 2020, 5:13 p.m. UTC | #5

On 1/27/20 3:07 PM, Xia, Hongyan wrote:
> On Mon, 2020-01-27 at 15:43 +0100, Jan Beulich wrote:
>> On 27.01.2020 15:33, Xia, Hongyan wrote:
>>> ...
>>>
>>> Reflecting back to your comment in v3 about whether the new Xen
>>> page
>>> table mapping APIs (map/unmap_xen_pagetable) are really necessary,
>>> I
>>> agree in the end they will just do the same thing as
>>> map/unmap_domain_page, although one thing is that the latter
>>> suggests
>>> it is trying to map a `domain' page, whose definition probably does
>>> not
>>> include Xen page tables, so the name could be a bit confusing
>>> (well, we
>>> could argue that Xen page tables are just idle `domain' pages so
>>> the
>>> name still holds). If we are happy with using map_domain_page on
>>> Xen
>>> PTE tables then I am okay with dropping the new mapping APIs and
>>> only
>>> include the new alloc APIs.
>>
>> Anyone on the To: list - opinions?
> 
> There is one argument. We already use map_domain_page on non-domain
> pages and outside the domheap. For example, the trap handler walks page
> tables and print traces via map_domain_page regardless of where the
> underlying page is from. The mapped page could just be from Xen.

Yes, I've long sort of mentally filtered out the 'domain' part of
"map_domain_page()"; it's really an artifact of a bygone era, when there
were separate xen and domain heaps.  (In fact, it's really "map domheap
page", and at the moment we only  have a domheap.)

It might be worth thinking about doing a global
s/map_domain_page/map_page/; but until then, I think it's fine to use
"map_domain_page" for "map pages in the domheap", given that there *is*
only the domheap.

 -George

Wei Liu Jan. 27, 2020, 5:49 p.m. UTC | #6

On Thu, Jan 16, 2020 at 01:04:16PM +0100, Jan Beulich wrote:
[...]
> > +/* v can point to an entry within a table or be NULL */
> > +void unmap_xen_pagetable(const void *v)
> 
> Why "entry" in the comment?

IIRC there would be cases that umap_xen_pagetable would be called with a
pointer to a PTE.

The comment was mostly a note to remind me that when I got around
implementing that function it would need to do v&PAGE_MASK.

This may or may not apply to this patch series in its current form.

Wei.

> 
> Jan

Wei Liu Jan. 27, 2020, 5:51 p.m. UTC | #7

On Mon, Jan 27, 2020 at 03:43:12PM +0100, Jan Beulich wrote:
> On 27.01.2020 15:33, Xia, Hongyan wrote:
> > On Thu, 2020-01-16 at 13:04 +0100, Jan Beulich wrote:
> >> ...
> >>
> >>> +void free_xen_pagetable(void *v)
> >>> +{
> >>> +    mfn_t mfn = v ? virt_to_mfn(v) : INVALID_MFN;
> >>> +
> >>> +    if ( system_state != SYS_STATE_early_boot )
> >>> +        free_xen_pagetable_new(mfn);
> >>
> >> The condition is (partly) redundant with what
> >> free_xen_pagetable_new() does. Therefore I'd like to ask that
> >> either the if() be dropped here, or be completed by also
> >> checking v to be non-NULL, at which point this would likely
> >> become just
> >>
> >> void free_xen_pagetable(void *v)
> >> {
> >>     if ( v && system_state != SYS_STATE_early_boot )
> >>         free_xen_pagetable_new(virt_to_mfn(v));
> >> }
> > 
> > You are right. Will change in the next revision.
> > 
> >>> +/* v can point to an entry within a table or be NULL */
> >>> +void unmap_xen_pagetable(const void *v)
> >>
> >> Why "entry" in the comment?
> > 
> > I think the comment originally meant pointing to the entry in its
> > parent table, which then meant pointing to this table. It's a bit
> > confusing. Will reword.
> > 
> > Reflecting back to your comment in v3 about whether the new Xen page
> > table mapping APIs (map/unmap_xen_pagetable) are really necessary, I
> > agree in the end they will just do the same thing as
> > map/unmap_domain_page, although one thing is that the latter suggests
> > it is trying to map a `domain' page, whose definition probably does not
> > include Xen page tables, so the name could be a bit confusing (well, we
> > could argue that Xen page tables are just idle `domain' pages so the
> > name still holds). If we are happy with using map_domain_page on Xen
> > PTE tables then I am okay with dropping the new mapping APIs and only
> > include the new alloc APIs.
> 
> Anyone on the To: list - opinions?

I'm not too fussed about this.

I think eventually we will just change map_domain_page to map_page and
use that directly. The intermediate steps aren't terribly interesting to
me.

Wei.

> 
> Thanks, Jan

Patch

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index cc0d71996c..22b55390f1 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -119,6 +119,7 @@ 
 #include <xen/efi.h>
 #include <xen/grant_table.h>
 #include <xen/hypercall.h>
+#include <xen/mm.h>
 #include <asm/paging.h>
 #include <asm/shadow.h>
 #include <asm/page.h>
@@ -4992,22 +4993,55 @@  int mmcfg_intercept_write(
 }
 
 void *alloc_xen_pagetable(void)
+{
+    mfn_t mfn = alloc_xen_pagetable_new();
+
+    return mfn_eq(mfn, INVALID_MFN) ? NULL : mfn_to_virt(mfn_x(mfn));
+}
+
+void free_xen_pagetable(void *v)
+{
+    mfn_t mfn = v ? virt_to_mfn(v) : INVALID_MFN;
+
+    if ( system_state != SYS_STATE_early_boot )
+        free_xen_pagetable_new(mfn);
+}
+
+/*
+ * For these PTE APIs, the caller must follow the alloc-map-unmap-free
+ * lifecycle, which means explicitly mapping the PTE pages before accessing
+ * them. The caller must check whether the allocation has succeeded, and only
+ * pass valid MFNs to map_xen_pagetable().
+ */
+mfn_t alloc_xen_pagetable_new(void)
 {
     if ( system_state != SYS_STATE_early_boot )
     {
         void *ptr = alloc_xenheap_page();
 
         BUG_ON(!hardware_domain && !ptr);
-        return ptr;
+        return ptr ? virt_to_mfn(ptr) : INVALID_MFN;
     }
 
-    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
+    return alloc_boot_pages(1, 1);
 }
 
-void free_xen_pagetable(void *v)
+void *map_xen_pagetable(mfn_t mfn)
 {
-    if ( system_state != SYS_STATE_early_boot )
-        free_xenheap_page(v);
+    return mfn_to_virt(mfn_x(mfn));
+}
+
+/* v can point to an entry within a table or be NULL */
+void unmap_xen_pagetable(const void *v)
+{
+    /* XXX still using xenheap page, no need to do anything.  */
+}
+
+/* mfn can be INVALID_MFN */
+void free_xen_pagetable_new(mfn_t mfn)
+{
+    if ( system_state != SYS_STATE_early_boot && !mfn_eq(mfn, INVALID_MFN) )
+        free_xenheap_page(mfn_to_virt(mfn_x(mfn)));
 }
 
 static DEFINE_SPINLOCK(map_pgdir_lock);
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 2ca8882ad0..861edba34e 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -582,6 +582,17 @@  void *do_page_walk(struct vcpu *v, unsigned long addr);
 /* Allocator functions for Xen pagetables. */
 void *alloc_xen_pagetable(void);
 void free_xen_pagetable(void *v);
+mfn_t alloc_xen_pagetable_new(void);
+void *map_xen_pagetable(mfn_t mfn);
+void unmap_xen_pagetable(const void *v);
+void free_xen_pagetable_new(mfn_t mfn);
+
+#define UNMAP_XEN_PAGETABLE(ptr)    \
+    do {                            \
+        unmap_xen_pagetable(ptr);   \
+        (ptr) = NULL;               \
+    } while (0)
+
 l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
 
 int __sync_local_execstate(void);