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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2020 15:08, Roger Pau Monné wrote:
> 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>

Thanks.

>> --- 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.

Without these the entire patch would need dropping, and excluding
domctl / sysctl in general is useful imo (I didn't even go check
whether further code has now ended up being dead, that'll be
incremental work over time). I agree the #ifdef-ary isn't ideal,
and I'd be happy to see some or all of it go away again in favor
of whichever better solution.

Jan