Re: [Xen-devel] [PATCH v4 3/7] x86/mm: put nested p2m code under CONFIG_HVM

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Sep 14, 2018 at 06:50:06AM -0600, Jan Beulich wrote:
> >>> On 13.09.18 at 18:38, <wei.liu2@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1691,7 +1691,9 @@ void context_switch(struct vcpu *prev, struct vcpu 
> > *next)
> >      {
> >          _update_runstate_area(prev);
> >          vpmu_switch_from(prev);
> > +#ifdef CONFIG_HVM
> >          np2m_schedule(NP2M_SCHEDLE_OUT);
> > +#endif
> >      }
> >  
> >      if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
> > @@ -1758,7 +1760,9 @@ void context_switch(struct vcpu *prev, struct vcpu 
> > *next)
> >  
> >          /* Must be done with interrupts enabled */
> >          vpmu_switch_to(next);
> > +#ifdef CONFIG_HVM
> >          np2m_schedule(NP2M_SCHEDLE_IN);
> > +#endif
> >      }
> 
> Instead of these two #ifdef-s a single stub would have been neater imo.

Done.

> 
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -63,23 +63,30 @@ static void p2m_pod_init(struct p2m_domain *p2m)
> >  #endif
> >  }
> >  
> > +static void p2m_nestedp2m_init(struct p2m_domain *p2m)
> > +{
> > +#ifdef CONFIG_HVM
> > +    INIT_LIST_HEAD(&p2m->np2m_list);
> > +
> > +    p2m->np2m_base = P2M_BASE_EADDR;
> > +    p2m->np2m_generation = 0;
> > +#endif
> > +}
> 
> Why don't you follow this placement model ...
> 
> > @@ -151,6 +158,7 @@ static void p2m_teardown_hostp2m(struct domain *d)
> >      }
> >  }
> >  
> > +#ifdef CONFIG_HVM
> >  static void p2m_teardown_nestedp2m(struct domain *d)
> >  {
> >      unsigned int i;
> > @@ -188,6 +196,7 @@ static int p2m_init_nestedp2m(struct domain *d)
> >  
> >      return 0;
> >  }
> > +#endif
> 
> ... here avoiding ...
> 
> > @@ -244,12 +254,15 @@ int p2m_init(struct domain *d)
> >          p2m_teardown_hostp2m(d);
> >          return rc;
> >      }
> > +#endif
> >  
> >      rc = p2m_init_altp2m(d);
> >      if ( rc )
> >      {
> >          p2m_teardown_hostp2m(d);
> > +#ifdef CONFIG_HVM
> >          p2m_teardown_nestedp2m(d);
> > +#endif

This and the later CONFIG_HVM will be expanded in the next patch.

> >      }
> >  
> >      return rc;
> > @@ -701,7 +714,9 @@ void p2m_final_teardown(struct domain *d)
> >       * we initialise them unconditionally.
> >       */
> >      p2m_teardown_altp2m(d);
> > +#ifdef CONFIG_HVM
> >      p2m_teardown_nestedp2m(d);
> > +#endif
> >  
> >      /* Iterate over all p2m tables per domain */
> >      p2m_teardown_hostp2m(d);
> 
> ... two other #ifdef-s?
> 
> Anyway, in the interest of forward progress, preferably with the
> adjustments made
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

Wei.

> 
> Jan
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel