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

[Tim Deegan <tim@xxxxxxx>]

At 11:10 +0000 on 28 Jan (1422439811), Andrew Cooper wrote:
> 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@xxxxxxxx>
> > ---
> > 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.

+1.

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

+1 to this too, for readability.  Or else define a $(shadow-dummy)
that's set appropriately and use obj-$(shadow-dummy) := none.o ?

Cheers,

Tim

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel