1: p2m: tidy p2m_add_foreign() a little 2: mm: p2m_add_foreign() is HVM-only 3: p2m: set_{foreign,mmio}_p2m_entry() are HVM-only 4: p2m: {,un}map_mmio_regions() are HVM-only 5: mm: the gva_to_gfn() hook is HVM-only 6: p2m: set_shared_p2m_entry() is MEM_SHARING-only Jan
Drop a bogus ASSERT() - we don't typically assert incoming domain pointers to be non-NULL, and there's no particular reason to do so here. Replace the open-coded DOMID_SELF check by use of rcu_lock_remote_domain_by_id(), at the same time covering the request being made with the current domain's actual ID. Move the "both domains same" check into just the path where it really is meaningful. Swap the order of the two puts, such that - the p2m lock isn't needlessly held across put_page(), - a separate put_page() on an error path can be avoided, - they're inverse to the order of the respective gets. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- The DOMID_SELF check being converted also suggests to me that there's an implication of tdom == current->domain, which would in turn appear to mean the "both domains same" check could as well be dropped altogether. --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2560,9 +2560,6 @@ int p2m_add_foreign(struct domain *tdom, int rc; struct domain *fdom; - ASSERT(tdom); - if ( foreigndom == DOMID_SELF ) - return -EINVAL; /* * hvm fixme: until support is added to p2m teardown code to cleanup any * foreign entries, limit this to hardware domain only. @@ -2573,13 +2570,15 @@ int p2m_add_foreign(struct domain *tdom, if ( foreigndom == DOMID_XEN ) fdom = rcu_lock_domain(dom_xen); else - fdom = rcu_lock_domain_by_id(foreigndom); - if ( fdom == NULL ) - return -ESRCH; + { + rc = rcu_lock_remote_domain_by_id(foreigndom, &fdom); + if ( rc ) + return rc; - rc = -EINVAL; - if ( tdom == fdom ) - goto out; + rc = -EINVAL; + if ( tdom == fdom ) + goto out; + } rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom); if ( rc ) @@ -2593,10 +2592,8 @@ int p2m_add_foreign(struct domain *tdom, if ( !page || !p2m_is_ram(p2mt) || p2m_is_shared(p2mt) || p2m_is_hole(p2mt) ) { - if ( page ) - put_page(page); rc = -EINVAL; - goto out; + goto put_one; } mfn = page_to_mfn(page); @@ -2625,8 +2622,6 @@ int p2m_add_foreign(struct domain *tdom, gpfn, mfn_x(mfn), fgfn, tdom->domain_id, fdom->domain_id); put_both: - put_page(page); - /* * This put_gfn for the above get_gfn for prev_mfn. We must do this * after set_foreign_p2m_entry so another cpu doesn't populate the gpfn @@ -2634,9 +2629,13 @@ int p2m_add_foreign(struct domain *tdom, */ put_gfn(tdom, gpfn); -out: + put_one: + put_page(page); + + out: if ( fdom ) rcu_unlock_domain(fdom); + return rc; }
On 15/12/2020 16:25, Jan Beulich wrote: > Drop a bogus ASSERT() - we don't typically assert incoming domain > pointers to be non-NULL, and there's no particular reason to do so here. > > Replace the open-coded DOMID_SELF check by use of > rcu_lock_remote_domain_by_id(), at the same time covering the request > being made with the current domain's actual ID. > > Move the "both domains same" check into just the path where it really > is meaningful. > > Swap the order of the two puts, such that > - the p2m lock isn't needlessly held across put_page(), > - a separate put_page() on an error path can be avoided, > - they're inverse to the order of the respective gets. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > The DOMID_SELF check being converted also suggests to me that there's an > implication of tdom == current->domain, which would in turn appear to > mean the "both domains same" check could as well be dropped altogether. I don't see anything conceptually wrong with the toolstack creating a foreign mapping on behalf of a guest at construction time. I'd go as far as to argue that it is an interface shortcoming if this didn't function correctly. > > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2560,9 +2560,6 @@ int p2m_add_foreign(struct domain *tdom, > int rc; > struct domain *fdom; > > - ASSERT(tdom); > - if ( foreigndom == DOMID_SELF ) > - return -EINVAL; > /* > * hvm fixme: until support is added to p2m teardown code to cleanup any > * foreign entries, limit this to hardware domain only. > @@ -2573,13 +2570,15 @@ int p2m_add_foreign(struct domain *tdom, > if ( foreigndom == DOMID_XEN ) > fdom = rcu_lock_domain(dom_xen); > else > - fdom = rcu_lock_domain_by_id(foreigndom); > - if ( fdom == NULL ) > - return -ESRCH; > + { > + rc = rcu_lock_remote_domain_by_id(foreigndom, &fdom); It occurs to me that rcu_lock_remote_domain_by_id()'s self error path ought to be -EINVAL rather than -EPERM. It's never for permissions reasons that we restrict to remote domains like this - always for technical ones. But that is definitely content for a different patch. ~Andrew
On 17.12.2020 20:03, Andrew Cooper wrote: > On 15/12/2020 16:25, Jan Beulich wrote: >> Drop a bogus ASSERT() - we don't typically assert incoming domain >> pointers to be non-NULL, and there's no particular reason to do so here. >> >> Replace the open-coded DOMID_SELF check by use of >> rcu_lock_remote_domain_by_id(), at the same time covering the request >> being made with the current domain's actual ID. >> >> Move the "both domains same" check into just the path where it really >> is meaningful. >> >> Swap the order of the two puts, such that >> - the p2m lock isn't needlessly held across put_page(), >> - a separate put_page() on an error path can be avoided, >> - they're inverse to the order of the respective gets. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. >> --- >> The DOMID_SELF check being converted also suggests to me that there's an >> implication of tdom == current->domain, which would in turn appear to >> mean the "both domains same" check could as well be dropped altogether. > > I don't see anything conceptually wrong with the toolstack creating a > foreign mapping on behalf of a guest at construction time. I'd go as > far as to argue that it is an interface shortcoming if this didn't > function correctly. Right, I actually didn't get the remark right. It's the DOMID_SELF check that's suspicious especially when tdom != current->domain, not the tdom != fdom one. Jan
This is together with its only caller, xenmem_add_to_physmap_one(). Move the latter next to p2m_add_foreign(), allowing this one to become static at the same time. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -118,7 +118,6 @@ #include <xen/vmap.h> #include <xen/xmalloc.h> #include <xen/efi.h> -#include <xen/grant_table.h> #include <xen/hypercall.h> #include <xen/mm.h> #include <asm/paging.h> @@ -142,10 +141,7 @@ #include <asm/pci.h> #include <asm/guest.h> #include <asm/hvm/ioreq.h> - -#include <asm/hvm/grant_table.h> #include <asm/pv/domain.h> -#include <asm/pv/grant_table.h> #include <asm/pv/mm.h> #ifdef CONFIG_PV @@ -4591,114 +4587,6 @@ static int handle_iomem_range(unsigned l return err || s > e ? err : _handle_iomem_range(s, e, p); } -int xenmem_add_to_physmap_one( - struct domain *d, - unsigned int space, - union add_to_physmap_extra extra, - unsigned long idx, - gfn_t gpfn) -{ - struct page_info *page = NULL; - unsigned long gfn = 0 /* gcc ... */, old_gpfn; - mfn_t prev_mfn; - int rc = 0; - mfn_t mfn = INVALID_MFN; - p2m_type_t p2mt; - - switch ( space ) - { - case XENMAPSPACE_shared_info: - if ( idx == 0 ) - mfn = virt_to_mfn(d->shared_info); - break; - case XENMAPSPACE_grant_table: - rc = gnttab_map_frame(d, idx, gpfn, &mfn); - if ( rc ) - return rc; - break; - case XENMAPSPACE_gmfn: - { - p2m_type_t p2mt; - - gfn = idx; - mfn = get_gfn_unshare(d, gfn, &p2mt); - /* If the page is still shared, exit early */ - if ( p2m_is_shared(p2mt) ) - { - put_gfn(d, gfn); - return -ENOMEM; - } - page = get_page_from_mfn(mfn, d); - if ( unlikely(!page) ) - mfn = INVALID_MFN; - break; - } - case XENMAPSPACE_gmfn_foreign: - return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid); - default: - break; - } - - if ( mfn_eq(mfn, INVALID_MFN) ) - { - rc = -EINVAL; - goto put_both; - } - - /* Remove previously mapped page if it was present. */ - prev_mfn = get_gfn(d, gfn_x(gpfn), &p2mt); - if ( mfn_valid(prev_mfn) ) - { - if ( is_special_page(mfn_to_page(prev_mfn)) ) - /* Special pages are simply unhooked from this phys slot. */ - rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); - else if ( !mfn_eq(mfn, prev_mfn) ) - /* Normal domain memory is freed, to avoid leaking memory. */ - rc = guest_remove_page(d, gfn_x(gpfn)); - } - /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ - put_gfn(d, gfn_x(gpfn)); - - if ( rc ) - goto put_both; - - /* Unmap from old location, if any. */ - old_gpfn = get_gpfn_from_mfn(mfn_x(mfn)); - ASSERT(!SHARED_M2P(old_gpfn)); - if ( space == XENMAPSPACE_gmfn && old_gpfn != gfn ) - { - rc = -EXDEV; - goto put_both; - } - if ( old_gpfn != INVALID_M2P_ENTRY ) - rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K); - - /* Map at new location. */ - if ( !rc ) - rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); - - put_both: - /* - * In the XENMAPSPACE_gmfn case, we took a ref of the gfn at the top. - * We also may need to transfer ownership of the page reference to our - * caller. - */ - if ( space == XENMAPSPACE_gmfn ) - { - put_gfn(d, gfn); - if ( !rc && extra.ppage ) - { - *extra.ppage = page; - page = NULL; - } - } - - if ( page ) - put_page(page); - - return rc; -} - int arch_acquire_resource(struct domain *d, unsigned int type, unsigned int id, unsigned long frame, unsigned int nr_frames, xen_pfn_t mfn_list[]) --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -27,6 +27,7 @@ #include <xen/mem_access.h> #include <xen/vm_event.h> #include <xen/event.h> +#include <xen/grant_table.h> #include <xen/param.h> #include <public/vm_event.h> #include <asm/domain.h> @@ -42,6 +43,10 @@ #include "mm-locks.h" +/* Override macro from asm/page.h to make work with mfn_t */ +#undef virt_to_mfn +#define virt_to_mfn(v) _mfn(__virt_to_mfn(v)) + /* Turn on/off host superpage page table support for hap, default on. */ bool_t __initdata opt_hap_1gb = 1, __initdata opt_hap_2mb = 1; boolean_param("hap_1gb", opt_hap_1gb); @@ -2535,6 +2540,8 @@ out_p2m_audit: } #endif /* P2M_AUDIT */ +#ifdef CONFIG_HVM + /* * Add frame from foreign domain to target domain's physmap. Similar to * XENMAPSPACE_gmfn but the frame is foreign being mapped into current, @@ -2551,8 +2558,8 @@ out_p2m_audit: * * Returns: 0 ==> success */ -int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, - unsigned long gpfn, domid_t foreigndom) +static int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, + unsigned long gpfn, domid_t foreigndom) { p2m_type_t p2mt, p2mt_prev; mfn_t prev_mfn, mfn; @@ -2639,7 +2646,114 @@ int p2m_add_foreign(struct domain *tdom, return rc; } -#ifdef CONFIG_HVM +int xenmem_add_to_physmap_one( + struct domain *d, + unsigned int space, + union add_to_physmap_extra extra, + unsigned long idx, + gfn_t gpfn) +{ + struct page_info *page = NULL; + unsigned long gfn = 0 /* gcc ... */, old_gpfn; + mfn_t prev_mfn; + int rc = 0; + mfn_t mfn = INVALID_MFN; + p2m_type_t p2mt; + + switch ( space ) + { + case XENMAPSPACE_shared_info: + if ( idx == 0 ) + mfn = virt_to_mfn(d->shared_info); + break; + case XENMAPSPACE_grant_table: + rc = gnttab_map_frame(d, idx, gpfn, &mfn); + if ( rc ) + return rc; + break; + case XENMAPSPACE_gmfn: + { + p2m_type_t p2mt; + + gfn = idx; + mfn = get_gfn_unshare(d, gfn, &p2mt); + /* If the page is still shared, exit early */ + if ( p2m_is_shared(p2mt) ) + { + put_gfn(d, gfn); + return -ENOMEM; + } + page = get_page_from_mfn(mfn, d); + if ( unlikely(!page) ) + mfn = INVALID_MFN; + break; + } + case XENMAPSPACE_gmfn_foreign: + return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid); + default: + break; + } + + if ( mfn_eq(mfn, INVALID_MFN) ) + { + rc = -EINVAL; + goto put_both; + } + + /* Remove previously mapped page if it was present. */ + prev_mfn = get_gfn(d, gfn_x(gpfn), &p2mt); + if ( mfn_valid(prev_mfn) ) + { + if ( is_special_page(mfn_to_page(prev_mfn)) ) + /* Special pages are simply unhooked from this phys slot. */ + rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); + else if ( !mfn_eq(mfn, prev_mfn) ) + /* Normal domain memory is freed, to avoid leaking memory. */ + rc = guest_remove_page(d, gfn_x(gpfn)); + } + /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */ + put_gfn(d, gfn_x(gpfn)); + + if ( rc ) + goto put_both; + + /* Unmap from old location, if any. */ + old_gpfn = get_gpfn_from_mfn(mfn_x(mfn)); + ASSERT(!SHARED_M2P(old_gpfn)); + if ( space == XENMAPSPACE_gmfn && old_gpfn != gfn ) + { + rc = -EXDEV; + goto put_both; + } + if ( old_gpfn != INVALID_M2P_ENTRY ) + rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K); + + /* Map at new location. */ + if ( !rc ) + rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K); + + put_both: + /* + * In the XENMAPSPACE_gmfn case, we took a ref of the gfn at the top. + * We also may need to transfer ownership of the page reference to our + * caller. + */ + if ( space == XENMAPSPACE_gmfn ) + { + put_gfn(d, gfn); + if ( !rc && extra.ppage ) + { + *extra.ppage = page; + page = NULL; + } + } + + if ( page ) + put_page(page); + + return rc; +} + /* * Set/clear the #VE suppress bit for a page. Only available on VMX. */ @@ -2792,7 +2906,8 @@ int p2m_set_altp2m_view_visibility(struc return rc; } -#endif + +#endif /* CONFIG_HVM */ /* * Local variables: --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -661,10 +661,6 @@ int set_identity_p2m_entry(struct domain p2m_access_t p2ma, unsigned int flag); int clear_identity_p2m_entry(struct domain *d, unsigned long gfn); -/* Add foreign mapping to the guest's p2m table. */ -int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, - unsigned long gpfn, domid_t foreign_domid); - /* * Populate-on-demand */
On 15/12/2020 16:26, Jan Beulich wrote: > This is together with its only caller, xenmem_add_to_physmap_one(). I can't parse this sentence. Perhaps "... as is it's only caller," as a follow-on from the subject sentence. > Move > the latter next to p2m_add_foreign(), allowing this one to become static > at the same time. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, although... > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2639,7 +2646,114 @@ int p2m_add_foreign(struct domain *tdom, > return rc; > } > > -#ifdef CONFIG_HVM > +int xenmem_add_to_physmap_one( > + struct domain *d, > + unsigned int space, > + union add_to_physmap_extra extra, > + unsigned long idx, > + gfn_t gpfn) > +{ > + struct page_info *page = NULL; > + unsigned long gfn = 0 /* gcc ... */, old_gpfn; > + mfn_t prev_mfn; > + int rc = 0; > + mfn_t mfn = INVALID_MFN; > + p2m_type_t p2mt; > + > + switch ( space ) > + { > + case XENMAPSPACE_shared_info: > + if ( idx == 0 ) > + mfn = virt_to_mfn(d->shared_info); > + break; > + case XENMAPSPACE_grant_table: > + rc = gnttab_map_frame(d, idx, gpfn, &mfn); > + if ( rc ) > + return rc; > + break; > + case XENMAPSPACE_gmfn: > + { > + p2m_type_t p2mt; > + > + gfn = idx; > + mfn = get_gfn_unshare(d, gfn, &p2mt); > + /* If the page is still shared, exit early */ > + if ( p2m_is_shared(p2mt) ) > + { > + put_gfn(d, gfn); > + return -ENOMEM; > + } > + page = get_page_from_mfn(mfn, d); > + if ( unlikely(!page) ) > + mfn = INVALID_MFN; > + break; > + } > + case XENMAPSPACE_gmfn_foreign: > + return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid); > + default: > + break; ... seeing as the function is moving wholesale, can we at least correct the indention, to save yet another large churn in the future? (If it were me, I'd go as far as deleting the default case as well.) ~Andrew
On 17.12.2020 20:18, Andrew Cooper wrote: > On 15/12/2020 16:26, Jan Beulich wrote: >> This is together with its only caller, xenmem_add_to_physmap_one(). > > I can't parse this sentence. Perhaps "... as is it's only caller," as a > follow-on from the subject sentence. Yeah, changed along these lines. >> Move >> the latter next to p2m_add_foreign(), allowing this one to become static >> at the same time. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. > , although... > >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -2639,7 +2646,114 @@ int p2m_add_foreign(struct domain *tdom, >> return rc; >> } >> >> -#ifdef CONFIG_HVM >> +int xenmem_add_to_physmap_one( >> + struct domain *d, >> + unsigned int space, >> + union add_to_physmap_extra extra, >> + unsigned long idx, >> + gfn_t gpfn) >> +{ >> + struct page_info *page = NULL; >> + unsigned long gfn = 0 /* gcc ... */, old_gpfn; >> + mfn_t prev_mfn; >> + int rc = 0; >> + mfn_t mfn = INVALID_MFN; >> + p2m_type_t p2mt; >> + >> + switch ( space ) >> + { >> + case XENMAPSPACE_shared_info: >> + if ( idx == 0 ) >> + mfn = virt_to_mfn(d->shared_info); >> + break; >> + case XENMAPSPACE_grant_table: >> + rc = gnttab_map_frame(d, idx, gpfn, &mfn); >> + if ( rc ) >> + return rc; >> + break; >> + case XENMAPSPACE_gmfn: >> + { >> + p2m_type_t p2mt; >> + >> + gfn = idx; >> + mfn = get_gfn_unshare(d, gfn, &p2mt); >> + /* If the page is still shared, exit early */ >> + if ( p2m_is_shared(p2mt) ) >> + { >> + put_gfn(d, gfn); >> + return -ENOMEM; >> + } >> + page = get_page_from_mfn(mfn, d); >> + if ( unlikely(!page) ) >> + mfn = INVALID_MFN; >> + break; >> + } >> + case XENMAPSPACE_gmfn_foreign: >> + return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid); >> + default: >> + break; > > ... seeing as the function is moving wholesale, can we at least correct > the indention, to save yet another large churn in the future? (If it > were me, I'd go as far as deleting the default case as well.) Oh, indeed. I did look for obvious style issues but didn't spot this (still quite obvious) one. I've done so and also added blank lines between the case blocks. Jan
On 17.12.2020 20:18, Andrew Cooper wrote: > On 15/12/2020 16:26, Jan Beulich wrote: >> This is together with its only caller, xenmem_add_to_physmap_one(). > > I can't parse this sentence. Perhaps "... as is it's only caller," as a > follow-on from the subject sentence. > >> Move >> the latter next to p2m_add_foreign(), allowing this one to become static >> at the same time. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> So I had to ask Andrew to revert this (I was already at home when noticing the breakage), as it turned out to break the shim build. The problem is that xenmem_add_to_physmap() is non-static and hence can't be eliminated altogether by the compiler when !HVM. We could make the function conditionally static "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this looks uglier to me than this extra hunk: --- unstable.orig/xen/common/memory.c +++ unstable/xen/common/memory.c @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain union add_to_physmap_extra extra = {}; struct page_info *pages[16]; - ASSERT(paging_mode_translate(d)); + if ( !paging_mode_translate(d) ) + { + ASSERT_UNREACHABLE(); + return -EACCES; + } if ( xatp->space == XENMAPSPACE_gmfn_foreign ) extra.foreign_domid = DOMID_INVALID; Andrew, please let me know whether your ack stands with this (or said alternative) added, or whether you'd prefer me to re-post. Jan
On 21/12/2020 08:10, Jan Beulich wrote: > On 17.12.2020 20:18, Andrew Cooper wrote: >> On 15/12/2020 16:26, Jan Beulich wrote: >>> This is together with its only caller, xenmem_add_to_physmap_one(). >> I can't parse this sentence. Perhaps "... as is it's only caller," as a >> follow-on from the subject sentence. >> >>> Move >>> the latter next to p2m_add_foreign(), allowing this one to become static >>> at the same time. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > So I had to ask Andrew to revert this (I was already at home when > noticing the breakage), as it turned out to break the shim build. > The problem is that xenmem_add_to_physmap() is non-static and > hence can't be eliminated altogether by the compiler when !HVM. > We could make the function conditionally static > "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this > looks uglier to me than this extra hunk: > > --- unstable.orig/xen/common/memory.c > +++ unstable/xen/common/memory.c > @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain > union add_to_physmap_extra extra = {}; > struct page_info *pages[16]; > > - ASSERT(paging_mode_translate(d)); > + if ( !paging_mode_translate(d) ) > + { > + ASSERT_UNREACHABLE(); > + return -EACCES; > + } > > if ( xatp->space == XENMAPSPACE_gmfn_foreign ) > extra.foreign_domid = DOMID_INVALID; > > Andrew, please let me know whether your ack stands with this (or > said alternative) added, or whether you'd prefer me to re-post. Yeah, this is probably neater than the ifdefary. My ack stands. ~Andrew
Hello all. [Sorry for the possible format issues] On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 21/12/2020 08:10, Jan Beulich wrote: > > On 17.12.2020 20:18, Andrew Cooper wrote: > >> On 15/12/2020 16:26, Jan Beulich wrote: > >>> This is together with its only caller, xenmem_add_to_physmap_one(). > >> I can't parse this sentence. Perhaps "... as is it's only caller," as a > >> follow-on from the subject sentence. > >> > >>> Move > >>> the latter next to p2m_add_foreign(), allowing this one to become > static > >>> at the same time. > >>> > >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > > So I had to ask Andrew to revert this (I was already at home when > > noticing the breakage), as it turned out to break the shim build. > > The problem is that xenmem_add_to_physmap() is non-static and > > hence can't be eliminated altogether by the compiler when !HVM. > > We could make the function conditionally static > > "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this > > looks uglier to me than this extra hunk: > > > > --- unstable.orig/xen/common/memory.c > > +++ unstable/xen/common/memory.c > > @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain > > union add_to_physmap_extra extra = {}; > > struct page_info *pages[16]; > > > > - ASSERT(paging_mode_translate(d)); > > + if ( !paging_mode_translate(d) ) > > + { > > + ASSERT_UNREACHABLE(); > > + return -EACCES; > > + } > > > > if ( xatp->space == XENMAPSPACE_gmfn_foreign ) > > extra.foreign_domid = DOMID_INVALID; > > > > Andrew, please let me know whether your ack stands with this (or > > said alternative) added, or whether you'd prefer me to re-post. > > Yeah, this is probably neater than the ifdefary. My ack stands. > > ~Andrew > I might miss something or did incorrect tests, but ... ... trying to build current staging (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is not set) I got the following: /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941: undefined reference to `xenmem_add_to_physmap_one' /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `xenmem_add_to_physmap_one' ld: /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' isn't defined ld: final link failed: Bad value It is worth mentioning that I do not use pvshim_defconfig (I disable HVM support via menuconfig manually before building). -- Regards, Oleksandr Tyshchenko
On 04.01.2021 17:57, Oleksandr Tyshchenko wrote: > Hello all. > > [Sorry for the possible format issues] > > On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com> > wrote: > >> On 21/12/2020 08:10, Jan Beulich wrote: >>> On 17.12.2020 20:18, Andrew Cooper wrote: >>>> On 15/12/2020 16:26, Jan Beulich wrote: >>>>> This is together with its only caller, xenmem_add_to_physmap_one(). >>>> I can't parse this sentence. Perhaps "... as is it's only caller," as a >>>> follow-on from the subject sentence. >>>> >>>>> Move >>>>> the latter next to p2m_add_foreign(), allowing this one to become >> static >>>>> at the same time. >>>>> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> So I had to ask Andrew to revert this (I was already at home when >>> noticing the breakage), as it turned out to break the shim build. >>> The problem is that xenmem_add_to_physmap() is non-static and >>> hence can't be eliminated altogether by the compiler when !HVM. >>> We could make the function conditionally static >>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this >>> looks uglier to me than this extra hunk: >>> >>> --- unstable.orig/xen/common/memory.c >>> +++ unstable/xen/common/memory.c >>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain >>> union add_to_physmap_extra extra = {}; >>> struct page_info *pages[16]; >>> >>> - ASSERT(paging_mode_translate(d)); >>> + if ( !paging_mode_translate(d) ) >>> + { >>> + ASSERT_UNREACHABLE(); >>> + return -EACCES; >>> + } >>> >>> if ( xatp->space == XENMAPSPACE_gmfn_foreign ) >>> extra.foreign_domid = DOMID_INVALID; >>> >>> Andrew, please let me know whether your ack stands with this (or >>> said alternative) added, or whether you'd prefer me to re-post. >> >> Yeah, this is probably neater than the ifdefary. My ack stands. >> >> ~Andrew >> > > I might miss something or did incorrect tests, but ... > ... trying to build current staging > (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is > not set) I got the following: > > /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941: > undefined reference to `xenmem_add_to_physmap_one' > /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391): > relocation truncated to fit: R_X86_64_PC32 against undefined symbol > `xenmem_add_to_physmap_one' > ld: > /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0: > hidden symbol `xenmem_add_to_physmap_one' isn't defined > ld: final link failed: Bad value > > It is worth mentioning that I do not use pvshim_defconfig (I disable HVM > support via menuconfig manually before building). The specific .config may matter. The specific compiler version may also matter. Things work fine for me, both for the shim config and a custom !HVM one, with gcc10. Jan
On 05.01.21 10:48, Jan Beulich wrote: Hi Jan > On 04.01.2021 17:57, Oleksandr Tyshchenko wrote: >> Hello all. >> >> [Sorry for the possible format issues] >> >> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com> >> wrote: >> >>> On 21/12/2020 08:10, Jan Beulich wrote: >>>> On 17.12.2020 20:18, Andrew Cooper wrote: >>>>> On 15/12/2020 16:26, Jan Beulich wrote: >>>>>> This is together with its only caller, xenmem_add_to_physmap_one(). >>>>> I can't parse this sentence. Perhaps "... as is it's only caller," as a >>>>> follow-on from the subject sentence. >>>>> >>>>>> Move >>>>>> the latter next to p2m_add_foreign(), allowing this one to become >>> static >>>>>> at the same time. >>>>>> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> So I had to ask Andrew to revert this (I was already at home when >>>> noticing the breakage), as it turned out to break the shim build. >>>> The problem is that xenmem_add_to_physmap() is non-static and >>>> hence can't be eliminated altogether by the compiler when !HVM. >>>> We could make the function conditionally static >>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this >>>> looks uglier to me than this extra hunk: >>>> >>>> --- unstable.orig/xen/common/memory.c >>>> +++ unstable/xen/common/memory.c >>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain >>>> union add_to_physmap_extra extra = {}; >>>> struct page_info *pages[16]; >>>> >>>> - ASSERT(paging_mode_translate(d)); >>>> + if ( !paging_mode_translate(d) ) >>>> + { >>>> + ASSERT_UNREACHABLE(); >>>> + return -EACCES; >>>> + } >>>> >>>> if ( xatp->space == XENMAPSPACE_gmfn_foreign ) >>>> extra.foreign_domid = DOMID_INVALID; >>>> >>>> Andrew, please let me know whether your ack stands with this (or >>>> said alternative) added, or whether you'd prefer me to re-post. >>> Yeah, this is probably neater than the ifdefary. My ack stands. >>> >>> ~Andrew >>> >> I might miss something or did incorrect tests, but ... >> ... trying to build current staging >> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is >> not set) I got the following: >> >> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941: >> undefined reference to `xenmem_add_to_physmap_one' >> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391): >> relocation truncated to fit: R_X86_64_PC32 against undefined symbol >> `xenmem_add_to_physmap_one' >> ld: >> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0: >> hidden symbol `xenmem_add_to_physmap_one' isn't defined >> ld: final link failed: Bad value >> >> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM >> support via menuconfig manually before building). > The specific .config may matter. The specific compiler version may > also matter. Things work fine for me, both for the shim config and > a custom !HVM one, with gcc10. ok, after updating my a little bit ancient compiler to the latest possible (?) on xenial gcc-9 the build issue had gone away. Sorry for the noise. -- Regards, Oleksandr Tyshchenko
On 08.01.2021 17:38, Oleksandr wrote: > On 05.01.21 10:48, Jan Beulich wrote: >> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote: >>> Hello all. >>> >>> [Sorry for the possible format issues] >>> >>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com> >>> wrote: >>> >>>> On 21/12/2020 08:10, Jan Beulich wrote: >>>>> On 17.12.2020 20:18, Andrew Cooper wrote: >>>>>> On 15/12/2020 16:26, Jan Beulich wrote: >>>>>>> This is together with its only caller, xenmem_add_to_physmap_one(). >>>>>> I can't parse this sentence. Perhaps "... as is it's only caller," as a >>>>>> follow-on from the subject sentence. >>>>>> >>>>>>> Move >>>>>>> the latter next to p2m_add_foreign(), allowing this one to become >>>> static >>>>>>> at the same time. >>>>>>> >>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> So I had to ask Andrew to revert this (I was already at home when >>>>> noticing the breakage), as it turned out to break the shim build. >>>>> The problem is that xenmem_add_to_physmap() is non-static and >>>>> hence can't be eliminated altogether by the compiler when !HVM. >>>>> We could make the function conditionally static >>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this >>>>> looks uglier to me than this extra hunk: >>>>> >>>>> --- unstable.orig/xen/common/memory.c >>>>> +++ unstable/xen/common/memory.c >>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain >>>>> union add_to_physmap_extra extra = {}; >>>>> struct page_info *pages[16]; >>>>> >>>>> - ASSERT(paging_mode_translate(d)); >>>>> + if ( !paging_mode_translate(d) ) >>>>> + { >>>>> + ASSERT_UNREACHABLE(); >>>>> + return -EACCES; >>>>> + } >>>>> >>>>> if ( xatp->space == XENMAPSPACE_gmfn_foreign ) >>>>> extra.foreign_domid = DOMID_INVALID; >>>>> >>>>> Andrew, please let me know whether your ack stands with this (or >>>>> said alternative) added, or whether you'd prefer me to re-post. >>>> Yeah, this is probably neater than the ifdefary. My ack stands. >>>> >>>> ~Andrew >>>> >>> I might miss something or did incorrect tests, but ... >>> ... trying to build current staging >>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is >>> not set) I got the following: >>> >>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941: >>> undefined reference to `xenmem_add_to_physmap_one' >>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391): >>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol >>> `xenmem_add_to_physmap_one' >>> ld: >>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0: >>> hidden symbol `xenmem_add_to_physmap_one' isn't defined >>> ld: final link failed: Bad value >>> >>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM >>> support via menuconfig manually before building). >> The specific .config may matter. The specific compiler version may >> also matter. Things work fine for me, both for the shim config and >> a custom !HVM one, with gcc10. > > ok, after updating my a little bit ancient compiler to the latest > possible (?) on xenial gcc-9 the build issue had gone away. Sorry for > the noise. There's no reason to be sorry - we want Xen to build with a wide range of compiler versions. It's just that if a build issue is version dependent, it is often up to the person running into it to determine how to address the issue (and submit a patch). Jan
On 08.01.21 19:01, Jan Beulich wrote: Hi Jan > On 08.01.2021 17:38, Oleksandr wrote: >> On 05.01.21 10:48, Jan Beulich wrote: >>> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote: >>>> Hello all. >>>> >>>> [Sorry for the possible format issues] >>>> >>>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com> >>>> wrote: >>>> >>>>> On 21/12/2020 08:10, Jan Beulich wrote: >>>>>> On 17.12.2020 20:18, Andrew Cooper wrote: >>>>>>> On 15/12/2020 16:26, Jan Beulich wrote: >>>>>>>> This is together with its only caller, xenmem_add_to_physmap_one(). >>>>>>> I can't parse this sentence. Perhaps "... as is it's only caller," as a >>>>>>> follow-on from the subject sentence. >>>>>>> >>>>>>>> Move >>>>>>>> the latter next to p2m_add_foreign(), allowing this one to become >>>>> static >>>>>>>> at the same time. >>>>>>>> >>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>> So I had to ask Andrew to revert this (I was already at home when >>>>>> noticing the breakage), as it turned out to break the shim build. >>>>>> The problem is that xenmem_add_to_physmap() is non-static and >>>>>> hence can't be eliminated altogether by the compiler when !HVM. >>>>>> We could make the function conditionally static >>>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this >>>>>> looks uglier to me than this extra hunk: >>>>>> >>>>>> --- unstable.orig/xen/common/memory.c >>>>>> +++ unstable/xen/common/memory.c >>>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain >>>>>> union add_to_physmap_extra extra = {}; >>>>>> struct page_info *pages[16]; >>>>>> >>>>>> - ASSERT(paging_mode_translate(d)); >>>>>> + if ( !paging_mode_translate(d) ) >>>>>> + { >>>>>> + ASSERT_UNREACHABLE(); >>>>>> + return -EACCES; >>>>>> + } >>>>>> >>>>>> if ( xatp->space == XENMAPSPACE_gmfn_foreign ) >>>>>> extra.foreign_domid = DOMID_INVALID; >>>>>> >>>>>> Andrew, please let me know whether your ack stands with this (or >>>>>> said alternative) added, or whether you'd prefer me to re-post. >>>>> Yeah, this is probably neater than the ifdefary. My ack stands. >>>>> >>>>> ~Andrew >>>>> >>>> I might miss something or did incorrect tests, but ... >>>> ... trying to build current staging >>>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is >>>> not set) I got the following: >>>> >>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941: >>>> undefined reference to `xenmem_add_to_physmap_one' >>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391): >>>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol >>>> `xenmem_add_to_physmap_one' >>>> ld: >>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0: >>>> hidden symbol `xenmem_add_to_physmap_one' isn't defined >>>> ld: final link failed: Bad value >>>> >>>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM >>>> support via menuconfig manually before building). >>> The specific .config may matter. The specific compiler version may >>> also matter. Things work fine for me, both for the shim config and >>> a custom !HVM one, with gcc10. >> ok, after updating my a little bit ancient compiler to the latest >> possible (?) on xenial gcc-9 the build issue had gone away. Sorry for >> the noise. > There's no reason to be sorry - we want Xen to build with a wide > range of compiler versions. It's just that if a build issue is > version dependent, it is often up to the person running into it > to determine how to address the issue (and submit a patch). ok, the issue was observed with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) 5.4.0 20160609 I think (but not 100% sure), to address the build issue something like the stub below could help: diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 85a8df9..ed35352 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -609,9 +609,19 @@ union add_to_physmap_extra { domid_t foreign_domid; }; +#ifdef CONFIG_HVM int xenmem_add_to_physmap_one(struct domain *d, unsigned int space, union add_to_physmap_extra extra, unsigned long idx, gfn_t gfn); +#else +static inline int xenmem_add_to_physmap_one(struct domain *d, + unsigned int space, + union add_to_physmap_extra extra, + unsigned long idx, gfn_t gfn) +{ + return -EACCES; +} +#endif int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, unsigned int start); -- Regards, Oleksandr Tyshchenko
On 08.01.2021 18:37, Oleksandr wrote: > > On 08.01.21 19:01, Jan Beulich wrote: > > Hi Jan > >> On 08.01.2021 17:38, Oleksandr wrote: >>> On 05.01.21 10:48, Jan Beulich wrote: >>>> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote: >>>>> Hello all. >>>>> >>>>> [Sorry for the possible format issues] >>>>> >>>>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com> >>>>> wrote: >>>>> >>>>>> On 21/12/2020 08:10, Jan Beulich wrote: >>>>>>> On 17.12.2020 20:18, Andrew Cooper wrote: >>>>>>>> On 15/12/2020 16:26, Jan Beulich wrote: >>>>>>>>> This is together with its only caller, xenmem_add_to_physmap_one(). >>>>>>>> I can't parse this sentence. Perhaps "... as is it's only caller," as a >>>>>>>> follow-on from the subject sentence. >>>>>>>> >>>>>>>>> Move >>>>>>>>> the latter next to p2m_add_foreign(), allowing this one to become >>>>>> static >>>>>>>>> at the same time. >>>>>>>>> >>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>> So I had to ask Andrew to revert this (I was already at home when >>>>>>> noticing the breakage), as it turned out to break the shim build. >>>>>>> The problem is that xenmem_add_to_physmap() is non-static and >>>>>>> hence can't be eliminated altogether by the compiler when !HVM. >>>>>>> We could make the function conditionally static >>>>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this >>>>>>> looks uglier to me than this extra hunk: >>>>>>> >>>>>>> --- unstable.orig/xen/common/memory.c >>>>>>> +++ unstable/xen/common/memory.c >>>>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain >>>>>>> union add_to_physmap_extra extra = {}; >>>>>>> struct page_info *pages[16]; >>>>>>> >>>>>>> - ASSERT(paging_mode_translate(d)); >>>>>>> + if ( !paging_mode_translate(d) ) >>>>>>> + { >>>>>>> + ASSERT_UNREACHABLE(); >>>>>>> + return -EACCES; >>>>>>> + } >>>>>>> >>>>>>> if ( xatp->space == XENMAPSPACE_gmfn_foreign ) >>>>>>> extra.foreign_domid = DOMID_INVALID; >>>>>>> >>>>>>> Andrew, please let me know whether your ack stands with this (or >>>>>>> said alternative) added, or whether you'd prefer me to re-post. >>>>>> Yeah, this is probably neater than the ifdefary. My ack stands. >>>>>> >>>>>> ~Andrew >>>>>> >>>>> I might miss something or did incorrect tests, but ... >>>>> ... trying to build current staging >>>>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is >>>>> not set) I got the following: >>>>> >>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941: >>>>> undefined reference to `xenmem_add_to_physmap_one' >>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391): >>>>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol >>>>> `xenmem_add_to_physmap_one' >>>>> ld: >>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0: >>>>> hidden symbol `xenmem_add_to_physmap_one' isn't defined >>>>> ld: final link failed: Bad value >>>>> >>>>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM >>>>> support via menuconfig manually before building). >>>> The specific .config may matter. The specific compiler version may >>>> also matter. Things work fine for me, both for the shim config and >>>> a custom !HVM one, with gcc10. >>> ok, after updating my a little bit ancient compiler to the latest >>> possible (?) on xenial gcc-9 the build issue had gone away. Sorry for >>> the noise. >> There's no reason to be sorry - we want Xen to build with a wide >> range of compiler versions. It's just that if a build issue is >> version dependent, it is often up to the person running into it >> to determine how to address the issue (and submit a patch). > > ok, the issue was observed with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) > 5.4.0 20160609 > > I think (but not 100% sure), to address the build issue something like > the stub below could help: I'm sure this would help, but personally I'd prefer if we could limit the number of such stubs and rely on the compiler DCE-ing the calls. Hence it would be at least desirable (imo necessary) to understand what prevents this to happen here, with this gcc version. If you could also provide your exact .config, I could see whether I can repro here with some of the gcc5 versions I have laying around. Jan
On 11.01.21 09:41, Jan Beulich wrote: Hi Jan > On 08.01.2021 18:37, Oleksandr wrote: >> On 08.01.21 19:01, Jan Beulich wrote: >> >> Hi Jan >> >>> On 08.01.2021 17:38, Oleksandr wrote: >>>> On 05.01.21 10:48, Jan Beulich wrote: >>>>> On 04.01.2021 17:57, Oleksandr Tyshchenko wrote: >>>>>> Hello all. >>>>>> >>>>>> [Sorry for the possible format issues] >>>>>> >>>>>> On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com> >>>>>> wrote: >>>>>> >>>>>>> On 21/12/2020 08:10, Jan Beulich wrote: >>>>>>>> On 17.12.2020 20:18, Andrew Cooper wrote: >>>>>>>>> On 15/12/2020 16:26, Jan Beulich wrote: >>>>>>>>>> This is together with its only caller, xenmem_add_to_physmap_one(). >>>>>>>>> I can't parse this sentence. Perhaps "... as is it's only caller," as a >>>>>>>>> follow-on from the subject sentence. >>>>>>>>> >>>>>>>>>> Move >>>>>>>>>> the latter next to p2m_add_foreign(), allowing this one to become >>>>>>> static >>>>>>>>>> at the same time. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>>> So I had to ask Andrew to revert this (I was already at home when >>>>>>>> noticing the breakage), as it turned out to break the shim build. >>>>>>>> The problem is that xenmem_add_to_physmap() is non-static and >>>>>>>> hence can't be eliminated altogether by the compiler when !HVM. >>>>>>>> We could make the function conditionally static >>>>>>>> "#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this >>>>>>>> looks uglier to me than this extra hunk: >>>>>>>> >>>>>>>> --- unstable.orig/xen/common/memory.c >>>>>>>> +++ unstable/xen/common/memory.c >>>>>>>> @@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain >>>>>>>> union add_to_physmap_extra extra = {}; >>>>>>>> struct page_info *pages[16]; >>>>>>>> >>>>>>>> - ASSERT(paging_mode_translate(d)); >>>>>>>> + if ( !paging_mode_translate(d) ) >>>>>>>> + { >>>>>>>> + ASSERT_UNREACHABLE(); >>>>>>>> + return -EACCES; >>>>>>>> + } >>>>>>>> >>>>>>>> if ( xatp->space == XENMAPSPACE_gmfn_foreign ) >>>>>>>> extra.foreign_domid = DOMID_INVALID; >>>>>>>> >>>>>>>> Andrew, please let me know whether your ack stands with this (or >>>>>>>> said alternative) added, or whether you'd prefer me to re-post. >>>>>>> Yeah, this is probably neater than the ifdefary. My ack stands. >>>>>>> >>>>>>> ~Andrew >>>>>>> >>>>>> I might miss something or did incorrect tests, but ... >>>>>> ... trying to build current staging >>>>>> (7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is >>>>>> not set) I got the following: >>>>>> >>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941: >>>>>> undefined reference to `xenmem_add_to_physmap_one' >>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391): >>>>>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol >>>>>> `xenmem_add_to_physmap_one' >>>>>> ld: >>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0: >>>>>> hidden symbol `xenmem_add_to_physmap_one' isn't defined >>>>>> ld: final link failed: Bad value >>>>>> >>>>>> It is worth mentioning that I do not use pvshim_defconfig (I disable HVM >>>>>> support via menuconfig manually before building). >>>>> The specific .config may matter. The specific compiler version may >>>>> also matter. Things work fine for me, both for the shim config and >>>>> a custom !HVM one, with gcc10. >>>> ok, after updating my a little bit ancient compiler to the latest >>>> possible (?) on xenial gcc-9 the build issue had gone away. Sorry for >>>> the noise. >>> There's no reason to be sorry - we want Xen to build with a wide >>> range of compiler versions. It's just that if a build issue is >>> version dependent, it is often up to the person running into it >>> to determine how to address the issue (and submit a patch). >> ok, the issue was observed with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) >> 5.4.0 20160609 >> >> I think (but not 100% sure), to address the build issue something like >> the stub below could help: > I'm sure this would help, but personally I'd prefer if we could limit > the number of such stubs and rely on the compiler DCE-ing the calls. > Hence it would be at least desirable (imo necessary) to understand > what prevents this to happen here, with this gcc version. Sounds reasonable > > If you could also provide your exact .config, I could see whether I > can repro here with some of the gcc5 versions I have laying around. Please see attached -- Regards, Oleksandr Tyshchenko # # Automatically generated file; DO NOT EDIT. # Xen/x86 4.15-unstable Configuration # CONFIG_CC_IS_GCC=y CONFIG_GCC_VERSION=50400 CONFIG_CLANG_VERSION=0 CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE=y CONFIG_X86_64=y CONFIG_X86=y CONFIG_ARCH_DEFCONFIG="arch/x86/configs/x86_64_defconfig" CONFIG_INDIRECT_THUNK=y # # Architecture Features # CONFIG_NR_CPUS=256 CONFIG_PV=y CONFIG_PV32=y CONFIG_PV_LINEAR_PT=y # CONFIG_HVM is not set CONFIG_SHADOW_PAGING=y # CONFIG_BIGMEM is not set CONFIG_TBOOT=y CONFIG_XEN_ALIGN_DEFAULT=y # CONFIG_XEN_ALIGN_2M is not set # CONFIG_XEN_GUEST is not set # CONFIG_HYPERV_GUEST is not set # end of Architecture Features # # Common Features # CONFIG_COMPAT=y CONFIG_CORE_PARKING=y CONFIG_GRANT_TABLE=y CONFIG_HAS_ALTERNATIVE=y CONFIG_HAS_EX_TABLE=y CONFIG_HAS_FAST_MULTIPLY=y CONFIG_HAS_IOPORTS=y CONFIG_HAS_KEXEC=y CONFIG_HAS_MEM_PAGING=y CONFIG_HAS_PDX=y CONFIG_HAS_SCHED_GRANULARITY=y CONFIG_HAS_UBSAN=y CONFIG_MEM_ACCESS_ALWAYS_ON=y CONFIG_MEM_ACCESS=y CONFIG_NEEDS_LIBELF=y # # Speculative hardening # CONFIG_SPECULATIVE_HARDEN_ARRAY=y CONFIG_SPECULATIVE_HARDEN_BRANCH=y # end of Speculative hardening CONFIG_HYPFS=y CONFIG_HYPFS_CONFIG=y CONFIG_KEXEC=y CONFIG_XENOPROF=y # CONFIG_XSM is not set CONFIG_SCHED_CREDIT=y CONFIG_SCHED_CREDIT2=y CONFIG_SCHED_RTDS=y CONFIG_SCHED_ARINC653=y CONFIG_SCHED_NULL=y CONFIG_SCHED_DEFAULT="credit2" CONFIG_CRYPTO=y CONFIG_LIVEPATCH=y CONFIG_FAST_SYMBOL_LOOKUP=y CONFIG_ENFORCE_UNIQUE_SYMBOLS=y CONFIG_CMDLINE="" CONFIG_DOM0_MEM="" CONFIG_TRACEBUFFER=y # end of Common Features # # Device Drivers # CONFIG_ACPI=y CONFIG_ACPI_LEGACY_TABLES_LOOKUP=y CONFIG_NUMA=y CONFIG_HAS_NS16550=y CONFIG_HAS_EHCI=y CONFIG_HAS_CPUFREQ=y CONFIG_HAS_PASSTHROUGH=y CONFIG_HAS_PCI=y CONFIG_VIDEO=y CONFIG_VGA=y # end of Device Drivers # CONFIG_EXPERT is not set CONFIG_ARCH_SUPPORTS_INT128=y # # Debugging Options # CONFIG_DEBUG=y # CONFIG_CRASH_DEBUG is not set CONFIG_GDBSX=y CONFIG_DEBUG_INFO=y CONFIG_FRAME_POINTER=y # CONFIG_DEBUG_LOCK_PROFILE is not set CONFIG_DEBUG_LOCKS=y # CONFIG_PERF_COUNTERS is not set CONFIG_VERBOSE_DEBUG=y CONFIG_SCRUB_DEBUG=y # CONFIG_UBSAN is not set # CONFIG_DEBUG_TRACE is not set CONFIG_XMEM_POOL_POISON=y # end of Debugging Options
On 11.01.2021 09:23, Oleksandr wrote: > On 11.01.21 09:41, Jan Beulich wrote: >> If you could also provide your exact .config, I could see whether I >> can repro here with some of the gcc5 versions I have laying around. > > Please see attached Builds perfectly fine with 5.4.0 here. Jan
On 12.01.21 13:58, Jan Beulich wrote: Hi Jan. > On 11.01.2021 09:23, Oleksandr wrote: >> On 11.01.21 09:41, Jan Beulich wrote: >>> If you could also provide your exact .config, I could see whether I >>> can repro here with some of the gcc5 versions I have laying around. >> Please see attached > Builds perfectly fine with 5.4.0 here. Thank you for testing. I wonder whether I indeed missed something. I have switched to 5.4.0 again (from 9.3.0) and rechecked, a build issue was still present. I even downloaded 5.4.0 sources and built them to try to build Xen, and got the same effect. What I noticed is that for non-debug builds the build issue wasn't present. Then I decided to build today's staging (414be7b66349e7dca42bc1fd47c2b2f5b2d27432 xen/memory: Fix compat XENMEM_acquire_resource for size requests) instead of 9-day's old one when I had initially reported about that build issue (7ba2ab495be54f608cb47440e1497b2795bd301a x86/p2m: Fix paging_gva_to_gfn() for nested virt). Today's staging builds perfectly fine with 5.4.0. It seems that commit in the middle (994f6478a48a60e3b407c7defc2d36a80f880b04 xsm/dummy: harden against speculative abuse) indirectly fixes that weird build issue with 5.4.0... -- Regards, Oleksandr Tyshchenko
Hi Jan & Oleksandr, On 13/01/2021 15:06, Oleksandr wrote: > > On 12.01.21 13:58, Jan Beulich wrote: > > Hi Jan. > >> On 11.01.2021 09:23, Oleksandr wrote: >>> On 11.01.21 09:41, Jan Beulich wrote: >>>> If you could also provide your exact .config, I could see whether I >>>> can repro here with some of the gcc5 versions I have laying around. >>> Please see attached >> Builds perfectly fine with 5.4.0 here. > > Thank you for testing. > > > I wonder whether I indeed missed something. I have switched to 5.4.0 > again (from 9.3.0) and rechecked, a build issue was still present. > I even downloaded 5.4.0 sources and built them to try to build Xen, and > got the same effect. What I noticed is that for non-debug builds the > build issue wasn't present. > Then I decided to build today's staging > (414be7b66349e7dca42bc1fd47c2b2f5b2d27432 xen/memory: Fix compat > XENMEM_acquire_resource for size requests) instead of 9-day's old one when > I had initially reported about that build issue > (7ba2ab495be54f608cb47440e1497b2795bd301a x86/p2m: Fix > paging_gva_to_gfn() for nested virt). Today's staging builds perfectly > fine with 5.4.0. > It seems that commit in the middle > (994f6478a48a60e3b407c7defc2d36a80f880b04 xsm/dummy: harden against > speculative abuse) indirectly fixes that weird build issue with 5.4.0... The gitlab CI reported a similar issue today (see [1]) when building with randconfig ([2]). This is happening on Debian sid with GCC 9.3. Note that the default compiler on sid is GCC 10.2.1. So you will have to install the package gcc-9 and then use CC=gcc-9 make <...>. From a local repro, I get the following message: ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch': /root/xen/xen/common/memory.c:942: undefined reference to `xenmem_add_to_physmap_one' /root/xen/xen/common/memory.c:942:(.text+0x22145): relocation truncated to fit: R_X86_64_PLT32 against undefined symbol `xenmem_add_to_physmap_one' prelink-efi.o: in function `xenmem_add_to_physmap_batch': /root/xen/xen/common/memory.c:942: undefined reference to `xenmem_add_to_physmap_one' make[2]: *** [Makefile:215: /root/xen/xen/xen.efi] Error 1 make[2]: *** Waiting for unfinished jobs.... ld: /root/xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' isn't defined ld: final link failed: bad value This points to the call in xenmem_add_to_physmap_batch(). I have played a bit with the .config options. I was able to get it built as soon as I disabled CONFIG_COVERAGE=y. So maybe the optimizer is not clever enough on GCC 9 when building with coverage enabled? With the diff below applied (borrowed from xenmem_add_to_physmap_batch()), I can build without tweaking the .config [1]: diff --git a/xen/common/memory.c b/xen/common/memory.c index ccb4d49fc6..5cfd36a53d 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -903,6 +903,12 @@ static int xenmem_add_to_physmap_batch(struct domain *d, { union add_to_physmap_extra extra = {}; + if ( !paging_mode_translate(d) ) + { + ASSERT_UNREACHABLE(); + return -EACCES; + } + if ( unlikely(xatpb->size < extent) ) return -EILSEQ; Cheers, [1] https://gitlab.com/xen-project/xen/-/jobs/981624525 [2] https://pastebin.com/vTbQXXV9 https://gitlab.com/xen-project/xen/-/jobs/981624525 > > -- Julien Grall
On 23.01.2021 14:22, Julien Grall wrote: > On 13/01/2021 15:06, Oleksandr wrote: >> On 12.01.21 13:58, Jan Beulich wrote: >>> On 11.01.2021 09:23, Oleksandr wrote: >>>> On 11.01.21 09:41, Jan Beulich wrote: >>>>> If you could also provide your exact .config, I could see whether I >>>>> can repro here with some of the gcc5 versions I have laying around. >>>> Please see attached >>> Builds perfectly fine with 5.4.0 here. >> >> Thank you for testing. >> >> >> I wonder whether I indeed missed something. I have switched to 5.4.0 >> again (from 9.3.0) and rechecked, a build issue was still present. >> I even downloaded 5.4.0 sources and built them to try to build Xen, and >> got the same effect. What I noticed is that for non-debug builds the >> build issue wasn't present. >> Then I decided to build today's staging >> (414be7b66349e7dca42bc1fd47c2b2f5b2d27432 xen/memory: Fix compat >> XENMEM_acquire_resource for size requests) instead of 9-day's old one when >> I had initially reported about that build issue >> (7ba2ab495be54f608cb47440e1497b2795bd301a x86/p2m: Fix >> paging_gva_to_gfn() for nested virt). Today's staging builds perfectly >> fine with 5.4.0. >> It seems that commit in the middle >> (994f6478a48a60e3b407c7defc2d36a80f880b04 xsm/dummy: harden against >> speculative abuse) indirectly fixes that weird build issue with 5.4.0... > > The gitlab CI reported a similar issue today (see [1]) when building > with randconfig ([2]). This is happening on Debian sid with GCC 9.3. > > Note that the default compiler on sid is GCC 10.2.1. So you will have to > install the package gcc-9 and then use CC=gcc-9 make <...>. > > > From a local repro, I get the following message: > > ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch': > /root/xen/xen/common/memory.c:942: undefined reference to > `xenmem_add_to_physmap_one' > /root/xen/xen/common/memory.c:942:(.text+0x22145): relocation truncated > to fit: R_X86_64_PLT32 against undefined symbol `xenmem_add_to_physmap_one' > prelink-efi.o: in function `xenmem_add_to_physmap_batch': > /root/xen/xen/common/memory.c:942: undefined reference to > `xenmem_add_to_physmap_one' > make[2]: *** [Makefile:215: /root/xen/xen/xen.efi] Error 1 > make[2]: *** Waiting for unfinished jobs.... > ld: /root/xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' > isn't defined > ld: final link failed: bad value > > > This points to the call in xenmem_add_to_physmap_batch(). I have played > a bit with the .config options. I was able to get it built as soon as I > disabled CONFIG_COVERAGE=y. > > So maybe the optimizer is not clever enough on GCC 9 when building with > coverage enabled? > > With the diff below applied (borrowed from > xenmem_add_to_physmap_batch()), I can build without tweaking the .config > [1]: > > diff --git a/xen/common/memory.c b/xen/common/memory.c > index ccb4d49fc6..5cfd36a53d 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -903,6 +903,12 @@ static int xenmem_add_to_physmap_batch(struct > domain *d, > { > union add_to_physmap_extra extra = {}; > > + if ( !paging_mode_translate(d) ) > + { > + ASSERT_UNREACHABLE(); > + return -EACCES; > + } > + > if ( unlikely(xatpb->size < extent) ) > return -EILSEQ; Interesting. So despite the function being static and xatp_permission_check() already doing this check, the function doesn't get squashed. Looking at my gcov build this might be due to xatp_permission_check() not getting inlined in this case, but I will need to try one with HVM=n to be certain. In any event, I wouldn't mind the above as a workaround to the issue, as long as its description makes clear this is a workaround only. Thanks for investigating! Jan
On 23.01.2021 14:22, Julien Grall wrote: > On 13/01/2021 15:06, Oleksandr wrote: >> On 12.01.21 13:58, Jan Beulich wrote: >>> On 11.01.2021 09:23, Oleksandr wrote: >>>> On 11.01.21 09:41, Jan Beulich wrote: >>>>> If you could also provide your exact .config, I could see whether I >>>>> can repro here with some of the gcc5 versions I have laying around. >>>> Please see attached >>> Builds perfectly fine with 5.4.0 here. >> >> Thank you for testing. >> >> >> I wonder whether I indeed missed something. I have switched to 5.4.0 >> again (from 9.3.0) and rechecked, a build issue was still present. >> I even downloaded 5.4.0 sources and built them to try to build Xen, and >> got the same effect. What I noticed is that for non-debug builds the >> build issue wasn't present. >> Then I decided to build today's staging >> (414be7b66349e7dca42bc1fd47c2b2f5b2d27432 xen/memory: Fix compat >> XENMEM_acquire_resource for size requests) instead of 9-day's old one when >> I had initially reported about that build issue >> (7ba2ab495be54f608cb47440e1497b2795bd301a x86/p2m: Fix >> paging_gva_to_gfn() for nested virt). Today's staging builds perfectly >> fine with 5.4.0. >> It seems that commit in the middle >> (994f6478a48a60e3b407c7defc2d36a80f880b04 xsm/dummy: harden against >> speculative abuse) indirectly fixes that weird build issue with 5.4.0... > > The gitlab CI reported a similar issue today (see [1]) when building > with randconfig ([2]). This is happening on Debian sid with GCC 9.3. > > Note that the default compiler on sid is GCC 10.2.1. So you will have to > install the package gcc-9 and then use CC=gcc-9 make <...>. > > > From a local repro, I get the following message: > > ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch': > /root/xen/xen/common/memory.c:942: undefined reference to > `xenmem_add_to_physmap_one' > /root/xen/xen/common/memory.c:942:(.text+0x22145): relocation truncated > to fit: R_X86_64_PLT32 against undefined symbol `xenmem_add_to_physmap_one' > prelink-efi.o: in function `xenmem_add_to_physmap_batch': > /root/xen/xen/common/memory.c:942: undefined reference to > `xenmem_add_to_physmap_one' > make[2]: *** [Makefile:215: /root/xen/xen/xen.efi] Error 1 > make[2]: *** Waiting for unfinished jobs.... > ld: /root/xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' > isn't defined > ld: final link failed: bad value > > > This points to the call in xenmem_add_to_physmap_batch(). I have played > a bit with the .config options. I was able to get it built as soon as I > disabled CONFIG_COVERAGE=y. > > So maybe the optimizer is not clever enough on GCC 9 when building with > coverage enabled? Possibly, albeit I can't repro this locally. Even with gcc9 the code gets collapsed sufficiently. I do notice though that overall gcc10 does quite a bit better a job, so I could see further factors potentially leading to what you did observe, and then possibly independent of the specific gcc9 build in use. Jan
Extend a respective #ifdef from inside set_typed_p2m_entry() to around all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety check path. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1257,6 +1257,8 @@ int p2m_finish_type_change(struct domain return rc; } +#ifdef CONFIG_HVM + /* * Returns: * 0 for success @@ -1277,7 +1279,10 @@ static int set_typed_p2m_entry(struct do struct p2m_domain *p2m = p2m_get_hostp2m(d); if ( !paging_mode_translate(d) ) + { + ASSERT_UNREACHABLE(); return -EIO; + } gfn_lock(p2m, gfn, order); omfn = p2m->get_entry(p2m, gfn, &ot, &a, 0, &cur_order, NULL); @@ -1308,7 +1313,6 @@ static int set_typed_p2m_entry(struct do if ( rc ) gdprintk(XENLOG_ERR, "p2m_set_entry: %#lx:%u -> %d (0x%"PRI_mfn")\n", gfn_l, order, rc, mfn_x(mfn)); -#ifdef CONFIG_HVM else if ( p2m_is_pod(ot) ) { pod_lock(p2m); @@ -1316,7 +1320,6 @@ static int set_typed_p2m_entry(struct do BUG_ON(p2m->pod.entry_count < 0); pod_unlock(p2m); } -#endif gfn_unlock(p2m, gfn, order); return rc; @@ -1341,6 +1344,8 @@ int set_mmio_p2m_entry(struct domain *d, p2m_get_hostp2m(d)->default_access); } +#endif /* CONFIG_HVM */ + int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l, p2m_access_t p2ma, unsigned int flag) {
On 15/12/2020 16:26, Jan Beulich wrote: > Extend a respective #ifdef from inside set_typed_p2m_entry() to around > all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety > check path. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> As the code currently stands, yes. However, I'm not sure I agree conceptually. The p2m APIs are either a common interface to use, or HVM-specific. PV guests don't actually have a p2m, but some of the APIs are used from common code (e.g. copy_to/from_guest()), and some p2m concepts are special cased as identity for PV (technically paging_mode_translate()), while other concepts, such as foreign/mmio, which do exist for both PV and HVM guests, are handled with totally different API sets for PV and HVM. This is a broken mess of an abstraction. I suspect some of it has to do with PV autotranslate mode in the past, but that doesn't alter the fact that we have a totally undocumented and error prone set of APIs here. Either P2M's should (fully) be the common abstraction (despite not being a real object for PV guests), or they should should be a different set of APIs which is the common abstraction, and P2Ms should move being exclusively for HVM guests. (It's also very obvious by all the CONFIG_X86 ifdefary that we've got arch specifics in our common code, and that is another aspect of the API mess which needs handling.) I'm honestly not sure which of these would be better, but I'm fairly sure that either would be better than what we've currently got. I certainly think it would be better to have a plan for improvement, to guide patches like this. ~Andrew
On 17.12.2020 20:54, Andrew Cooper wrote: > On 15/12/2020 16:26, Jan Beulich wrote: >> Extend a respective #ifdef from inside set_typed_p2m_entry() to around >> all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety >> check path. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > As the code currently stands, yes. However, I'm not sure I agree > conceptually. > > The p2m APIs are either a common interface to use, or HVM-specific. > > PV guests don't actually have a p2m, but some of the APIs are used from > common code (e.g. copy_to/from_guest()), and some p2m concepts are > special cased as identity for PV (technically paging_mode_translate()), > while other concepts, such as foreign/mmio, which do exist for both PV > and HVM guests, are handled with totally different API sets for PV and HVM. > > This is a broken mess of an abstraction. I suspect some of it has to do > with PV autotranslate mode in the past, but that doesn't alter the fact > that we have a totally undocumented and error prone set of APIs here. > > Either P2M's should (fully) be the common abstraction (despite not being > a real object for PV guests), or they should should be a different set > of APIs which is the common abstraction, and P2Ms should move being > exclusively for HVM guests. > > (It's also very obvious by all the CONFIG_X86 ifdefary that we've got > arch specifics in our common code, and that is another aspect of the API > mess which needs handling.) > > I'm honestly not sure which of these would be better, but I'm fairly > sure that either would be better than what we've currently got. I > certainly think it would be better to have a plan for improvement, to > guide patches like this. Well, by the end of this series fairly large parts of p2m.c are inside #ifdef CONFIG_HVM. I would have thought the route is clear - eventually p2m.c should get built only when HVM is enabled. This change is simply getting us one tiny step closer. Otoh, when considering common code, hiding PV specifics inside the p2m functions may turn out better, as else we may need another layer around them (like effectively we already have with e.g. guest_physmap_{add,remove}_page(), which I think would need to move out of p2m.c if that was to become HVM-only as a whole) ... Jan
Mirror the "translated" check the functions do to do_domctl(), allowing the calls to be DCEd by the compiler. Add ASSERT_UNREACHABLE() to the original checks. Also arrange for {set,clear}_mmio_p2m_entry() and {set,clear}_identity_p2m_entry() to respectively live next to each other, such that clear_mmio_p2m_entry() can also be covered by the #ifdef already covering set_mmio_p2m_entry(). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Arguably the original checks, returning success, could also be dropped at this point. --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1344,52 +1344,6 @@ int set_mmio_p2m_entry(struct domain *d, p2m_get_hostp2m(d)->default_access); } -#endif /* CONFIG_HVM */ - -int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l, - p2m_access_t p2ma, unsigned int flag) -{ - p2m_type_t p2mt; - p2m_access_t a; - gfn_t gfn = _gfn(gfn_l); - mfn_t mfn; - struct p2m_domain *p2m = p2m_get_hostp2m(d); - int ret; - - if ( !paging_mode_translate(p2m->domain) ) - { - if ( !is_iommu_enabled(d) ) - return 0; - return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), - 1ul << PAGE_ORDER_4K, - IOMMUF_readable | IOMMUF_writable); - } - - gfn_lock(p2m, gfn, 0); - - mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL); - - if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm ) - ret = p2m_set_entry(p2m, gfn, _mfn(gfn_l), PAGE_ORDER_4K, - p2m_mmio_direct, p2ma); - else if ( mfn_x(mfn) == gfn_l && p2mt == p2m_mmio_direct && a == p2ma ) - ret = 0; - else - { - if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED ) - ret = 0; - else - ret = -EBUSY; - printk(XENLOG_G_WARNING - "Cannot setup identity map d%d:%lx," - " gfn already mapped to %lx.\n", - d->domain_id, gfn_l, mfn_x(mfn)); - } - - gfn_unlock(p2m, gfn, 0); - return ret; -} - /* * Returns: * 0 for success @@ -1439,6 +1393,52 @@ int clear_mmio_p2m_entry(struct domain * return rc; } +#endif /* CONFIG_HVM */ + +int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l, + p2m_access_t p2ma, unsigned int flag) +{ + p2m_type_t p2mt; + p2m_access_t a; + gfn_t gfn = _gfn(gfn_l); + mfn_t mfn; + struct p2m_domain *p2m = p2m_get_hostp2m(d); + int ret; + + if ( !paging_mode_translate(p2m->domain) ) + { + if ( !is_iommu_enabled(d) ) + return 0; + return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), + 1ul << PAGE_ORDER_4K, + IOMMUF_readable | IOMMUF_writable); + } + + gfn_lock(p2m, gfn, 0); + + mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL); + + if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm ) + ret = p2m_set_entry(p2m, gfn, _mfn(gfn_l), PAGE_ORDER_4K, + p2m_mmio_direct, p2ma); + else if ( mfn_x(mfn) == gfn_l && p2mt == p2m_mmio_direct && a == p2ma ) + ret = 0; + else + { + if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED ) + ret = 0; + else + ret = -EBUSY; + printk(XENLOG_G_WARNING + "Cannot setup identity map d%d:%lx," + " gfn already mapped to %lx.\n", + d->domain_id, gfn_l, mfn_x(mfn)); + } + + gfn_unlock(p2m, gfn, 0); + return ret; +} + int clear_identity_p2m_entry(struct domain *d, unsigned long gfn_l) { p2m_type_t p2mt; @@ -1868,6 +1868,8 @@ void *map_domain_gfn(struct p2m_domain * return map_domain_page(*mfn); } +#ifdef CONFIG_HVM + static unsigned int mmio_order(const struct domain *d, unsigned long start_fn, unsigned long nr) { @@ -1908,7 +1910,10 @@ int map_mmio_regions(struct domain *d, unsigned int iter, order; if ( !paging_mode_translate(d) ) + { + ASSERT_UNREACHABLE(); return 0; + } for ( iter = i = 0; i < nr && iter < MAP_MMIO_MAX_ITER; i += 1UL << order, ++iter ) @@ -1940,7 +1945,10 @@ int unmap_mmio_regions(struct domain *d, unsigned int iter, order; if ( !paging_mode_translate(d) ) + { + ASSERT_UNREACHABLE(); return 0; + } for ( iter = i = 0; i < nr && iter < MAP_MMIO_MAX_ITER; i += 1UL << order, ++iter ) @@ -1962,8 +1970,6 @@ int unmap_mmio_regions(struct domain *d, return i == nr ? 0 : i ?: ret; } -#ifdef CONFIG_HVM - int altp2m_get_effective_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn, p2m_type_t *t, p2m_access_t *a, bool prepopulate) --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -750,6 +750,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe if ( ret ) break; + if ( !paging_mode_translate(d) ) + break; + if ( add ) { printk(XENLOG_G_DEBUG
As is the adjacent ga_to_gfn() one as well as paging_gva_to_gfn(). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1772,7 +1772,6 @@ void np2m_schedule(int dir) p2m_unlock(p2m); } } -#endif unsigned long paging_gva_to_gfn(struct vcpu *v, unsigned long va, @@ -1820,6 +1819,8 @@ unsigned long paging_gva_to_gfn(struct v return hostmode->gva_to_gfn(v, hostp2m, va, pfec); } +#endif /* CONFIG_HVM */ + /* * If the map is non-NULL, we leave this function having acquired an extra ref * on mfn_to_page(*mfn). In all cases, *pfec contains appropriate --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3414,6 +3414,7 @@ static bool sh_invlpg(struct vcpu *v, un return true; } +#ifdef CONFIG_HVM static unsigned long sh_gva_to_gfn(struct vcpu *v, struct p2m_domain *p2m, @@ -3447,6 +3448,7 @@ sh_gva_to_gfn(struct vcpu *v, struct p2m return gfn_x(gfn); } +#endif /* CONFIG_HVM */ static inline void sh_update_linear_entries(struct vcpu *v) @@ -4571,7 +4573,9 @@ int sh_audit_l4_table(struct vcpu *v, mf const struct paging_mode sh_paging_mode = { .page_fault = sh_page_fault, .invlpg = sh_invlpg, +#ifdef CONFIG_HVM .gva_to_gfn = sh_gva_to_gfn, +#endif .update_cr3 = sh_update_cr3, .update_paging_modes = shadow_update_paging_modes, .flush_tlb = shadow_flush_tlb, --- a/xen/include/asm-x86/paging.h +++ b/xen/include/asm-x86/paging.h @@ -127,6 +127,7 @@ struct paging_mode { struct cpu_user_regs *regs); bool (*invlpg )(struct vcpu *v, unsigned long linear); +#ifdef CONFIG_HVM unsigned long (*gva_to_gfn )(struct vcpu *v, struct p2m_domain *p2m, unsigned long va, @@ -136,6 +137,7 @@ struct paging_mode { unsigned long cr3, paddr_t ga, uint32_t *pfec, unsigned int *page_order); +#endif void (*update_cr3 )(struct vcpu *v, int do_locking, bool noflush); void (*update_paging_modes )(struct vcpu *v); @@ -286,6 +288,8 @@ unsigned long paging_gva_to_gfn(struct v unsigned long va, uint32_t *pfec); +#ifdef CONFIG_HVM + /* Translate a guest address using a particular CR3 value. This is used * to by nested HAP code, to walk the guest-supplied NPT tables as if * they were pagetables. @@ -304,6 +308,8 @@ static inline unsigned long paging_ga_to page_order); } +#endif /* CONFIG_HVM */ + /* Update all the things that are derived from the guest's CR3. * Called when the guest changes CR3; the caller can then use v->arch.cr3 * as the value to load into the host CR3 to schedule this vcpu */
On 15.12.2020 17:27, Jan Beulich wrote: > As is the adjacent ga_to_gfn() one as well as paging_gva_to_gfn(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1772,7 +1772,6 @@ void np2m_schedule(int dir) > p2m_unlock(p2m); > } > } > -#endif > > unsigned long paging_gva_to_gfn(struct vcpu *v, > unsigned long va, > @@ -1820,6 +1819,8 @@ unsigned long paging_gva_to_gfn(struct v > return hostmode->gva_to_gfn(v, hostp2m, va, pfec); > } > > +#endif /* CONFIG_HVM */ > + > /* > * If the map is non-NULL, we leave this function having acquired an extra ref > * on mfn_to_page(*mfn). In all cases, *pfec contains appropriate > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3414,6 +3414,7 @@ static bool sh_invlpg(struct vcpu *v, un > return true; > } > > +#ifdef CONFIG_HVM > > static unsigned long > sh_gva_to_gfn(struct vcpu *v, struct p2m_domain *p2m, > @@ -3447,6 +3448,7 @@ sh_gva_to_gfn(struct vcpu *v, struct p2m > return gfn_x(gfn); > } > > +#endif /* CONFIG_HVM */ > > static inline void > sh_update_linear_entries(struct vcpu *v) > @@ -4571,7 +4573,9 @@ int sh_audit_l4_table(struct vcpu *v, mf > const struct paging_mode sh_paging_mode = { > .page_fault = sh_page_fault, > .invlpg = sh_invlpg, > +#ifdef CONFIG_HVM > .gva_to_gfn = sh_gva_to_gfn, > +#endif > .update_cr3 = sh_update_cr3, > .update_paging_modes = shadow_update_paging_modes, > .flush_tlb = shadow_flush_tlb, I've noticed (or really the compiler told me) I forgot to also change none.c: --- a/xen/arch/x86/mm/shadow/none.c +++ b/xen/arch/x86/mm/shadow/none.c @@ -43,12 +43,14 @@ static bool _invlpg(struct vcpu *v, unsi return true; } +#ifdef CONFIG_HVM static unsigned long _gva_to_gfn(struct vcpu *v, struct p2m_domain *p2m, unsigned long va, uint32_t *pfec) { ASSERT_UNREACHABLE(); return gfn_x(INVALID_GFN); } +#endif static void _update_cr3(struct vcpu *v, int do_locking, bool noflush) { @@ -63,7 +65,9 @@ static void _update_paging_modes(struct static const struct paging_mode sh_paging_none = { .page_fault = _page_fault, .invlpg = _invlpg, +#ifdef CONFIG_HVM .gva_to_gfn = _gva_to_gfn, +#endif .update_cr3 = _update_cr3, .update_paging_modes = _update_paging_modes, }; Jan > --- a/xen/include/asm-x86/paging.h > +++ b/xen/include/asm-x86/paging.h > @@ -127,6 +127,7 @@ struct paging_mode { > struct cpu_user_regs *regs); > bool (*invlpg )(struct vcpu *v, > unsigned long linear); > +#ifdef CONFIG_HVM > unsigned long (*gva_to_gfn )(struct vcpu *v, > struct p2m_domain *p2m, > unsigned long va, > @@ -136,6 +137,7 @@ struct paging_mode { > unsigned long cr3, > paddr_t ga, uint32_t *pfec, > unsigned int *page_order); > +#endif > void (*update_cr3 )(struct vcpu *v, int do_locking, > bool noflush); > void (*update_paging_modes )(struct vcpu *v); > @@ -286,6 +288,8 @@ unsigned long paging_gva_to_gfn(struct v > unsigned long va, > uint32_t *pfec); > > +#ifdef CONFIG_HVM > + > /* Translate a guest address using a particular CR3 value. This is used > * to by nested HAP code, to walk the guest-supplied NPT tables as if > * they were pagetables. > @@ -304,6 +308,8 @@ static inline unsigned long paging_ga_to > page_order); > } > > +#endif /* CONFIG_HVM */ > + > /* Update all the things that are derived from the guest's CR3. > * Called when the guest changes CR3; the caller can then use v->arch.cr3 > * as the value to load into the host CR3 to schedule this vcpu */ > >
Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1476,6 +1476,8 @@ int clear_identity_p2m_entry(struct doma return ret; } +#ifdef CONFIG_MEM_SHARING + /* Returns: 0 for success, -errno for failure */ int set_shared_p2m_entry(struct domain *d, unsigned long gfn_l, mfn_t mfn) { @@ -1514,7 +1516,10 @@ int set_shared_p2m_entry(struct domain * return rc; } +#endif /* CONFIG_MEM_SHARING */ + #ifdef CONFIG_HVM + static struct p2m_domain * p2m_getlru_nestedp2m(struct domain *d, struct p2m_domain *p2m) {
© 2016 - 2024 Red Hat, Inc.