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

[Jan Beulich <jbeulich@xxxxxxxx>]

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