Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Oleksandr <olekstysh@xxxxxxxxx>]

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@xxxxxxxxxx>
> > 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@xxxxxxxx>
> > > > > Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > 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