Re: [PATCH v2 7/7] x86: don't include domctl and alike in shim-exclusive builds

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Aug 07, 2020 at 01:35:08PM +0200, Jan Beulich wrote:
> There is no need for platform-wide, system-wide, or per-domain control
> in this case. Hence avoid including this dead code in the build.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

One comment below.

> 
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -23,7 +23,6 @@ obj-$(CONFIG_GDBSX) += debug.o
>  obj-y += delay.o
>  obj-y += desc.o
>  obj-bin-y += dmi_scan.init.o
> -obj-y += domctl.o
>  obj-y += domain.o
>  obj-bin-y += dom0_build.init.o
>  obj-y += domain_page.o
> @@ -51,7 +50,6 @@ obj-y += numa.o
>  obj-y += pci.o
>  obj-y += percpu.o
>  obj-y += physdev.o x86_64/physdev.o
> -obj-y += platform_hypercall.o x86_64/platform_hypercall.o
>  obj-y += psr.o
>  obj-y += setup.o
>  obj-y += shutdown.o
> @@ -60,7 +58,6 @@ obj-y += smpboot.o
>  obj-y += spec_ctrl.o
>  obj-y += srat.o
>  obj-y += string.o
> -obj-y += sysctl.o
>  obj-y += time.o
>  obj-y += trace.o
>  obj-y += traps.o
> @@ -71,6 +68,13 @@ obj-$(CONFIG_TBOOT) += tboot.o
>  obj-y += hpet.o
>  obj-y += vm_event.o
>  obj-y += xstate.o
> +
> +ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
> +obj-y += domctl.o
> +obj-y += platform_hypercall.o x86_64/platform_hypercall.o
> +obj-y += sysctl.o
> +endif
> +
>  extra-y += asm-macros.i
>  
>  ifneq ($(CONFIG_HVM),y)
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -47,6 +47,8 @@
>  /* Per-CPU variable for enforcing the lock ordering */
>  DEFINE_PER_CPU(int, mm_lock_level);
>  
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +
>  /************************************************/
>  /*              LOG DIRTY SUPPORT               */
>  /************************************************/
> @@ -628,6 +630,8 @@ void paging_log_dirty_init(struct domain
>      d->arch.paging.log_dirty.ops = ops;
>  }
>  
> +#endif /* CONFIG_PV_SHIM_EXCLUSIVE */
> +
>  /************************************************/
>  /*           CODE FOR PAGING SUPPORT            */
>  /************************************************/
> @@ -667,7 +671,7 @@ void paging_vcpu_init(struct vcpu *v)
>          shadow_vcpu_init(v);
>  }
>  
> -
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
>  int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
>                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl,
>                    bool_t resuming)
> @@ -788,6 +792,7 @@ long paging_domctl_continuation(XEN_GUES
>  
>      return ret;
>  }
> +#endif /* CONFIG_PV_SHIM_EXCLUSIVE */
>  
>  /* Call when destroying a domain */
>  int paging_teardown(struct domain *d)
> @@ -803,10 +808,12 @@ int paging_teardown(struct domain *d)
>      if ( preempted )
>          return -ERESTART;
>  
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
>      /* clean up log dirty resources. */
>      rc = paging_free_log_dirty_bitmap(d, 0);
>      if ( rc == -ERESTART )
>          return rc;
> +#endif

Adding all this ifndefs make the code worse IMO, is it really that much
of a win in terms of size?

TBH I'm not sure it's worth it.

Thanks, Roger.