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

[Julien Grall <julien@xen.org>]

On 01/02/2021 15:02, Jan Beulich wrote:
> 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@epam.com>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> 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.

Right, I chose this approach over the one discussed previously because 
it doesn't duplicate the check for auto-translated domain and I couldn't 
really find a good way to justify it in the code (you requested one).

Regarding calling the xatp_permission_check() from 
xenmem_add_to_physmap(). It would means that the DM code will have two 
XSM check (one XSM_DM_PRIV, one XSM_TARGET). It is not clear to me 
whether this is going to be introducing more headache for the XSM folks.

Therefore, for Xen 4.15, I would prefer to stick with my patch.

Cheers,

-- 
Julien Grall