Re: [Xen-devel] [PATCH v2 14/23] x86/mm: put nested p2m code under CONFIG_HVM

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Aug 27, 2018 at 09:56:24AM -0600, Jan Beulich wrote:
> >>> On 26.08.18 at 14:19, <wei.liu2@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1689,7 +1689,8 @@ void context_switch(struct vcpu *prev, struct vcpu 
> > *next)
> >      {
> >          _update_runstate_area(prev);
> >          vpmu_switch_from(prev);
> > -        np2m_schedule(NP2M_SCHEDLE_OUT);
> > +        if ( nestedhvm_enabled(prevd) )
> > +            np2m_schedule(NP2M_SCHEDLE_OUT);
> >      }
> >  
> >      if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm_vcpu.tm_list) 
> > )
> > @@ -1756,7 +1757,8 @@ void context_switch(struct vcpu *prev, struct vcpu 
> > *next)
> >  
> >          /* Must be done with interrupts enabled */
> >          vpmu_switch_to(next);
> > -        np2m_schedule(NP2M_SCHEDLE_IN);
> > +        if ( nestedhvm_enabled(nextd) )
> > +            np2m_schedule(NP2M_SCHEDLE_IN);
> >      }
> 
> How do these additions help? nestedhvm_enabled() is not an inline
> function, and the entire series doesn't seem to touch hvm/nestedhvm.h
> (i.e. there's no inline stub being added).

The patch that changed nestedhvm_enabled was committed before I sent out
this series -- please pull the latest changes.

> 
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -144,6 +144,7 @@ static void p2m_teardown_hostp2m(struct domain *d)
> >  
> >  static void p2m_teardown_nestedp2m(struct domain *d)
> >  {
> > +#ifdef CONFIG_HVM
> >      unsigned int i;
> >      struct p2m_domain *p2m;
> >  
> > @@ -156,10 +157,12 @@ static void p2m_teardown_nestedp2m(struct domain *d)
> >          p2m_free_one(p2m);
> >          d->arch.nested_p2m[i] = NULL;
> >      }
> > +#endif
> >  }
> >  
> >  static int p2m_init_nestedp2m(struct domain *d)
> >  {
> > +#ifdef CONFIG_HVM
> >      unsigned int i;
> >      struct p2m_domain *p2m;
> >  
> > @@ -176,6 +179,7 @@ static int p2m_init_nestedp2m(struct domain *d)
> >          p2m->write_p2m_entry = nestedp2m_write_p2m_entry;
> >          list_add(&p2m->np2m_list, &p2m_get_hostp2m(d)->np2m_list);
> >      }
> > +#endif
> >  
> >      return 0;
> >  }
> 
> Hmm, I think this is too ad hoc for my taste: For one I'm puzzled
> by the lack of any (existing) is_hvm_domain() here. And then the
> fields initialization of which you skip should also disappear, to
> eliminate the risk of some code somewhere using the fields
> uninitialized. This might simply mean to move the fields from
> struct arch_domain to struct hvm_domain. I understand this may
> end up being a more involved task, but it looks pretty much
> unavoidable to me.

p2m_init is called unconditionally for both PV and HVM. At the time I
read the code it appeared that it required nestedp2m to be initialised
and tear down unconditionally.

Do you want me to rewrite p2m_init and p2m_teardown_final to put things
under is_hvm_domain?

Regarding fields in data structure, I think moving them would be a good
idea.

Wei.

> 
> Jan
> 
> 

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