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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

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

Jan



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