Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages]

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.02.2021 15:54, Ian Jackson wrote:
> Julien Grall writes ("[PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n 
> and CONFIG_COVERAGE=y"):
>> 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.
> 
> Thanks for the clear explanation.
> 
>> It is not entirely clear why the compiler DCE is not detecting the
>> unused code. However, moving the permission check from do_memory_op()
>> to xenmem_add_to_physmap_batch() does the trick.
> 
> How unfortunate.
> 
>> Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only")
>> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> I have reviewed the diff, but not the code in context.
> 
>> The gitlab CI is used to provide basic testing on a per-series basis. So
>> I would like to request this patch to be merged in Xen 4.15 in order to
>> reduce the number of failure not related to the series tested.
> 
> Quite so.
> 
> Jan Beulich writes ("Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n 
> and CONFIG_COVERAGE=y"):
>> On 30.01.2021 16:22, Julien Grall wrote:
>>> @@ -1442,13 +1447,6 @@ long do_memory_op(unsigned long cmd, 
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>          if ( d == NULL )
>>>              return -ESRCH;
>>>  
>>> -        rc = xatp_permission_check(d, xatpb.space);
>>> -        if ( rc )
>>> -        {
>>> -            rcu_unlock_domain(d);
>>> -            return rc;
>>> -        }
>>> -
>>>          rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent);
>>>  
>>>          rcu_unlock_domain(d);
>>
>> I'd be okay with the code movement if you did so consistently,
>> i.e. also for the other invocation. I realize this would have
>> an effect on the dm-op call of the function, but I wonder
>> whether this wouldn't even be a good thing. If not, I think
>> duplicating xenmem_add_to_physmap()'s early ASSERT() into
>> xenmem_add_to_physmap_batch() would be the better course of
>> action.
> 
> Jan, can you confirm whether in your opinion this patch as originally
> posted by Julien is *correct* as is ?  In particular, Julien did not
> intend a functional change.  Have you satisfied yourself that there is
> no functional change here ?

Yes and yes.

> I understand your objectiion above to relate to style or neatness,
> rather than function.  Is that correct ?

Yes.

>  And that your proposed
> additional change would have some impact whilch would have to be
> assessed.

The first of the proposed alternatives may need further
investigation, yes. The second of the alternatives would
shrink this patch to a 2-line one, i.e. far less code
churn, and is not in need of any assessment afaia. In
fact I believe this latter alternative was discussed as
the approach to take here, before the patch was submitted.

Jan

> In which case I think it would be better to defer the style
> improvement until after the release.
> 
> IOW, the original patch
> 
> Release-Acked-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> 
> assuming a favourable functional code review from a relevant
> maintainer.
> 
> Thanks,
> Ian.
>