While looking at "x86_64/mm: map and unmap page tables in destroy_compat_m2p_mapping" it occurred to me that the mappings changed there can be dropped altogether, as can the linear address range used for this. Note that both patches have only been lightly tested so far, I guess in particular the 2nd one may still have issues. 1: x86: drop unnecessary page table walking in compat r/o M2P handling 2: x86: drop high compat r/o M2P table address range Jan
On 15.04.2020 10:21, Jan Beulich wrote: > While looking at "x86_64/mm: map and unmap page tables in > destroy_compat_m2p_mapping" it occurred to me that the mappings > changed there can be dropped altogether, as can the linear > address range used for this. Note that both patches have only > been lightly tested so far, I guess in particular the 2nd one > may still have issues. > > 1: x86: drop unnecessary page table walking in compat r/o M2P handling > 2: x86: drop high compat r/o M2P table address range Just fyi - with the reviews I've got I'm intending to commit this later this week, unless I hear otherwise soon. Jan
We have a global variable where the necessary L2 table is recorded; no
need to inspect L4 and L3 tables (and this way a few less places will
eventually need adjustment when we want to support 5-level page tables).
Also avoid setting up the L3 entry, as the address range never gets used
anyway (it'll be dropped altogether in a subsequent patch).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -219,9 +219,7 @@ static void destroy_compat_m2p_mapping(s
{
unsigned long i, va, rwva, pt_pfn;
unsigned long smap = info->spfn, emap = info->spfn;
-
- l3_pgentry_t *l3_ro_mpt;
- l2_pgentry_t *l2_ro_mpt;
+ l2_pgentry_t *l2_ro_mpt = compat_idle_pg_table_l2;
if ( smap > ((RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) >> 2) )
return;
@@ -229,12 +227,6 @@ static void destroy_compat_m2p_mapping(s
if ( emap > ((RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) >> 2) )
emap = (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) >> 2;
- l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(HIRO_COMPAT_MPT_VIRT_START)]);
-
- ASSERT(l3e_get_flags(l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)]) & _PAGE_PRESENT);
-
- l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)]);
-
for ( i = smap; i < emap; )
{
va = HIRO_COMPAT_MPT_VIRT_START +
@@ -323,7 +315,6 @@ static int setup_compat_m2p_table(struct
unsigned long i, va, smap, emap, rwva, epfn = info->epfn;
mfn_t mfn;
unsigned int n;
- l3_pgentry_t *l3_ro_mpt = NULL;
l2_pgentry_t *l2_ro_mpt = NULL;
int err = 0;
@@ -342,13 +333,7 @@ static int setup_compat_m2p_table(struct
emap = ( (epfn + ((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1 )) &
~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) );
- va = HIRO_COMPAT_MPT_VIRT_START +
- smap * sizeof(*compat_machine_to_phys_mapping);
- l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(va)]);
-
- ASSERT(l3e_get_flags(l3_ro_mpt[l3_table_offset(va)]) & _PAGE_PRESENT);
-
- l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
+ l2_ro_mpt = compat_idle_pg_table_l2;
#define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int))
#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
@@ -627,16 +612,10 @@ void __init paging_init(void)
#undef MFN
/* Create user-accessible L2 directory to map the MPT for compat guests. */
- BUILD_BUG_ON(l4_table_offset(RDWR_MPT_VIRT_START) !=
- l4_table_offset(HIRO_COMPAT_MPT_VIRT_START));
- l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(
- HIRO_COMPAT_MPT_VIRT_START)]);
if ( (l2_ro_mpt = alloc_xen_pagetable()) == NULL )
goto nomem;
compat_idle_pg_table_l2 = l2_ro_mpt;
clear_page(l2_ro_mpt);
- l3e_write(&l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)],
- l3e_from_paddr(__pa(l2_ro_mpt), __PAGE_HYPERVISOR_RO));
l2_ro_mpt += l2_table_offset(HIRO_COMPAT_MPT_VIRT_START);
/* Allocate and map the compatibility mode machine-to-phys table. */
mpt_size = (mpt_size >> 1) + (1UL << (L2_PAGETABLE_SHIFT - 1));
On Wed, 2020-04-15 at 10:23 +0200, Jan Beulich wrote:
> We have a global variable where the necessary L2 table is recorded;
> no
> need to inspect L4 and L3 tables (and this way a few less places will
> eventually need adjustment when we want to support 5-level page
> tables).
> Also avoid setting up the L3 entry, as the address range never gets
> used
> anyway (it'll be dropped altogether in a subsequent patch).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -219,9 +219,7 @@ static void destroy_compat_m2p_mapping(s
> {
> unsigned long i, va, rwva, pt_pfn;
> unsigned long smap = info->spfn, emap = info->spfn;
> -
> - l3_pgentry_t *l3_ro_mpt;
> - l2_pgentry_t *l2_ro_mpt;
> + l2_pgentry_t *l2_ro_mpt = compat_idle_pg_table_l2;
>
> if ( smap > ((RDWR_COMPAT_MPT_VIRT_END -
> RDWR_COMPAT_MPT_VIRT_START) >> 2) )
> return;
> @@ -229,12 +227,6 @@ static void destroy_compat_m2p_mapping(s
> if ( emap > ((RDWR_COMPAT_MPT_VIRT_END -
> RDWR_COMPAT_MPT_VIRT_START) >> 2) )
> emap = (RDWR_COMPAT_MPT_VIRT_END -
> RDWR_COMPAT_MPT_VIRT_START) >> 2;
>
> - l3_ro_mpt =
> l4e_to_l3e(idle_pg_table[l4_table_offset(HIRO_COMPAT_MPT_VIRT_START)]
> );
> -
> - ASSERT(l3e_get_flags(l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_V
> IRT_START)]) & _PAGE_PRESENT);
> -
> - l2_ro_mpt =
> l3e_to_l2e(l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)]);
> -
> for ( i = smap; i < emap; )
> {
> va = HIRO_COMPAT_MPT_VIRT_START +
> @@ -323,7 +315,6 @@ static int setup_compat_m2p_table(struct
> unsigned long i, va, smap, emap, rwva, epfn = info->epfn;
> mfn_t mfn;
> unsigned int n;
> - l3_pgentry_t *l3_ro_mpt = NULL;
> l2_pgentry_t *l2_ro_mpt = NULL;
> int err = 0;
>
> @@ -342,13 +333,7 @@ static int setup_compat_m2p_table(struct
> emap = ( (epfn + ((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1 )) &
> ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) );
>
> - va = HIRO_COMPAT_MPT_VIRT_START +
> - smap * sizeof(*compat_machine_to_phys_mapping);
> - l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(va)]);
> -
> - ASSERT(l3e_get_flags(l3_ro_mpt[l3_table_offset(va)]) &
> _PAGE_PRESENT);
> -
> - l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> + l2_ro_mpt = compat_idle_pg_table_l2;
>
> #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int))
> #define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
> @@ -627,16 +612,10 @@ void __init paging_init(void)
> #undef MFN
>
> /* Create user-accessible L2 directory to map the MPT for compat
> guests. */
> - BUILD_BUG_ON(l4_table_offset(RDWR_MPT_VIRT_START) !=
> - l4_table_offset(HIRO_COMPAT_MPT_VIRT_START));
> - l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(
> - HIRO_COMPAT_MPT_VIRT_START)]);
> if ( (l2_ro_mpt = alloc_xen_pagetable()) == NULL )
> goto nomem;
> compat_idle_pg_table_l2 = l2_ro_mpt;
> clear_page(l2_ro_mpt);
> - l3e_write(&l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)
> ],
> - l3e_from_paddr(__pa(l2_ro_mpt),
> __PAGE_HYPERVISOR_RO));
> l2_ro_mpt += l2_table_offset(HIRO_COMPAT_MPT_VIRT_START);
> /* Allocate and map the compatibility mode machine-to-phys
> table. */
> mpt_size = (mpt_size >> 1) + (1UL << (L2_PAGETABLE_SHIFT - 1));
The code around here, I am wondering if there is a reason to put it in
this patch. If we bisect, we can end up in a commit which says the
address range of compat is still there in config.h, but actually it's
gone, so this probably belongs to the 2nd patch.
Apart from that,
Reviewed-by: Hongyan Xia <hongyxia@amazon.com>
I would like to drop relevant map/unmap patches and replace them with
the new clean-up ones (and place them at the beginning of the series),
if there is no objection with that.
Hongyan
On 15.04.2020 11:59, Hongyan Xia wrote: > On Wed, 2020-04-15 at 10:23 +0200, Jan Beulich wrote: >> @@ -627,16 +612,10 @@ void __init paging_init(void) >> #undef MFN >> >> /* Create user-accessible L2 directory to map the MPT for compat >> guests. */ >> - BUILD_BUG_ON(l4_table_offset(RDWR_MPT_VIRT_START) != >> - l4_table_offset(HIRO_COMPAT_MPT_VIRT_START)); >> - l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset( >> - HIRO_COMPAT_MPT_VIRT_START)]); >> if ( (l2_ro_mpt = alloc_xen_pagetable()) == NULL ) >> goto nomem; >> compat_idle_pg_table_l2 = l2_ro_mpt; >> clear_page(l2_ro_mpt); >> - l3e_write(&l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START) >> ], >> - l3e_from_paddr(__pa(l2_ro_mpt), >> __PAGE_HYPERVISOR_RO)); >> l2_ro_mpt += l2_table_offset(HIRO_COMPAT_MPT_VIRT_START); >> /* Allocate and map the compatibility mode machine-to-phys >> table. */ >> mpt_size = (mpt_size >> 1) + (1UL << (L2_PAGETABLE_SHIFT - 1)); > > The code around here, I am wondering if there is a reason to put it in > this patch. If we bisect, we can end up in a commit which says the > address range of compat is still there in config.h, but actually it's > gone, so this probably belongs to the 2nd patch. I could be done either way, I guess. Note though how patch 2's description starts with "Now that we don't properly hook things up into the page tables anymore". I.e. after this patch the address range continues to exist solely for calculation purposes. > Apart from that, > Reviewed-by: Hongyan Xia <hongyxia@amazon.com> Thanks. > I would like to drop relevant map/unmap patches and replace them with > the new clean-up ones (and place them at the beginning of the series), > if there is no objection with that. Depending on turnaround, I'd much rather see this go in before you re-post. But if you feel like making it part of your series, go ahead. Jan
On Wed, 2020-04-15 at 12:34 +0200, Jan Beulich wrote: > On 15.04.2020 11:59, Hongyan Xia wrote: > > ... > > I would like to drop relevant map/unmap patches and replace them > > with > > the new clean-up ones (and place them at the beginning of the > > series), > > if there is no objection with that. > > Depending on turnaround, I'd much rather see this go in before > you re-post. But if you feel like making it part of your series, > go ahead. I actually also very much prefer to see those clean-up patches go in before I post the next revision, so please go ahead. I will post the next revision minus patch 4 then to avoid conflict. Hongyan
On Wed, Apr 15, 2020 at 10:23:31AM +0200, Jan Beulich wrote: > We have a global variable where the necessary L2 table is recorded; no > need to inspect L4 and L3 tables (and this way a few less places will > eventually need adjustment when we want to support 5-level page tables). > Also avoid setting up the L3 entry, as the address range never gets used > anyway (it'll be dropped altogether in a subsequent patch). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Wei Liu <wl@xen.org>
Now that we don't properly hook things up into the page tables anymore
we also don't need to set aside an address range. Drop it, using
compat_idle_pg_table_l2[] simply (explicitly) from slot 0.
While doing the re-arrangement, which is accompanied by the dropping or
replacing of some local variables, restrict the scopes of some further
ones at the same time.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: With the changed usage perhaps we want to also rename
compat_idle_pg_table_l2[] (to e.g. compat_idle_l2_entries[])?
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1454,8 +1454,7 @@ static bool pae_xen_mappings_check(const
void init_xen_pae_l2_slots(l2_pgentry_t *l2t, const struct domain *d)
{
memcpy(&l2t[COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(d)],
- &compat_idle_pg_table_l2[
- l2_table_offset(HIRO_COMPAT_MPT_VIRT_START)],
+ compat_idle_pg_table_l2,
COMPAT_L2_PAGETABLE_XEN_SLOTS(d) * sizeof(*l2t));
}
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -217,9 +217,7 @@ static int share_hotadd_m2p_table(struct
static void destroy_compat_m2p_mapping(struct mem_hotadd_info *info)
{
- unsigned long i, va, rwva, pt_pfn;
- unsigned long smap = info->spfn, emap = info->spfn;
- l2_pgentry_t *l2_ro_mpt = compat_idle_pg_table_l2;
+ unsigned long i, smap = info->spfn, emap = info->spfn;
if ( smap > ((RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) >> 2) )
return;
@@ -229,18 +227,19 @@ static void destroy_compat_m2p_mapping(s
for ( i = smap; i < emap; )
{
- va = HIRO_COMPAT_MPT_VIRT_START +
- i * sizeof(*compat_machine_to_phys_mapping);
- rwva = RDWR_COMPAT_MPT_VIRT_START +
- i * sizeof(*compat_machine_to_phys_mapping);
- if ( l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) & _PAGE_PRESENT )
+ unsigned int off = i * sizeof(*compat_machine_to_phys_mapping);
+ l2_pgentry_t *pl2e = compat_idle_pg_table_l2 + l2_table_offset(off);
+
+ if ( l2e_get_flags(*pl2e) & _PAGE_PRESENT )
{
- pt_pfn = l2e_get_pfn(l2_ro_mpt[l2_table_offset(va)]);
+ unsigned long pt_pfn = l2e_get_pfn(*pl2e);
+
if ( hotadd_mem_valid(pt_pfn, info) )
{
- destroy_xen_mappings(rwva, rwva +
- (1UL << L2_PAGETABLE_SHIFT));
- l2e_write(&l2_ro_mpt[l2_table_offset(va)], l2e_empty());
+ unsigned long rwva = RDWR_COMPAT_MPT_VIRT_START + off;
+
+ destroy_xen_mappings(rwva, rwva + (1UL << L2_PAGETABLE_SHIFT));
+ l2e_write(pl2e, l2e_empty());
}
}
@@ -312,10 +311,9 @@ static void destroy_m2p_mapping(struct m
*/
static int setup_compat_m2p_table(struct mem_hotadd_info *info)
{
- unsigned long i, va, smap, emap, rwva, epfn = info->epfn;
+ unsigned long i, smap, emap, epfn = info->epfn;
mfn_t mfn;
unsigned int n;
- l2_pgentry_t *l2_ro_mpt = NULL;
int err = 0;
smap = info->spfn & (~((1UL << (L2_PAGETABLE_SHIFT - 2)) -1));
@@ -333,8 +331,6 @@ static int setup_compat_m2p_table(struct
emap = ( (epfn + ((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1 )) &
~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) );
- l2_ro_mpt = compat_idle_pg_table_l2;
-
#define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int))
#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
sizeof(*compat_machine_to_phys_mapping))
@@ -343,13 +339,11 @@ static int setup_compat_m2p_table(struct
for ( i = smap; i < emap; i += (1UL << (L2_PAGETABLE_SHIFT - 2)) )
{
- va = HIRO_COMPAT_MPT_VIRT_START +
- i * sizeof(*compat_machine_to_phys_mapping);
-
- rwva = RDWR_COMPAT_MPT_VIRT_START +
- i * sizeof(*compat_machine_to_phys_mapping);
+ unsigned int off = i * sizeof(*compat_machine_to_phys_mapping);
+ l2_pgentry_t *pl2e = compat_idle_pg_table_l2 + l2_table_offset(off);
+ unsigned long rwva = RDWR_COMPAT_MPT_VIRT_START + off;
- if (l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) & _PAGE_PRESENT)
+ if ( l2e_get_flags(*pl2e) & _PAGE_PRESENT )
continue;
for ( n = 0; n < CNT; ++n)
@@ -366,8 +360,7 @@ static int setup_compat_m2p_table(struct
/* Fill with INVALID_M2P_ENTRY. */
memset((void *)rwva, 0xFF, 1UL << L2_PAGETABLE_SHIFT);
/* NB. Cannot be GLOBAL as the ptes get copied into per-VM space. */
- l2e_write(&l2_ro_mpt[l2_table_offset(va)],
- l2e_from_mfn(mfn, _PAGE_PSE|_PAGE_PRESENT));
+ l2e_write(pl2e, l2e_from_mfn(mfn, _PAGE_PSE|_PAGE_PRESENT));
}
#undef CNT
#undef MFN
@@ -616,7 +609,6 @@ void __init paging_init(void)
goto nomem;
compat_idle_pg_table_l2 = l2_ro_mpt;
clear_page(l2_ro_mpt);
- l2_ro_mpt += l2_table_offset(HIRO_COMPAT_MPT_VIRT_START);
/* Allocate and map the compatibility mode machine-to-phys table. */
mpt_size = (mpt_size >> 1) + (1UL << (L2_PAGETABLE_SHIFT - 1));
if ( mpt_size > RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START )
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -215,11 +215,8 @@ extern unsigned char boot_edid_info[128]
/* Slot 261: compatibility machine-to-phys conversion table (1GB). */
#define RDWR_COMPAT_MPT_VIRT_START VMAP_VIRT_END
#define RDWR_COMPAT_MPT_VIRT_END (RDWR_COMPAT_MPT_VIRT_START + GB(1))
-/* Slot 261: high read-only compat machine-to-phys conversion table (1GB). */
-#define HIRO_COMPAT_MPT_VIRT_START RDWR_COMPAT_MPT_VIRT_END
-#define HIRO_COMPAT_MPT_VIRT_END (HIRO_COMPAT_MPT_VIRT_START + GB(1))
/* Slot 261: xen text, static data, bss, per-cpu stubs and executable fixmap (1GB). */
-#define XEN_VIRT_START (HIRO_COMPAT_MPT_VIRT_END)
+#define XEN_VIRT_START RDWR_COMPAT_MPT_VIRT_END
#define XEN_VIRT_END (XEN_VIRT_START + GB(1))
#ifndef CONFIG_BIGMEM
On Wed, Apr 15, 2020 at 10:23:58AM +0200, Jan Beulich wrote: > Now that we don't properly hook things up into the page tables anymore > we also don't need to set aside an address range. Drop it, using > compat_idle_pg_table_l2[] simply (explicitly) from slot 0. > > While doing the re-arrangement, which is accompanied by the dropping or > replacing of some local variables, restrict the scopes of some further > ones at the same time. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Wei Liu <wl@xen.org> > --- > TBD: With the changed usage perhaps we want to also rename > compat_idle_pg_table_l2[] (to e.g. compat_idle_l2_entries[])? No opinion on this.
© 2016 - 2024 Red Hat, Inc.