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

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

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 ?

I understand your objectiion above to relate to style or neatness,
rather than function.  Is that correct ?  And that your proposed
additional change would have some impact whilch would have to be
assessed.

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.