From: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
---
xen/arch/x86/pv/dom0_build.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 5678da782d..28a939b68a 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -50,17 +50,17 @@ static __init void mark_pv_pt_pages_rdonly(struct domain *d,
unsigned long count;
struct page_info *page;
l4_pgentry_t *pl4e;
- l3_pgentry_t *pl3e;
- l2_pgentry_t *pl2e;
- l1_pgentry_t *pl1e;
+ l3_pgentry_t *pl3e, *l3t;
+ l2_pgentry_t *pl2e, *l2t;
+ l1_pgentry_t *pl1e, *l1t;
pl4e = l4start + l4_table_offset(vpt_start);
- pl3e = l4e_to_l3e(*pl4e);
- pl3e += l3_table_offset(vpt_start);
- pl2e = l3e_to_l2e(*pl3e);
- pl2e += l2_table_offset(vpt_start);
- pl1e = l2e_to_l1e(*pl2e);
- pl1e += l1_table_offset(vpt_start);
+ l3t = map_l3t_from_l4e(*pl4e);
+ pl3e = l3t + l3_table_offset(vpt_start);
+ l2t = map_l2t_from_l3e(*pl3e);
+ pl2e = l2t + l2_table_offset(vpt_start);
+ l1t = map_l1t_from_l2e(*pl2e);
+ pl1e = l1t + l1_table_offset(vpt_start);
for ( count = 0; count < nr_pt_pages; count++ )
{
l1e_remove_flags(*pl1e, _PAGE_RW);
@@ -85,12 +85,20 @@ static __init void mark_pv_pt_pages_rdonly(struct domain *d,
if ( !((unsigned long)++pl2e & (PAGE_SIZE - 1)) )
{
if ( !((unsigned long)++pl3e & (PAGE_SIZE - 1)) )
- pl3e = l4e_to_l3e(*++pl4e);
- pl2e = l3e_to_l2e(*pl3e);
+ {
+ unmap_domain_page(l3t);
+ pl3e = l3t = map_l3t_from_l4e(*++pl4e);
+ }
+ unmap_domain_page(l2t);
+ pl2e = l2t = map_l2t_from_l3e(*pl3e);
}
- pl1e = l2e_to_l1e(*pl2e);
+ unmap_domain_page(l1t);
+ pl1e = l1t = map_l1t_from_l2e(*pl2e);
}
}
+ unmap_domain_page(l1t);
+ unmap_domain_page(l2t);
+ unmap_domain_page(l3t);
}
static __init void setup_pv_physmap(struct domain *d, unsigned long pgtbl_pfn,
--
2.24.1.AMZN
Hi, On 17/04/2020 10:52, Hongyan Xia wrote: > From: Wei Liu <wei.liu2@citrix.com> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers, -- Julien Grall
On 17.04.2020 11:52, Hongyan Xia wrote: > --- a/xen/arch/x86/pv/dom0_build.c > +++ b/xen/arch/x86/pv/dom0_build.c > @@ -50,17 +50,17 @@ static __init void mark_pv_pt_pages_rdonly(struct domain *d, > unsigned long count; > struct page_info *page; > l4_pgentry_t *pl4e; > - l3_pgentry_t *pl3e; > - l2_pgentry_t *pl2e; > - l1_pgentry_t *pl1e; > + l3_pgentry_t *pl3e, *l3t; > + l2_pgentry_t *pl2e, *l2t; > + l1_pgentry_t *pl1e, *l1t; I don't quite see why the new local variables get introduced: unmap_domain_page(), iirc, is quite fine with a non-page- aligned argument. Jan
On Tue, Apr 28, 2020 at 05:33:29PM +0200, Jan Beulich wrote: > On 17.04.2020 11:52, Hongyan Xia wrote: > > --- a/xen/arch/x86/pv/dom0_build.c > > +++ b/xen/arch/x86/pv/dom0_build.c > > @@ -50,17 +50,17 @@ static __init void mark_pv_pt_pages_rdonly(struct domain *d, > > unsigned long count; > > struct page_info *page; > > l4_pgentry_t *pl4e; > > - l3_pgentry_t *pl3e; > > - l2_pgentry_t *pl2e; > > - l1_pgentry_t *pl1e; > > + l3_pgentry_t *pl3e, *l3t; > > + l2_pgentry_t *pl2e, *l2t; > > + l1_pgentry_t *pl1e, *l1t; > > I don't quite see why the new local variables get introduced: > unmap_domain_page(), iirc, is quite fine with a non-page- > aligned argument. (Assuming this is actually written by me) I wanted to make things abundantly clear: plXe points to an entry while lXt points to the start of a page table. In a long function the distinction could be helpful; in a short function (like this one?) not so much. Wei. > > Jan
On Tue, 2020-04-28 at 17:33 +0200, Jan Beulich wrote: > On 17.04.2020 11:52, Hongyan Xia wrote: > > --- a/xen/arch/x86/pv/dom0_build.c > > +++ b/xen/arch/x86/pv/dom0_build.c > > @@ -50,17 +50,17 @@ static __init void > > mark_pv_pt_pages_rdonly(struct domain *d, > > unsigned long count; > > struct page_info *page; > > l4_pgentry_t *pl4e; > > - l3_pgentry_t *pl3e; > > - l2_pgentry_t *pl2e; > > - l1_pgentry_t *pl1e; > > + l3_pgentry_t *pl3e, *l3t; > > + l2_pgentry_t *pl2e, *l2t; > > + l1_pgentry_t *pl1e, *l1t; > > I don't quite see why the new local variables get introduced: > unmap_domain_page(), iirc, is quite fine with a non-page- > aligned argument. You are right, although in this function, where plXe points to may not be the page we want to unmap. When plXe becomes aligned and points to a new page, we actually want to unmap the page before it increments to an aligned value. Hongyan
On Tue, 2020-04-28 at 16:55 +0100, Hongyan Xia wrote: > On Tue, 2020-04-28 at 17:33 +0200, Jan Beulich wrote: > > On 17.04.2020 11:52, Hongyan Xia wrote: > > > --- a/xen/arch/x86/pv/dom0_build.c > > > +++ b/xen/arch/x86/pv/dom0_build.c > > > @@ -50,17 +50,17 @@ static __init void > > > mark_pv_pt_pages_rdonly(struct domain *d, > > > unsigned long count; > > > struct page_info *page; > > > l4_pgentry_t *pl4e; > > > - l3_pgentry_t *pl3e; > > > - l2_pgentry_t *pl2e; > > > - l1_pgentry_t *pl1e; > > > + l3_pgentry_t *pl3e, *l3t; > > > + l2_pgentry_t *pl2e, *l2t; > > > + l1_pgentry_t *pl1e, *l1t; > > > > I don't quite see why the new local variables get introduced: > > unmap_domain_page(), iirc, is quite fine with a non-page- > > aligned argument. > > You are right, although in this function, where plXe points to may > not > be the page we want to unmap. When plXe becomes aligned and points to > a > new page, we actually want to unmap the page before it increments to > an > aligned value. Hmm, we can just unmap(plXe - 1) if my logic is correct, and save 3 local variables. Hongyan
On Tue, 2020-04-28 at 16:59 +0100, Hongyan Xia wrote: > On Tue, 2020-04-28 at 16:55 +0100, Hongyan Xia wrote: > > On Tue, 2020-04-28 at 17:33 +0200, Jan Beulich wrote: > > > On 17.04.2020 11:52, Hongyan Xia wrote: > > > > --- a/xen/arch/x86/pv/dom0_build.c > > > > +++ b/xen/arch/x86/pv/dom0_build.c > > > > @@ -50,17 +50,17 @@ static __init void > > > > mark_pv_pt_pages_rdonly(struct domain *d, > > > > unsigned long count; > > > > struct page_info *page; > > > > l4_pgentry_t *pl4e; > > > > - l3_pgentry_t *pl3e; > > > > - l2_pgentry_t *pl2e; > > > > - l1_pgentry_t *pl1e; > > > > + l3_pgentry_t *pl3e, *l3t; > > > > + l2_pgentry_t *pl2e, *l2t; > > > > + l1_pgentry_t *pl1e, *l1t; > > > > > > I don't quite see why the new local variables get introduced: > > > unmap_domain_page(), iirc, is quite fine with a non-page- > > > aligned argument. > > > > You are right, although in this function, where plXe points to may > > not > > be the page we want to unmap. When plXe becomes aligned and points > > to > > a > > new page, we actually want to unmap the page before it increments > > to > > an > > aligned value. > > Hmm, we can just unmap(plXe - 1) if my logic is correct, and save 3 > local variables. Sorry for monologuing, but I still prefer separating plXe and lXt because it makes it clear what we are unmapping. Unmapping plXe - 1 is a bit hackish. But if people do not have a problem with plXe - 1, I will post a new revision for that. Hongyan
On 29.04.2020 11:26, Hongyan Xia wrote: > But if people do not have a problem with plXe - 1, I will post a new > revision for that. I'd prefer that; I could live with the current version, but I'm not in favor of it. Jan
(Looks like other patches in this series have been merged. Replying to
this one only.)
From: Wei Liu <wei.liu2@citrix.com>
Date: Tue, 5 Feb 2019 16:32:54 +0000
Subject: [PATCH] x86/pv: map and unmap page tables in
mark_pv_pt_pages_rdonly
Also, clean up the initialisation of plXe.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
xen/arch/x86/pv/dom0_build.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/pv/dom0_build.c
b/xen/arch/x86/pv/dom0_build.c
index abfbe5f436..3522eb0114 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -49,18 +49,11 @@ static __init void mark_pv_pt_pages_rdonly(struct
domain *d,
{
unsigned long count;
struct page_info *page;
- l4_pgentry_t *pl4e;
- l3_pgentry_t *pl3e;
- l2_pgentry_t *pl2e;
- l1_pgentry_t *pl1e;
-
- pl4e = l4start + l4_table_offset(vpt_start);
- pl3e = l4e_to_l3e(*pl4e);
- pl3e += l3_table_offset(vpt_start);
- pl2e = l3e_to_l2e(*pl3e);
- pl2e += l2_table_offset(vpt_start);
- pl1e = l2e_to_l1e(*pl2e);
- pl1e += l1_table_offset(vpt_start);
+ l4_pgentry_t *pl4e = l4start + l4_table_offset(vpt_start);
+ l3_pgentry_t *pl3e = map_l3t_from_l4e(*pl4e) +
l3_table_offset(vpt_start);
+ l2_pgentry_t *pl2e = map_l2t_from_l3e(*pl3e) +
l2_table_offset(vpt_start);
+ l1_pgentry_t *pl1e = map_l1t_from_l2e(*pl2e) +
l1_table_offset(vpt_start);
+
for ( count = 0; count < nr_pt_pages; count++ )
{
l1e_remove_flags(*pl1e, _PAGE_RW);
@@ -85,12 +78,21 @@ static __init void mark_pv_pt_pages_rdonly(struct
domain *d,
if ( !((unsigned long)++pl2e & (PAGE_SIZE - 1)) )
{
if ( !((unsigned long)++pl3e & (PAGE_SIZE - 1)) )
- pl3e = l4e_to_l3e(*++pl4e);
- pl2e = l3e_to_l2e(*pl3e);
+ {
+ /* Need to unmap the page before the increment. */
+ unmap_domain_page(pl3e - 1);
+ pl3e = map_l3t_from_l4e(*++pl4e);
+ }
+ unmap_domain_page(pl2e - 1);
+ pl2e = map_l2t_from_l3e(*pl3e);
}
- pl1e = l2e_to_l1e(*pl2e);
+ unmap_domain_page(pl1e - 1);
+ pl1e = map_l1t_from_l2e(*pl2e);
}
}
+ unmap_domain_page(pl1e);
+ unmap_domain_page(pl2e);
+ unmap_domain_page(pl3e);
}
static __init void setup_pv_physmap(struct domain *d, unsigned long
pgtbl_pfn,
--
2.24.1.AMZN
On 29.04.2020 14:29, Hongyan Xia wrote:
> (Looks like other patches in this series have been merged. Replying to
> this one only.)
Please send as a proper patch, this one came through ...
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Tue, 5 Feb 2019 16:32:54 +0000
> Subject: [PATCH] x86/pv: map and unmap page tables in
> mark_pv_pt_pages_rdonly
>
> Also, clean up the initialisation of plXe.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/x86/pv/dom0_build.c | 32 +++++++++++++++++---------------
> 1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/pv/dom0_build.c
> b/xen/arch/x86/pv/dom0_build.c
> index abfbe5f436..3522eb0114 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -49,18 +49,11 @@ static __init void mark_pv_pt_pages_rdonly(struct
> domain *d,
> {
> unsigned long count;
> struct page_info *page;
> - l4_pgentry_t *pl4e;
> - l3_pgentry_t *pl3e;
> - l2_pgentry_t *pl2e;
> - l1_pgentry_t *pl1e;
> -
> - pl4e = l4start + l4_table_offset(vpt_start);
> - pl3e = l4e_to_l3e(*pl4e);
> - pl3e += l3_table_offset(vpt_start);
> - pl2e = l3e_to_l2e(*pl3e);
> - pl2e += l2_table_offset(vpt_start);
> - pl1e = l2e_to_l1e(*pl2e);
> - pl1e += l1_table_offset(vpt_start);
> + l4_pgentry_t *pl4e = l4start + l4_table_offset(vpt_start);
> + l3_pgentry_t *pl3e = map_l3t_from_l4e(*pl4e) +
> l3_table_offset(vpt_start);
> + l2_pgentry_t *pl2e = map_l2t_from_l3e(*pl3e) +
> l2_table_offset(vpt_start);
> + l1_pgentry_t *pl1e = map_l1t_from_l2e(*pl2e) +
> l1_table_offset(vpt_start);
... mangled anyway. I also think with the change made you need to
drop the R-b.
Jan
© 2016 - 2026 Red Hat, Inc.