Re: [Xen-devel] [PATCH for-next 4/8] x86/domain: push some code down to hvm_domain_initialise

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Apr 24, 2017 at 04:10:14AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 15:27, <wei.liu2@xxxxxxxxxx> wrote:
> > We want to have a single entry point to initialise hvm guest.  The
> > timing to set hap bit and create per domain mapping is deferred, but
> > that's not a problem.
> > 
> > No functional change.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/domain.c         | 11 ++---------
> >  xen/arch/x86/hvm/hvm.c        | 25 +++++++++++++++++--------
> >  xen/include/asm-x86/hvm/hvm.h |  2 +-
> >  3 files changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index ddebff6187..af060d8239 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -587,14 +587,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> > domcr_flags,
> >          d->arch.emulation_flags = emflags;
> >      }
> >  
> > -    if ( is_hvm_domain(d) )
> > -    {
> > -        d->arch.hvm_domain.hap_enabled =
> > -            hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
> > -
> > -        rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, 
> > NULL);
> 
> I'm not really certain it is a good idea to move this last one, as I can't
> immediately spot uses of it from the hvm/ subtree.
> 

I think the sole purpose of this call is to create perdomain_l3_pg (when
nr is 0), which is then referenced in monitor table creation. Moving
this doesn't seem to cause problem.

> > -    }
> > -    else if ( is_idle_domain(d) )
> > +    if ( is_idle_domain(d) )
> >          rc = 0;
> >      else
> 
> This now needs to be "else if ( !is_hvm_domain(d) )" afaict.
> 

I see what you mean. 

> > @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d)
> >  
> >      hvm_init_cacheattr_region_list(d);
> >  
> > -    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
> > +    d->arch.hvm_domain.hap_enabled =
> > +        hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
> > +
> > +    rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);
> >      if ( rc != 0 )
> >          goto fail0;
> >  
> > +    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
> > +    if ( rc != 0 )
> > +        goto fail1;
> 
> Is the order of these required to be that way? The various failN
> labels would seem to not require re-numbering if you swapped
> the last two actions.
> 

The mapping needs to be created before updating paging mode. I don't
think the order is important in this function.

Wei.

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel