Re: [PATCH 2/4] x86/mm: allow for building without shadow mode support

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/01/15 08:11, Jan Beulich wrote:
> Considering the complexity of the code, it seems to be a reasonable
> thing to allow people to disable that code entirely even outside the
> immediate need for this by the next patch.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> There is one XXX being added to the code - while it doesn't look like
> we need to set v->arch.paging.mode in shadow_vcpu_init(), it may be
> deemed more secure to set up a table with stub function pointers.

I would agree that setting up a function pointer table is the safer
course of action to take.

>
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -32,9 +32,13 @@ x86 := y
>  x86_32 := n
>  x86_64 := y
>  
> +shadow-paging ?= y
> +
>  CFLAGS += -mno-red-zone -mno-sse -fpic
>  CFLAGS += -fno-asynchronous-unwind-tables
>  # -fvisibility=hidden reduces -fpic cost, if it's available
>  ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
>  CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
>  endif
> +
> +CFLAGS-$(shadow-paging) += -DCONFIG_SHADOW_PAGING
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -635,16 +635,16 @@ int paging_domain_init(struct domain *d,
>       * don't want to leak any active log-dirty bitmaps */
>      d->arch.paging.log_dirty.top = _mfn(INVALID_MFN);
>  
> -    /* The order of the *_init calls below is important, as the later
> -     * ones may rewrite some common fields.  Shadow pagetables are the
> -     * default... */
> -    shadow_domain_init(d, domcr_flags);
> -
> -    /* ... but we will use hardware assistance if it's available. */
> +    /*
> +     * Shadow pagetables are the default, but we will use
> +     * hardware assistance if it's available and enabled.
> +     */
>      if ( hap_enabled(d) )
>          hap_domain_init(d);
> +    else
> +        rc = shadow_domain_init(d, domcr_flags);
>  
> -    return 0;
> +    return rc;
>  }
>  
>  /* vcpu paging struct initialization goes here */
> @@ -822,12 +822,16 @@ int paging_enable(struct domain *d, u32 
>   * and therefore its pagetables will soon be discarded */
>  void pagetable_dying(struct domain *d, paddr_t gpa)
>  {
> +#ifdef CONFIG_SHADOW_PAGING
>      struct vcpu *v;
>  
>      ASSERT(paging_mode_shadow(d));
>  
>      v = d->vcpu[0];
>      v->arch.paging.mode->shadow.pagetable_dying(v, gpa);
> +#else
> +    BUG();
> +#endif
>  }
>  
>  /* Print paging-assistance info to the console */
> --- a/xen/arch/x86/mm/shadow/Makefile
> +++ b/xen/arch/x86/mm/shadow/Makefile
> @@ -1,4 +1,5 @@
> -obj-$(x86_64) += common.o guest_2.o guest_3.o guest_4.o
> +obj-y                := none.o
> +obj-$(shadow-paging) := common.o guest_2.o guest_3.o guest_4.o
>  

Can this be

ifeq($(shadow-paging),y)
obj-y := common.o guest_2.o guest_3.o guest_4.o
else
obj-y := none.o
endif

Rather than relying on the double := to clobber none.o and prevent a
link failure ?

~Andrew