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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.08.18 at 10:40, <wei.liu2@xxxxxxxxxx> wrote:
> 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.

Oh, I see - I'm sorry.

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

I think that's what is needed, yes.

Jan



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