| Message ID | 4a69a1177f9496ad0e3ea77e9b1d5b802bf83b60.1606994506.git.hongyxia@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/vmap: handle superpages in vmap_to_mfn() | expand |
Apologies. Missing v2 in the title.
On 03.12.2020 12:21, Hongyan Xia wrote: > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -5194,6 +5194,60 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v) > } \ > } while ( false ) > > +/* Translate mapped Xen address to MFN. */ > +mfn_t xen_map_to_mfn(unsigned long va) > +{ > +#define CHECK_MAPPED(cond_) \ > + if ( !(cond_) ) \ > + { \ > + ASSERT_UNREACHABLE(); \ > + ret = INVALID_MFN; \ > + goto out; \ > + } \ This should be coded such that use sites ... > + bool locking = system_state > SYS_STATE_boot; > + unsigned int l2_offset = l2_table_offset(va); > + unsigned int l1_offset = l1_table_offset(va); > + const l3_pgentry_t *pl3e = virt_to_xen_l3e(va); > + const l2_pgentry_t *pl2e = NULL; > + const l1_pgentry_t *pl1e = NULL; > + struct page_info *l3page; > + mfn_t ret; > + > + L3T_INIT(l3page); > + CHECK_MAPPED(pl3e) > + l3page = virt_to_page(pl3e); > + L3T_LOCK(l3page); > + > + CHECK_MAPPED(l3e_get_flags(*pl3e) & _PAGE_PRESENT) ... will properly require a statement-ending semicolon. With additionally the trailing underscore dropped from the macro's parameter name Reviewed-by: Jan Beulich <jbeulich@suse.com> Or wait, > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -578,6 +578,7 @@ mfn_t alloc_xen_pagetable_new(void); > void free_xen_pagetable_new(mfn_t mfn); > > l1_pgentry_t *virt_to_xen_l1e(unsigned long v); > +mfn_t xen_map_to_mfn(unsigned long va); This is now a pretty proper companion of map_page_to_xen(), and hence imo ought to be declared next to that one rather than here. Ultimately Arm may also need to gain an implementation. > --- a/xen/include/asm-x86/page.h > +++ b/xen/include/asm-x86/page.h > @@ -291,7 +291,7 @@ void copy_page_sse2(void *, const void *); > #define pfn_to_paddr(pfn) __pfn_to_paddr(pfn) > #define paddr_to_pfn(pa) __paddr_to_pfn(pa) > #define paddr_to_pdx(pa) pfn_to_pdx(paddr_to_pfn(pa)) > -#define vmap_to_mfn(va) l1e_get_mfn(*virt_to_xen_l1e((unsigned long)(va))) > +#define vmap_to_mfn(va) xen_map_to_mfn((unsigned long)va) You've lost parentheses around va. Jan
On Mon, 2020-12-07 at 11:11 +0100, Jan Beulich wrote: > On 03.12.2020 12:21, Hongyan Xia wrote: > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -5194,6 +5194,60 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long > > v) > > } \ > > } while ( false ) > > > > +/* Translate mapped Xen address to MFN. */ > > +mfn_t xen_map_to_mfn(unsigned long va) > > +{ > > +#define CHECK_MAPPED(cond_) \ > > + if ( !(cond_) ) \ > > + { \ > > + ASSERT_UNREACHABLE(); \ > > + ret = INVALID_MFN; \ > > + goto out; \ > > + } \ > > This should be coded such that use sites ... > > > + bool locking = system_state > SYS_STATE_boot; > > + unsigned int l2_offset = l2_table_offset(va); > > + unsigned int l1_offset = l1_table_offset(va); > > + const l3_pgentry_t *pl3e = virt_to_xen_l3e(va); > > + const l2_pgentry_t *pl2e = NULL; > > + const l1_pgentry_t *pl1e = NULL; > > + struct page_info *l3page; > > + mfn_t ret; > > + > > + L3T_INIT(l3page); > > + CHECK_MAPPED(pl3e) > > + l3page = virt_to_page(pl3e); > > + L3T_LOCK(l3page); > > + > > + CHECK_MAPPED(l3e_get_flags(*pl3e) & _PAGE_PRESENT) > > ... will properly require a statement-ending semicolon. With > additionally the trailing underscore dropped from the macro's > parameter name The immediate solution that came to mind is a do-while construct. Would you be happy with that? > Reviewed-by: Jan Beulich <jbeulich@suse.com> > Thanks. > Or wait, > > > --- a/xen/include/asm-x86/mm.h > > +++ b/xen/include/asm-x86/mm.h > > @@ -578,6 +578,7 @@ mfn_t alloc_xen_pagetable_new(void); > > void free_xen_pagetable_new(mfn_t mfn); > > > > l1_pgentry_t *virt_to_xen_l1e(unsigned long v); > > +mfn_t xen_map_to_mfn(unsigned long va); > > This is now a pretty proper companion of map_page_to_xen(), and > hence imo ought to be declared next to that one rather than here. > Ultimately Arm may also need to gain an implementation. Since map_pages_to_xen() is in the common header, are we okay with having the declaration but not an implementation on the Arm side in this patch? Or do we also want to introduce the Arm implementation in this patch? > > --- a/xen/include/asm-x86/page.h > > +++ b/xen/include/asm-x86/page.h > > @@ -291,7 +291,7 @@ void copy_page_sse2(void *, const void *); > > #define pfn_to_paddr(pfn) __pfn_to_paddr(pfn) > > #define paddr_to_pfn(pa) __paddr_to_pfn(pa) > > #define paddr_to_pdx(pa) pfn_to_pdx(paddr_to_pfn(pa)) > > -#define vmap_to_mfn(va) l1e_get_mfn(*virt_to_xen_l1e((unsigned > > long)(va))) > > +#define vmap_to_mfn(va) xen_map_to_mfn((unsigned long)va) > > You've lost parentheses around va. Will fix. Hongyan
On 07.12.2020 12:54, Hongyan Xia wrote: > On Mon, 2020-12-07 at 11:11 +0100, Jan Beulich wrote: >> On 03.12.2020 12:21, Hongyan Xia wrote: >>> --- a/xen/arch/x86/mm.c >>> +++ b/xen/arch/x86/mm.c >>> @@ -5194,6 +5194,60 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long >>> v) >>> } \ >>> } while ( false ) >>> >>> +/* Translate mapped Xen address to MFN. */ >>> +mfn_t xen_map_to_mfn(unsigned long va) >>> +{ >>> +#define CHECK_MAPPED(cond_) \ >>> + if ( !(cond_) ) \ >>> + { \ >>> + ASSERT_UNREACHABLE(); \ >>> + ret = INVALID_MFN; \ >>> + goto out; \ >>> + } \ >> >> This should be coded such that use sites ... >> >>> + bool locking = system_state > SYS_STATE_boot; >>> + unsigned int l2_offset = l2_table_offset(va); >>> + unsigned int l1_offset = l1_table_offset(va); >>> + const l3_pgentry_t *pl3e = virt_to_xen_l3e(va); >>> + const l2_pgentry_t *pl2e = NULL; >>> + const l1_pgentry_t *pl1e = NULL; >>> + struct page_info *l3page; >>> + mfn_t ret; >>> + >>> + L3T_INIT(l3page); >>> + CHECK_MAPPED(pl3e) >>> + l3page = virt_to_page(pl3e); >>> + L3T_LOCK(l3page); >>> + >>> + CHECK_MAPPED(l3e_get_flags(*pl3e) & _PAGE_PRESENT) >> >> ... will properly require a statement-ending semicolon. With >> additionally the trailing underscore dropped from the macro's >> parameter name > > The immediate solution that came to mind is a do-while construct. Would > you be happy with that? Sure. >>> --- a/xen/include/asm-x86/mm.h >>> +++ b/xen/include/asm-x86/mm.h >>> @@ -578,6 +578,7 @@ mfn_t alloc_xen_pagetable_new(void); >>> void free_xen_pagetable_new(mfn_t mfn); >>> >>> l1_pgentry_t *virt_to_xen_l1e(unsigned long v); >>> +mfn_t xen_map_to_mfn(unsigned long va); >> >> This is now a pretty proper companion of map_page_to_xen(), and >> hence imo ought to be declared next to that one rather than here. >> Ultimately Arm may also need to gain an implementation. > > Since map_pages_to_xen() is in the common header, are we okay with > having the declaration but not an implementation on the Arm side in > this patch? Or do we also want to introduce the Arm implementation in > this patch? Just a declaration is fine imo. If a use in common code appears, it'll still be noticeable at link time that Arm will need to have an implementation added. Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 5a50339284c7..53cc5c6de2e6 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5194,6 +5194,60 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v) } \ } while ( false ) +/* Translate mapped Xen address to MFN. */ +mfn_t xen_map_to_mfn(unsigned long va) +{ +#define CHECK_MAPPED(cond_) \ + if ( !(cond_) ) \ + { \ + ASSERT_UNREACHABLE(); \ + ret = INVALID_MFN; \ + goto out; \ + } \ + + bool locking = system_state > SYS_STATE_boot; + unsigned int l2_offset = l2_table_offset(va); + unsigned int l1_offset = l1_table_offset(va); + const l3_pgentry_t *pl3e = virt_to_xen_l3e(va); + const l2_pgentry_t *pl2e = NULL; + const l1_pgentry_t *pl1e = NULL; + struct page_info *l3page; + mfn_t ret; + + L3T_INIT(l3page); + CHECK_MAPPED(pl3e) + l3page = virt_to_page(pl3e); + L3T_LOCK(l3page); + + CHECK_MAPPED(l3e_get_flags(*pl3e) & _PAGE_PRESENT) + if ( l3e_get_flags(*pl3e) & _PAGE_PSE ) + { + ret = mfn_add(l3e_get_mfn(*pl3e), + (l2_offset << PAGETABLE_ORDER) + l1_offset); + goto out; + } + + pl2e = map_l2t_from_l3e(*pl3e) + l2_offset; + CHECK_MAPPED(l2e_get_flags(*pl2e) & _PAGE_PRESENT) + if ( l2e_get_flags(*pl2e) & _PAGE_PSE ) + { + ret = mfn_add(l2e_get_mfn(*pl2e), l1_offset); + goto out; + } + + pl1e = map_l1t_from_l2e(*pl2e) + l1_offset; + CHECK_MAPPED(l1e_get_flags(*pl1e) & _PAGE_PRESENT) + ret = l1e_get_mfn(*pl1e); + +#undef CHECK_MAPPED + out: + L3T_UNLOCK(l3page); + unmap_domain_page(pl1e); + unmap_domain_page(pl2e); + unmap_domain_page(pl3e); + return ret; +} + int map_pages_to_xen( unsigned long virt, mfn_t mfn, diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index deeba75a1cbb..2fc8eeaf7aad 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -578,6 +578,7 @@ mfn_t alloc_xen_pagetable_new(void); void free_xen_pagetable_new(mfn_t mfn); l1_pgentry_t *virt_to_xen_l1e(unsigned long v); +mfn_t xen_map_to_mfn(unsigned long va); int __sync_local_execstate(void); diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h index 7a771baf7cb3..886adf17a40c 100644 --- a/xen/include/asm-x86/page.h +++ b/xen/include/asm-x86/page.h @@ -291,7 +291,7 @@ void copy_page_sse2(void *, const void *); #define pfn_to_paddr(pfn) __pfn_to_paddr(pfn) #define paddr_to_pfn(pa) __paddr_to_pfn(pa) #define paddr_to_pdx(pa) pfn_to_pdx(paddr_to_pfn(pa)) -#define vmap_to_mfn(va) l1e_get_mfn(*virt_to_xen_l1e((unsigned long)(va))) +#define vmap_to_mfn(va) xen_map_to_mfn((unsigned long)va) #define vmap_to_page(va) mfn_to_page(vmap_to_mfn(va)) #endif /* !defined(__ASSEMBLY__) */