Re: [Xen-devel] [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise

[Wei Liu]

On Wed, Apr 26, 2017 at 07:27:23AM -0600, Jan Beulich wrote:
> >>> On 26.04.17 at 14:53,  wrote:
> > On Wed, Apr 26, 2017 at 06:25:32AM -0600, Jan Beulich wrote:
> >> >  if ( is_hvm_domain(d) )
> >> > -{
> >> >  rc = hvm_vcpu_initialise(v);
> >> > -goto done;
> >> > -}
> >> > -
> >> > -
> >> > -spin_lock_init(>arch.pv_vcpu.shadow_ldt_lock);
> >> > -
> >> > -if ( !is_idle_domain(d) )
> >> > -{
> >> > -rc = pv_create_gdt_ldt_l1tab(d, v);
> >> > -if ( rc )
> >> > -goto done;
> >> > -
> >> > -BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
> >> > - PAGE_SIZE);
> >> > -v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
> >> > -  NR_VECTORS);
> >> > -if ( !v->arch.pv_vcpu.trap_ctxt )
> >> > -{
> >> > -pv_destroy_gdt_ldt_l1tab(d, v);
> >> > -rc = -ENOMEM;
> >> > -goto done;
> >> > -}
> >> > -
> >> > -/* PV guests by default have a 100Hz ticker. */
> >> > -v->periodic_period = MILLISECS(10);
> >> > -}
> >> > +else if ( is_pv_domain(d) && !is_idle_domain(d) )
> >> 
> >> Only the right side of the && is needed, as is_pv is now (again) the
> >> opposite of is_hvm.
> >> 
> > 
> > The idle domain is a PV guest (see domain_create) and handled in the
> > following else branch. 
> 
> Exactly, so all you need here is "else if ( !is_idle_domain(d) )".
> 

Yes you're right. I misread your comment, sorry.

> Jan
> 
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [Xen-devel] [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise

[Jan Beulich]

>>> On 26.04.17 at 14:53,  wrote:
> On Wed, Apr 26, 2017 at 06:25:32AM -0600, Jan Beulich wrote:
>> >  if ( is_hvm_domain(d) )
>> > -{
>> >  rc = hvm_vcpu_initialise(v);
>> > -goto done;
>> > -}
>> > -
>> > -
>> > -spin_lock_init(>arch.pv_vcpu.shadow_ldt_lock);
>> > -
>> > -if ( !is_idle_domain(d) )
>> > -{
>> > -rc = pv_create_gdt_ldt_l1tab(d, v);
>> > -if ( rc )
>> > -goto done;
>> > -
>> > -BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
>> > - PAGE_SIZE);
>> > -v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
>> > -  NR_VECTORS);
>> > -if ( !v->arch.pv_vcpu.trap_ctxt )
>> > -{
>> > -pv_destroy_gdt_ldt_l1tab(d, v);
>> > -rc = -ENOMEM;
>> > -goto done;
>> > -}
>> > -
>> > -/* PV guests by default have a 100Hz ticker. */
>> > -v->periodic_period = MILLISECS(10);
>> > -}
>> > +else if ( is_pv_domain(d) && !is_idle_domain(d) )
>> 
>> Only the right side of the && is needed, as is_pv is now (again) the
>> opposite of is_hvm.
>> 
> 
> The idle domain is a PV guest (see domain_create) and handled in the
> following else branch. 

Exactly, so all you need here is "else if ( !is_idle_domain(d) )".

Jan


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

Re: [Xen-devel] [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise

[Wei Liu]

On Wed, Apr 26, 2017 at 06:25:32AM -0600, Jan Beulich wrote:
> >  if ( is_hvm_domain(d) )
> > -{
> >  rc = hvm_vcpu_initialise(v);
> > -goto done;
> > -}
> > -
> > -
> > -spin_lock_init(>arch.pv_vcpu.shadow_ldt_lock);
> > -
> > -if ( !is_idle_domain(d) )
> > -{
> > -rc = pv_create_gdt_ldt_l1tab(d, v);
> > -if ( rc )
> > -goto done;
> > -
> > -BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
> > - PAGE_SIZE);
> > -v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
> > -  NR_VECTORS);
> > -if ( !v->arch.pv_vcpu.trap_ctxt )
> > -{
> > -pv_destroy_gdt_ldt_l1tab(d, v);
> > -rc = -ENOMEM;
> > -goto done;
> > -}
> > -
> > -/* PV guests by default have a 100Hz ticker. */
> > -v->periodic_period = MILLISECS(10);
> > -}
> > +else if ( is_pv_domain(d) && !is_idle_domain(d) )
> 
> Only the right side of the && is needed, as is_pv is now (again) the
> opposite of is_hvm.
> 

The idle domain is a PV guest (see domain_create) and handled in the
following else branch. 

> Jan
> 

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

Re: [Xen-devel] [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise

[Jan Beulich]

>>> On 25.04.17 at 15:52,  wrote:
> +static int pv_vcpu_initialise(struct vcpu *v)
> +{
> +struct domain *d = v->domain;
> +int rc;
> +
> +ASSERT(!is_idle_domain(d));
> +
> +spin_lock_init(>arch.pv_vcpu.shadow_ldt_lock);
> +
> +rc = pv_create_gdt_ldt_l1tab(d, v);
> +if ( rc )
> +goto done;

You may simply return here.

> @@ -424,61 +468,18 @@ int vcpu_initialise(struct vcpu *v)
>  spin_lock_init(>arch.vpmu.vpmu_lock);
>  
>  if ( is_hvm_domain(d) )
> -{
>  rc = hvm_vcpu_initialise(v);
> -goto done;
> -}
> -
> -
> -spin_lock_init(>arch.pv_vcpu.shadow_ldt_lock);
> -
> -if ( !is_idle_domain(d) )
> -{
> -rc = pv_create_gdt_ldt_l1tab(d, v);
> -if ( rc )
> -goto done;
> -
> -BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
> - PAGE_SIZE);
> -v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
> -  NR_VECTORS);
> -if ( !v->arch.pv_vcpu.trap_ctxt )
> -{
> -pv_destroy_gdt_ldt_l1tab(d, v);
> -rc = -ENOMEM;
> -goto done;
> -}
> -
> -/* PV guests by default have a 100Hz ticker. */
> -v->periodic_period = MILLISECS(10);
> -}
> +else if ( is_pv_domain(d) && !is_idle_domain(d) )

Only the right side of the && is needed, as is_pv is now (again) the
opposite of is_hvm.

Jan


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

Re: [Xen-devel] [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise

[Wei Liu]

On Tue, Apr 25, 2017 at 03:08:23PM +0100, Andrew Cooper wrote:
> On 25/04/17 14:52, Wei Liu wrote:
> > Move PV specific vcpu initialisation code to said function, but leave
> > the only line needed by idle domain in vcpu_initialise.
> >
> > Use pv_vcpu_destroy in error path to simplify code. It is safe to do so
> > because the destruction function accepts partially initialised vcpu
> > struct.
> >
> > Signed-off-by: Wei Liu 
> > ---
> > Cc: Jan Beulich 
> > Cc: Andrew Cooper 
> > ---
> >  xen/arch/x86/domain.c | 99 
> > ++-
> >  1 file changed, 50 insertions(+), 49 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index cde0917f5b..38fc4f5d8b 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -398,6 +398,50 @@ static void pv_destroy_gdt_ldt_l1tab(struct domain *d, 
> > struct vcpu *v)
> >  destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << 
> > GDT_LDT_VCPU_SHIFT);
> >  }
> >  
> > +static void pv_vcpu_destroy(struct vcpu *v);
> 
> If in the previous patch, you create pv_vcpu_destroy() earlier than
> vcpu_initialise(), you wouldn't need this forward declaration here.
> 

Yeah I know. I chose to rearranged them in the patch that moved pv code
instead, because rebasing huge chunk of code is error-prone.

> Otherwise, Reviewed-by: Andrew Cooper 

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

Re: [Xen-devel] [PATCH for-next v2 05/10] x86/domain: factor out pv_vcpu_initialise

[Andrew Cooper]

On 25/04/17 14:52, Wei Liu wrote:
> Move PV specific vcpu initialisation code to said function, but leave
> the only line needed by idle domain in vcpu_initialise.
>
> Use pv_vcpu_destroy in error path to simplify code. It is safe to do so
> because the destruction function accepts partially initialised vcpu
> struct.
>
> Signed-off-by: Wei Liu 
> ---
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> ---
>  xen/arch/x86/domain.c | 99 
> ++-
>  1 file changed, 50 insertions(+), 49 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index cde0917f5b..38fc4f5d8b 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -398,6 +398,50 @@ static void pv_destroy_gdt_ldt_l1tab(struct domain *d, 
> struct vcpu *v)
>  destroy_perdomain_mapping(d, GDT_VIRT_START(v), 1 << GDT_LDT_VCPU_SHIFT);
>  }
>  
> +static void pv_vcpu_destroy(struct vcpu *v);

If in the previous patch, you create pv_vcpu_destroy() earlier than
vcpu_initialise(), you wouldn't need this forward declaration here.

Otherwise, Reviewed-by: Andrew Cooper 

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