Re: [Xen-devel] [PATCH v3 10/16] x86/mm: put nested p2m code under CONFIG_HVM

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Sep 07, 2018 at 01:06:38AM -0600, Jan Beulich wrote:
> >>> On 04.09.18 at 18:15, <wei.liu2@xxxxxxxxxx> wrote:
> > @@ -149,6 +149,7 @@ static void p2m_teardown_hostp2m(struct domain *d)
> >      }
> >  }
> >  
> > +#ifdef CONFIG_HVM
> >  static void p2m_teardown_nestedp2m(struct domain *d)
> >  {
> >      unsigned int i;
> > @@ -186,6 +187,7 @@ static int p2m_init_nestedp2m(struct domain *d)
> >  
> >      return 0;
> >  }
> > +#endif
> 
> With the goal of limited code churn I think these would better be put
> around the entire body of the function. That way the ones below
> enclosing the function calls can go away.

Later the ifdefs here and some other places will be expand to cover
altp2m as well. If we enclose only the body here, we will need to do the
same things for altp2m functions.

The end result is we will actually have more or less the
same amount whether we change to that method or not. But with this
method we will have less code churn when moving those functions out to
another file.

> 
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -333,9 +333,11 @@ struct arch_domain
> >          void (*tail)(struct vcpu *);
> >      } *ctxt_switch;
> >  
> > +#ifdef CONFIG_HVM
> >      /* nestedhvm: translate l2 guest physical to host physical */
> >      struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
> >      mm_lock_t nested_p2m_lock;
> > +#endif
> 
> Not something to be done here, just a general remark / question (perhaps
> also more to George than you): Why do we have part of things here ...
> 
> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -204,6 +204,7 @@ struct p2m_domain {
> >  
> >      p2m_class_t       p2m_class; /* host/nested/alternate */
> >  
> > +#ifdef CONFIG_HVM
> >      /* Nested p2ms only: nested p2m base value that this p2m shadows.
> >       * This can be cleared to P2M_BASE_EADDR under the per-p2m lock but
> >       * needs both the per-p2m lock and the per-domain nestedp2m lock
> > @@ -216,6 +217,7 @@ struct p2m_domain {
> >       * The host p2m hasolds the head of the list and the np2ms are 
> >       * threaded on in LRU order. */
> >      struct list_head   np2m_list;
> > +#endif
> 
> ... and another part here?

I also found them a bit strange to be placed in two different
structures. I will leave this question to George.

Wei.

> 
> Jan
> 
> 

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