| Message ID | 84a05b9e-a0c3-7860-4a59-a591a873b884@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: fix build with COVERAGE but !HVM | expand |
On 01/02/2021 16:20, Jan Beulich wrote: > Xen is heavily relying on the DCE stage to remove unused code so the > linker doesn't throw an error because a function is not implemented > yet we defined a prototype for it. > > On some GCC versions (such as 9.4 provided by Debian sid), the compiler > DCE stage will not manage to figure that out for > xenmem_add_to_physmap_batch(): > > ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch': > /xen/xen/common/memory.c:942: undefined reference to `xenmem_add_to_physmap_one' > /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': > /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: /xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' isn't defined > ld: final link failed: bad value > > It is not entirely clear why the compiler DCE is not detecting the > unused code. However, cloning the check introduced by the commit below > into xenmem_add_to_physmap_batch() does the trick. > > No functional change intended. > > Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only") > Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Release-Acked-by: Ian Jackson <iwj@xenproject.org> > --- > Julien, since I reused most of your patch'es description, I've kept your > S-o-b. Please let me know if you want me to drop it. > > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -904,6 +904,19 @@ static int xenmem_add_to_physmap_batch(s > { > union add_to_physmap_extra extra = {}; > > + /* > + * While, unlike xenmem_add_to_physmap(), this function is static, there > + * still have been cases observed where xatp_permission_check(), invoked > + * by our caller, doesn't lead to elimination of this entire function when > + * the compile time evaluation of paging_mode_translate(d) is false. Guard > + * against this be replicating the same check here. > + */ Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but I feel this comment can be far more precise/concise. /* In some configurations, (!HVM, COVERAGE), the xenmem_add_to_physmap_one() call doesn't succumb to dead-code-elimination. Duplicate the short-circut from xatp_permission_check() to try and help the compiler out. */ ~Andrew
On Mon, Feb 01, 2021 at 05:20:54PM +0100, Jan Beulich wrote: > Xen is heavily relying on the DCE stage to remove unused code so the > linker doesn't throw an error because a function is not implemented > yet we defined a prototype for it. > > On some GCC versions (such as 9.4 provided by Debian sid), the compiler > DCE stage will not manage to figure that out for > xenmem_add_to_physmap_batch(): > > ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch': > /xen/xen/common/memory.c:942: undefined reference to `xenmem_add_to_physmap_one' > /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': > /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: /xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' isn't defined > ld: final link failed: bad value > > It is not entirely clear why the compiler DCE is not detecting the > unused code. However, cloning the check introduced by the commit below > into xenmem_add_to_physmap_batch() does the trick. > > No functional change intended. > > Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only") > Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Release-Acked-by: Ian Jackson <iwj@xenproject.org> Reviewed-by: Wei Liu <wl@xen.org>
On 01.02.2021 19:26, Andrew Cooper wrote: > On 01/02/2021 16:20, Jan Beulich wrote: >> Xen is heavily relying on the DCE stage to remove unused code so the >> linker doesn't throw an error because a function is not implemented >> yet we defined a prototype for it. >> >> On some GCC versions (such as 9.4 provided by Debian sid), the compiler >> DCE stage will not manage to figure that out for >> xenmem_add_to_physmap_batch(): >> >> ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch': >> /xen/xen/common/memory.c:942: undefined reference to `xenmem_add_to_physmap_one' >> /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': >> /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: /xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' isn't defined >> ld: final link failed: bad value >> >> It is not entirely clear why the compiler DCE is not detecting the >> unused code. However, cloning the check introduced by the commit below >> into xenmem_add_to_physmap_batch() does the trick. >> >> No functional change intended. >> >> Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only") >> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Release-Acked-by: Ian Jackson <iwj@xenproject.org> >> --- >> Julien, since I reused most of your patch'es description, I've kept your >> S-o-b. Please let me know if you want me to drop it. >> >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -904,6 +904,19 @@ static int xenmem_add_to_physmap_batch(s >> { >> union add_to_physmap_extra extra = {}; >> >> + /* >> + * While, unlike xenmem_add_to_physmap(), this function is static, there >> + * still have been cases observed where xatp_permission_check(), invoked >> + * by our caller, doesn't lead to elimination of this entire function when >> + * the compile time evaluation of paging_mode_translate(d) is false. Guard >> + * against this be replicating the same check here. >> + */ > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, Thanks. > but I feel this > comment can be far more precise/concise. > > /* In some configurations, (!HVM, COVERAGE), the > xenmem_add_to_physmap_one() call doesn't succumb to > dead-code-elimination. Duplicate the short-circut from > xatp_permission_check() to try and help the compiler out. */ I'm perfectly fine to take this one. I have to admit though that I first needed to look up "succumb" ... Jan
--- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -904,6 +904,19 @@ static int xenmem_add_to_physmap_batch(s { union add_to_physmap_extra extra = {}; + /* + * While, unlike xenmem_add_to_physmap(), this function is static, there + * still have been cases observed where xatp_permission_check(), invoked + * by our caller, doesn't lead to elimination of this entire function when + * the compile time evaluation of paging_mode_translate(d) is false. Guard + * against this be replicating the same check here. + */ + if ( !paging_mode_translate(d) ) + { + ASSERT_UNREACHABLE(); + return -EACCES; + } + if ( unlikely(xatpb->size < extent) ) return -EILSEQ;
Xen is heavily relying on the DCE stage to remove unused code so the linker doesn't throw an error because a function is not implemented yet we defined a prototype for it. On some GCC versions (such as 9.4 provided by Debian sid), the compiler DCE stage will not manage to figure that out for xenmem_add_to_physmap_batch(): ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch': /xen/xen/common/memory.c:942: undefined reference to `xenmem_add_to_physmap_one' /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': /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: /xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' isn't defined ld: final link failed: bad value It is not entirely clear why the compiler DCE is not detecting the unused code. However, cloning the check introduced by the commit below into xenmem_add_to_physmap_batch() does the trick. No functional change intended. Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only") Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Signed-off-by: Julien Grall <jgrall@amazon.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> Release-Acked-by: Ian Jackson <iwj@xenproject.org> --- Julien, since I reused most of your patch'es description, I've kept your S-o-b. Please let me know if you want me to drop it.