| Message ID | a1c29e58a5d40748413e8088ad88ba4319a328d4.1587735799.git.hongyxia@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | switch to domheap for Xen page tables | expand |
On 24.04.2020 16:09, Hongyan Xia wrote: > From: Wei Liu <wei.liu2@citrix.com> Nit: Why the emphasis on pl*e in the title? Is there anything left unconverted in the function? IOW how about "switch clone_mapping() to new page table APIs"? > --- a/xen/arch/x86/smpboot.c > +++ b/xen/arch/x86/smpboot.c > @@ -672,9 +672,9 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) > { > unsigned long linear = (unsigned long)ptr, pfn; > unsigned int flags; > - l3_pgentry_t *pl3e; > - l2_pgentry_t *pl2e; > - l1_pgentry_t *pl1e; > + l3_pgentry_t *pl3e = NULL; > + l2_pgentry_t *pl2e = NULL; > + l1_pgentry_t *pl1e = NULL; The latter two need initializers, yes, but why pl3e? > @@ -689,8 +689,8 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) > (linear >= XEN_VIRT_END && linear < DIRECTMAP_VIRT_START) ) > return -EINVAL; > > - pl3e = l4e_to_l3e(idle_pg_table[root_table_offset(linear)]) + > - l3_table_offset(linear); > + pl3e = map_l3t_from_l4e(idle_pg_table[root_table_offset(linear)]); > + pl3e += l3_table_offset(linear); By keeping original style (a single assignment) you'd have slightly less of a diff, and I think keeping original style where it's not colliding with any of our rules is generally a good idea. Doing so won't even make you hit the slightly flawed definition of map_l3t_from_l4e() at al (missing outer parentheses). I notice you do so ... > @@ -702,7 +702,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) > } > else > { > - pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(linear); > + pl2e = map_l2t_from_l3e(*pl3e) + l2_table_offset(linear); > flags = l2e_get_flags(*pl2e); > ASSERT(flags & _PAGE_PRESENT); > if ( flags & _PAGE_PSE ) > @@ -713,7 +713,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) > } > else > { > - pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear); > + pl1e = map_l1t_from_l2e(*pl2e) + l1_table_offset(linear); ... in both of these cases. > @@ -724,48 +724,61 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) > } > } > > + UNMAP_DOMAIN_PAGE(pl1e); > + UNMAP_DOMAIN_PAGE(pl2e); > + UNMAP_DOMAIN_PAGE(pl3e); > + > if ( !(root_get_flags(rpt[root_table_offset(linear)]) & _PAGE_PRESENT) ) > { > - pl3e = alloc_xen_pagetable(); > - if ( !pl3e ) > + mfn_t l3mfn = alloc_xen_pagetable_new(); > + > + if ( mfn_eq(l3mfn, INVALID_MFN) ) > goto out; > + > + pl3e = map_domain_page(l3mfn); Seeing this recur (from other patches) I wonder whether we wouldn't better make map_domain_page() accept INVALID_MFN and return NULL in this case. In cases like the one here it would eliminate the need for several local variables. Of course the downside of this is that then we'll have to start checking map_domain_page()'s return value. A middle ground could be to have void *alloc_mapped_pagetable(mfn_t *mfn); allowing to pass in NULL if the MFN is of no interest. > @@ -781,6 +794,9 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) > > rc = 0; > out: > + UNMAP_DOMAIN_PAGE(pl1e); > + UNMAP_DOMAIN_PAGE(pl2e); > + UNMAP_DOMAIN_PAGE(pl3e); > return rc; > } I don't think the writing of NULL into the variables is necessary here. And if the needed if()-s are of concern, then perhaps we should consider making unmap_domain_page() finally accept NULL as input? Jan
On Thu, 2020-04-30 at 17:15 +0200, Jan Beulich wrote: > On 24.04.2020 16:09, Hongyan Xia wrote: > > From: Wei Liu <wei.liu2@citrix.com> > > Nit: Why the emphasis on pl*e in the title? Is there anything left > unconverted in the function? IOW how about "switch clone_mapping() > to new page table APIs"? The title seems stale. Will fix. > ... > > @@ -724,48 +724,61 @@ static int clone_mapping(const void *ptr, > > root_pgentry_t *rpt) > > } > > } > > > > + UNMAP_DOMAIN_PAGE(pl1e); > > + UNMAP_DOMAIN_PAGE(pl2e); > > + UNMAP_DOMAIN_PAGE(pl3e); > > + > > if ( !(root_get_flags(rpt[root_table_offset(linear)]) & > > _PAGE_PRESENT) ) > > { > > - pl3e = alloc_xen_pagetable(); > > - if ( !pl3e ) > > + mfn_t l3mfn = alloc_xen_pagetable_new(); > > + > > + if ( mfn_eq(l3mfn, INVALID_MFN) ) > > goto out; > > + > > + pl3e = map_domain_page(l3mfn); > > Seeing this recur (from other patches) I wonder whether we wouldn't > better make map_domain_page() accept INVALID_MFN and return NULL in > this case. In cases like the one here it would eliminate the need > for several local variables. Of course the downside of this is that > then we'll have to start checking map_domain_page()'s return value. > A middle ground could be to have > > void *alloc_mapped_pagetable(mfn_t *mfn); > > allowing to pass in NULL if the MFN is of no interest. I would say that when the caller requires a new Xen page table allocation, almost all the time both the mfn and the virt are needed (on top of my head I cannot think of a case where we pass in NULL, you almost always need the mfn to write new page table entries), so I think the benefit of this is just compressing two calls into one, which I am not quite sure is worth it. > > @@ -781,6 +794,9 @@ static int clone_mapping(const void *ptr, > > root_pgentry_t *rpt) > > > > rc = 0; > > out: > > + UNMAP_DOMAIN_PAGE(pl1e); > > + UNMAP_DOMAIN_PAGE(pl2e); > > + UNMAP_DOMAIN_PAGE(pl3e); > > return rc; > > } > > I don't think the writing of NULL into the variables is necessary > here. And if the needed if()-s are of concern, then perhaps we > should consider making unmap_domain_page() finally accept NULL as > input? I usually don't have a problem with this because a sane compiler would definitely remove the unnecessary clearing, so I would use the macro version as much as possible. I am okay with moving the NULL check into unmap() itself, but note that this also needs changes on Arm side. Hongyan
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index 5b0e24f925..0e0ae56c76 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -672,9 +672,9 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) { unsigned long linear = (unsigned long)ptr, pfn; unsigned int flags; - l3_pgentry_t *pl3e; - l2_pgentry_t *pl2e; - l1_pgentry_t *pl1e; + l3_pgentry_t *pl3e = NULL; + l2_pgentry_t *pl2e = NULL; + l1_pgentry_t *pl1e = NULL; int rc = -ENOMEM; /* @@ -689,8 +689,8 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) (linear >= XEN_VIRT_END && linear < DIRECTMAP_VIRT_START) ) return -EINVAL; - pl3e = l4e_to_l3e(idle_pg_table[root_table_offset(linear)]) + - l3_table_offset(linear); + pl3e = map_l3t_from_l4e(idle_pg_table[root_table_offset(linear)]); + pl3e += l3_table_offset(linear); flags = l3e_get_flags(*pl3e); ASSERT(flags & _PAGE_PRESENT); @@ -702,7 +702,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) } else { - pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(linear); + pl2e = map_l2t_from_l3e(*pl3e) + l2_table_offset(linear); flags = l2e_get_flags(*pl2e); ASSERT(flags & _PAGE_PRESENT); if ( flags & _PAGE_PSE ) @@ -713,7 +713,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) } else { - pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear); + pl1e = map_l1t_from_l2e(*pl2e) + l1_table_offset(linear); flags = l1e_get_flags(*pl1e); if ( !(flags & _PAGE_PRESENT) ) { @@ -724,48 +724,61 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) } } + UNMAP_DOMAIN_PAGE(pl1e); + UNMAP_DOMAIN_PAGE(pl2e); + UNMAP_DOMAIN_PAGE(pl3e); + if ( !(root_get_flags(rpt[root_table_offset(linear)]) & _PAGE_PRESENT) ) { - pl3e = alloc_xen_pagetable(); - if ( !pl3e ) + mfn_t l3mfn = alloc_xen_pagetable_new(); + + if ( mfn_eq(l3mfn, INVALID_MFN) ) goto out; + + pl3e = map_domain_page(l3mfn); clear_page(pl3e); l4e_write(&rpt[root_table_offset(linear)], - l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR)); + l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR)); } else - pl3e = l4e_to_l3e(rpt[root_table_offset(linear)]); + pl3e = map_l3t_from_l4e(rpt[root_table_offset(linear)]); pl3e += l3_table_offset(linear); if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) ) { - pl2e = alloc_xen_pagetable(); - if ( !pl2e ) + mfn_t l2mfn = alloc_xen_pagetable_new(); + + if ( mfn_eq(l2mfn, INVALID_MFN) ) goto out; + + pl2e = map_domain_page(l2mfn); clear_page(pl2e); - l3e_write(pl3e, l3e_from_paddr(__pa(pl2e), __PAGE_HYPERVISOR)); + l3e_write(pl3e, l3e_from_mfn(l2mfn, __PAGE_HYPERVISOR)); } else { ASSERT(!(l3e_get_flags(*pl3e) & _PAGE_PSE)); - pl2e = l3e_to_l2e(*pl3e); + pl2e = map_l2t_from_l3e(*pl3e); } pl2e += l2_table_offset(linear); if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ) { - pl1e = alloc_xen_pagetable(); - if ( !pl1e ) + mfn_t l1mfn = alloc_xen_pagetable_new(); + + if ( mfn_eq(l1mfn, INVALID_MFN) ) goto out; + + pl1e = map_domain_page(l1mfn); clear_page(pl1e); - l2e_write(pl2e, l2e_from_paddr(__pa(pl1e), __PAGE_HYPERVISOR)); + l2e_write(pl2e, l2e_from_mfn(l1mfn, __PAGE_HYPERVISOR)); } else { ASSERT(!(l2e_get_flags(*pl2e) & _PAGE_PSE)); - pl1e = l2e_to_l1e(*pl2e); + pl1e = map_l1t_from_l2e(*pl2e); } pl1e += l1_table_offset(linear); @@ -781,6 +794,9 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt) rc = 0; out: + UNMAP_DOMAIN_PAGE(pl1e); + UNMAP_DOMAIN_PAGE(pl2e); + UNMAP_DOMAIN_PAGE(pl3e); return rc; }