| Message ID | 2fa83ef5818805c179757caac99ccf7ab4f7ba3a.1584955616.git.hongyxia@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | use new API for Xen page tables | expand |
On 23.03.2020 10:41, Hongyan Xia wrote: > --- a/xen/include/asm-x86/page.h > +++ b/xen/include/asm-x86/page.h > @@ -196,6 +196,24 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags) > #define map_l2t_from_l3e(x) (l2_pgentry_t *)map_domain_page(l3e_get_mfn(x)) > #define map_l3t_from_l4e(x) (l3_pgentry_t *)map_domain_page(l4e_get_mfn(x)) > > +#define l1e_from_l2e(l2e, off) ({ \ > + l1_pgentry_t *l1t = map_l1t_from_l2e(l2e); \ > + l1_pgentry_t l1e = l1t[off]; \ > + UNMAP_DOMAIN_PAGE(l1t); \ > + l1e; }) > + > +#define l2e_from_l3e(l3e, off) ({ \ > + l2_pgentry_t *l2t = map_l2t_from_l3e(l3e); \ > + l2_pgentry_t l2e = l2t[off]; \ > + UNMAP_DOMAIN_PAGE(l2t); \ > + l2e; }) > + > +#define l3e_from_l4e(l4e, off) ({ \ > + l3_pgentry_t *l3t = map_l3t_from_l4e(l4e); \ > + l3_pgentry_t l3e = l3t[off]; \ > + UNMAP_DOMAIN_PAGE(l3t); \ > + l3e; }) There's a reason these are macros rather than inline functions, I assume? (This reason would be nice to be stated in the description.) I don't see why you use UNMAP_DOMAIN_PAGE() rather than unmap_domain_page() here - the variables in question go out of scope right afterwards, so the storing of NULL into them is pointless (and very likely to be eliminated by the compiler anyway). Finally the pointers should each be "to const", as you're only reading through them. Jan
On Wed, 2020-04-01 at 14:29 +0200, Jan Beulich wrote: > On 23.03.2020 10:41, Hongyan Xia wrote: > > --- a/xen/include/asm-x86/page.h > > +++ b/xen/include/asm-x86/page.h > > @@ -196,6 +196,24 @@ static inline l4_pgentry_t > > l4e_from_paddr(paddr_t pa, unsigned int flags) > > #define map_l2t_from_l3e(x) (l2_pgentry_t > > *)map_domain_page(l3e_get_mfn(x)) > > #define map_l3t_from_l4e(x) (l3_pgentry_t > > *)map_domain_page(l4e_get_mfn(x)) > > > > +#define l1e_from_l2e(l2e, off) ({ \ > > + l1_pgentry_t *l1t = map_l1t_from_l2e(l2e); \ > > + l1_pgentry_t l1e = l1t[off]; \ > > + UNMAP_DOMAIN_PAGE(l1t); \ > > + l1e; }) > > + > > +#define l2e_from_l3e(l3e, off) ({ \ > > + l2_pgentry_t *l2t = map_l2t_from_l3e(l3e); \ > > + l2_pgentry_t l2e = l2t[off]; \ > > + UNMAP_DOMAIN_PAGE(l2t); \ > > + l2e; }) > > + > > +#define l3e_from_l4e(l4e, off) ({ \ > > + l3_pgentry_t *l3t = map_l3t_from_l4e(l4e); \ > > + l3_pgentry_t l3e = l3t[off]; \ > > + UNMAP_DOMAIN_PAGE(l3t); \ > > + l3e; }) > > There's a reason these are macros rather than inline functions, > I assume? (This reason would be nice to be stated in the > description.) Actually no specific reasons, just to keep them as macros to be consistent with other helpers above. Fairly trivial to convert them into inline functions if this is desired. > I don't see why you use UNMAP_DOMAIN_PAGE() rather than > unmap_domain_page() here - the variables in question go out of > scope right afterwards, so the storing of NULL into them is > pointless (and very likely to be eliminated by the compiler > anyway). My intention is to just avoid using the lower case one altogether, so that one day when we need to expand any function or macros we do not need to look back at the code and worry about whether any lower case ones need to be converted to upper case (sometimes for large functions this may not be trivial), and the compiler can deal with the extra nullification anyway. > Finally the pointers should each be "to const", as you're > only reading through them. Good point. Will const qualify in next revision. Thanks, Hongyan
On 07.04.2020 17:11, Hongyan Xia wrote: > On Wed, 2020-04-01 at 14:29 +0200, Jan Beulich wrote: >> On 23.03.2020 10:41, Hongyan Xia wrote: >>> --- a/xen/include/asm-x86/page.h >>> +++ b/xen/include/asm-x86/page.h >>> @@ -196,6 +196,24 @@ static inline l4_pgentry_t >>> l4e_from_paddr(paddr_t pa, unsigned int flags) >>> #define map_l2t_from_l3e(x) (l2_pgentry_t >>> *)map_domain_page(l3e_get_mfn(x)) >>> #define map_l3t_from_l4e(x) (l3_pgentry_t >>> *)map_domain_page(l4e_get_mfn(x)) >>> >>> +#define l1e_from_l2e(l2e, off) ({ \ >>> + l1_pgentry_t *l1t = map_l1t_from_l2e(l2e); \ >>> + l1_pgentry_t l1e = l1t[off]; \ >>> + UNMAP_DOMAIN_PAGE(l1t); \ >>> + l1e; }) >>> + >>> +#define l2e_from_l3e(l3e, off) ({ \ >>> + l2_pgentry_t *l2t = map_l2t_from_l3e(l3e); \ >>> + l2_pgentry_t l2e = l2t[off]; \ >>> + UNMAP_DOMAIN_PAGE(l2t); \ >>> + l2e; }) >>> + >>> +#define l3e_from_l4e(l4e, off) ({ \ >>> + l3_pgentry_t *l3t = map_l3t_from_l4e(l4e); \ >>> + l3_pgentry_t l3e = l3t[off]; \ >>> + UNMAP_DOMAIN_PAGE(l3t); \ >>> + l3e; }) >> >> There's a reason these are macros rather than inline functions, >> I assume? (This reason would be nice to be stated in the >> description.) > > Actually no specific reasons, just to keep them as macros to be > consistent with other helpers above. Fairly trivial to convert them > into inline functions if this is desired. Where possible this would be the preferred route for new helpers. >> I don't see why you use UNMAP_DOMAIN_PAGE() rather than >> unmap_domain_page() here - the variables in question go out of >> scope right afterwards, so the storing of NULL into them is >> pointless (and very likely to be eliminated by the compiler >> anyway). > > My intention is to just avoid using the lower case one altogether, so > that one day when we need to expand any function or macros we do not > need to look back at the code and worry about whether any lower case > ones need to be converted to upper case (sometimes for large functions > this may not be trivial), and the compiler can deal with the extra > nullification anyway. I don't agree with this goal; perhaps others on Cc have an opinion? Jan
On Wed, 2020-04-01 at 14:29 +0200, Jan Beulich wrote: > On 23.03.2020 10:41, Hongyan Xia wrote: > > --- a/xen/include/asm-x86/page.h > > +++ b/xen/include/asm-x86/page.h > > @@ -196,6 +196,24 @@ static inline l4_pgentry_t > > l4e_from_paddr(paddr_t pa, unsigned int flags) > > #define map_l2t_from_l3e(x) (l2_pgentry_t > > *)map_domain_page(l3e_get_mfn(x)) > > #define map_l3t_from_l4e(x) (l3_pgentry_t > > *)map_domain_page(l4e_get_mfn(x)) > > > > +#define l1e_from_l2e(l2e, off) ({ \ > > + l1_pgentry_t *l1t = map_l1t_from_l2e(l2e); \ > > + l1_pgentry_t l1e = l1t[off]; \ > > + UNMAP_DOMAIN_PAGE(l1t); \ > > + l1e; }) > > + > > +#define l2e_from_l3e(l3e, off) ({ \ > > + l2_pgentry_t *l2t = map_l2t_from_l3e(l3e); \ > > + l2_pgentry_t l2e = l2t[off]; \ > > + UNMAP_DOMAIN_PAGE(l2t); \ > > + l2e; }) > > + > > +#define l3e_from_l4e(l4e, off) ({ \ > > + l3_pgentry_t *l3t = map_l3t_from_l4e(l4e); \ > > + l3_pgentry_t l3e = l3t[off]; \ > > + UNMAP_DOMAIN_PAGE(l3t); \ > > + l3e; }) > > There's a reason these are macros rather than inline functions, > I assume? (This reason would be nice to be stated in the > description.) While converting them into inline functions, I realised I cannot do that due to the header mess. Converting into inline functions needs the domain_page.h header, which opens a can of worms if I include it here (page.h). Keeping them as macros works around this issue. I will add this in the description. Hongyan
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index a440dac25e..2680173fab 100644 --- a/xen/arch/x86/x86_64/mm.c +++ b/xen/arch/x86/x86_64/mm.c @@ -173,14 +173,14 @@ static int share_hotadd_m2p_table(struct mem_hotadd_info *info) v += n << PAGE_SHIFT ) { n = L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES; - l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[ - l3_table_offset(v)]; + l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(v)], + l3_table_offset(v)); if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) ) continue; if ( !(l3e_get_flags(l3e) & _PAGE_PSE) ) { n = L1_PAGETABLE_ENTRIES; - l2e = l3e_to_l2e(l3e)[l2_table_offset(v)]; + l2e = l2e_from_l3e(l3e, l2_table_offset(v)); if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) ) continue; m2p_start_mfn = l2e_get_mfn(l2e); @@ -201,11 +201,11 @@ static int share_hotadd_m2p_table(struct mem_hotadd_info *info) v != RDWR_COMPAT_MPT_VIRT_END; v += 1 << L2_PAGETABLE_SHIFT ) { - l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[ - l3_table_offset(v)]; + l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(v)], + l3_table_offset(v)); if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) ) continue; - l2e = l3e_to_l2e(l3e)[l2_table_offset(v)]; + l2e = l2e_from_l3e(l3e, l2_table_offset(v)); if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) ) continue; m2p_start_mfn = l2e_get_mfn(l2e); diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h index c98d8f5ede..d4752a5925 100644 --- a/xen/include/asm-x86/page.h +++ b/xen/include/asm-x86/page.h @@ -196,6 +196,24 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags) #define map_l2t_from_l3e(x) (l2_pgentry_t *)map_domain_page(l3e_get_mfn(x)) #define map_l3t_from_l4e(x) (l3_pgentry_t *)map_domain_page(l4e_get_mfn(x)) +#define l1e_from_l2e(l2e, off) ({ \ + l1_pgentry_t *l1t = map_l1t_from_l2e(l2e); \ + l1_pgentry_t l1e = l1t[off]; \ + UNMAP_DOMAIN_PAGE(l1t); \ + l1e; }) + +#define l2e_from_l3e(l3e, off) ({ \ + l2_pgentry_t *l2t = map_l2t_from_l3e(l3e); \ + l2_pgentry_t l2e = l2t[off]; \ + UNMAP_DOMAIN_PAGE(l2t); \ + l2e; }) + +#define l3e_from_l4e(l4e, off) ({ \ + l3_pgentry_t *l3t = map_l3t_from_l4e(l4e); \ + l3_pgentry_t l3e = l3t[off]; \ + UNMAP_DOMAIN_PAGE(l3t); \ + l3e; }) + /* Given a virtual address, get an entry offset into a page table. */ #define l1_table_offset(a) \ (((a) >> L1_PAGETABLE_SHIFT) & (L1_PAGETABLE_ENTRIES - 1))