Re: [Xen-devel] [PATCH v3 3/8] xen: delay allocation of grant table sub structures

[Wei Liu]

On Wed, Sep 06, 2017 at 05:36:54PM +0200, Juergen Gross wrote:
> On 06/09/17 17:32, Wei Liu wrote:
> > On Wed, Sep 06, 2017 at 05:15:46PM +0200, Juergen Gross wrote:
>  +grant_table_init(struct domain *d)
>  +{
>  +struct grant_table *gt = d->grant_table;
>  +unsigned int i, j;
>  +
>  +if ( gt->nr_grant_frames )
>  +return 0;
>  +
> >>>
> >>> EBUSY here? I think we should catch the cases when this is called
> >>> multiple times.
> >>
> >> No. The call of grant_table_init() from
> >> domain_unpause_by_systemcontroller() can't be masked, otherwise I
> >> would have to make struct grant_table public again. Multiple calls
> >> are okay.
> > 
> > For domain_unpause_by_systemcontroller, isn't it already guarded by
> > d->creation_finished to ensure there is only one call to
> > grant_table_init?
> > 
> > Or do you mean if gnttab_table_init fails the system administrator will
> > somehow tries to unpause the domain again hence calling grant_table_init
> > again?
> > 
> 
> No. It might have been called already due to e.g. gnttab_setup_table()
> being called as a result of xc_dom_gnttab_seed() during creation of the
> domU. The call from domain_unpause_by_systemcontroller() is just a
> safety net for cases where gnttab_setup_table() wasn't used (e.g. in
> case of a xenstore stubdom).
> 

Hmm... OK. In that case I think the multiple call paths is justified.
You can have my Rb on this patch.

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

Re: [Xen-devel] [PATCH v3 3/8] xen: delay allocation of grant table sub structures

[Juergen Gross]

On 06/09/17 17:32, Wei Liu wrote:
> On Wed, Sep 06, 2017 at 05:15:46PM +0200, Juergen Gross wrote:
 +grant_table_init(struct domain *d)
 +{
 +struct grant_table *gt = d->grant_table;
 +unsigned int i, j;
 +
 +if ( gt->nr_grant_frames )
 +return 0;
 +
>>>
>>> EBUSY here? I think we should catch the cases when this is called
>>> multiple times.
>>
>> No. The call of grant_table_init() from
>> domain_unpause_by_systemcontroller() can't be masked, otherwise I
>> would have to make struct grant_table public again. Multiple calls
>> are okay.
> 
> For domain_unpause_by_systemcontroller, isn't it already guarded by
> d->creation_finished to ensure there is only one call to
> grant_table_init?
> 
> Or do you mean if gnttab_table_init fails the system administrator will
> somehow tries to unpause the domain again hence calling grant_table_init
> again?
> 

No. It might have been called already due to e.g. gnttab_setup_table()
being called as a result of xc_dom_gnttab_seed() during creation of the
domU. The call from domain_unpause_by_systemcontroller() is just a
safety net for cases where gnttab_setup_table() wasn't used (e.g. in
case of a xenstore stubdom).


Juergen

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

Re: [Xen-devel] [PATCH v3 3/8] xen: delay allocation of grant table sub structures

[Wei Liu]

On Wed, Sep 06, 2017 at 05:15:46PM +0200, Juergen Gross wrote:
> >> +grant_table_init(struct domain *d)
> >> +{
> >> +struct grant_table *gt = d->grant_table;
> >> +unsigned int i, j;
> >> +
> >> +if ( gt->nr_grant_frames )
> >> +return 0;
> >> +
> > 
> > EBUSY here? I think we should catch the cases when this is called
> > multiple times.
> 
> No. The call of grant_table_init() from
> domain_unpause_by_systemcontroller() can't be masked, otherwise I
> would have to make struct grant_table public again. Multiple calls
> are okay.

For domain_unpause_by_systemcontroller, isn't it already guarded by
d->creation_finished to ensure there is only one call to
grant_table_init?

Or do you mean if gnttab_table_init fails the system administrator will
somehow tries to unpause the domain again hence calling grant_table_init
again?

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

Re: [Xen-devel] [PATCH v3 3/8] xen: delay allocation of grant table sub structures

[Juergen Gross]

On 06/09/17 17:11, Wei Liu wrote:
> On Wed, Sep 06, 2017 at 02:46:48PM +0200, Juergen Gross wrote:
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 5aebcf265f..11eb1778a3 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -363,6 +363,9 @@ struct domain *domain_create(domid_t domid, unsigned int 
>> domcr_flags,
>>  goto fail;
>>  init_status |= INIT_gnttab;
>>  
>> +if ( domid == 0 && grant_table_init(d) )
>> +goto fail;
>> +
>>  poolid = 0;
>>  
>>  err = -ENOMEM;
>> @@ -998,7 +1001,8 @@ int __domain_pause_by_systemcontroller(struct domain *d,
>>  prev = cmpxchg(>controller_pause_count, old, new);
>>  } while ( prev != old );
>>  
>> -pause_fn(d);
>> +if ( pause_fn )
>> +pause_fn(d);
>>  
>>  return 0;
>>  }
>> @@ -1006,6 +1010,7 @@ int __domain_pause_by_systemcontroller(struct domain 
>> *d,
>>  int domain_unpause_by_systemcontroller(struct domain *d)
>>  {
>>  int old, new, prev = d->controller_pause_count;
>> +int ret;
>>  
>>  do
>>  {
>> @@ -1029,8 +1034,16 @@ int domain_unpause_by_systemcontroller(struct domain 
>> *d)
>>   * Creation is considered finished when the controller reference count
>>   * first drops to 0.
>>   */
>> -if ( new == 0 )
>> +if ( new == 0 && !d->creation_finished )
>> +{
> 
> ret can be defined locally here.

Hmm, yes.

> 
>> +ret = grant_table_init(d);
>> +if ( ret )
>> +{
>> +__domain_pause_by_systemcontroller(d, NULL);
>> +return ret;
>> +}
>>  d->creation_finished = true;
>> +}
>>  
>>  domain_unpause(d);
>>  
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 4520e36d90..29e7fa539b 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -1655,6 +1655,78 @@ gnttab_unpopulate_status_frames(struct domain *d, 
>> struct grant_table *gt)
>>  gt->nr_status_frames = 0;
>>  }
>>  
>> +int
>> +grant_table_init(struct domain *d)
>> +{
>> +struct grant_table *gt = d->grant_table;
>> +unsigned int i, j;
>> +
>> +if ( gt->nr_grant_frames )
>> +return 0;
>> +
> 
> EBUSY here? I think we should catch the cases when this is called
> multiple times.

No. The call of grant_table_init() from
domain_unpause_by_systemcontroller() can't be masked, otherwise I
would have to make struct grant_table public again. Multiple calls
are okay.

> 
> With those changed:
> 
> Reviewed-by: Wei Liu 
> 

Juergen

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

Re: [Xen-devel] [PATCH v3 3/8] xen: delay allocation of grant table sub structures

[Wei Liu]

On Wed, Sep 06, 2017 at 02:46:48PM +0200, Juergen Gross wrote:
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 5aebcf265f..11eb1778a3 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -363,6 +363,9 @@ struct domain *domain_create(domid_t domid, unsigned int 
> domcr_flags,
>  goto fail;
>  init_status |= INIT_gnttab;
>  
> +if ( domid == 0 && grant_table_init(d) )
> +goto fail;
> +
>  poolid = 0;
>  
>  err = -ENOMEM;
> @@ -998,7 +1001,8 @@ int __domain_pause_by_systemcontroller(struct domain *d,
>  prev = cmpxchg(>controller_pause_count, old, new);
>  } while ( prev != old );
>  
> -pause_fn(d);
> +if ( pause_fn )
> +pause_fn(d);
>  
>  return 0;
>  }
> @@ -1006,6 +1010,7 @@ int __domain_pause_by_systemcontroller(struct domain *d,
>  int domain_unpause_by_systemcontroller(struct domain *d)
>  {
>  int old, new, prev = d->controller_pause_count;
> +int ret;
>  
>  do
>  {
> @@ -1029,8 +1034,16 @@ int domain_unpause_by_systemcontroller(struct domain 
> *d)
>   * Creation is considered finished when the controller reference count
>   * first drops to 0.
>   */
> -if ( new == 0 )
> +if ( new == 0 && !d->creation_finished )
> +{

ret can be defined locally here.

> +ret = grant_table_init(d);
> +if ( ret )
> +{
> +__domain_pause_by_systemcontroller(d, NULL);
> +return ret;
> +}
>  d->creation_finished = true;
> +}
>  
>  domain_unpause(d);
>  
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 4520e36d90..29e7fa539b 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1655,6 +1655,78 @@ gnttab_unpopulate_status_frames(struct domain *d, 
> struct grant_table *gt)
>  gt->nr_status_frames = 0;
>  }
>  
> +int
> +grant_table_init(struct domain *d)
> +{
> +struct grant_table *gt = d->grant_table;
> +unsigned int i, j;
> +
> +if ( gt->nr_grant_frames )
> +return 0;
> +

EBUSY here? I think we should catch the cases when this is called
multiple times.

With those changed:

Reviewed-by: Wei Liu 

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

Re: [Xen-devel] [PATCH v3 3/8] xen: delay allocation of grant table sub structures

[Paul Durrant]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
> Juergen Gross
> Sent: 06 September 2017 13:47
> To: xen-devel@lists.xen.org
> Cc: Juergen Gross ; sstabell...@kernel.org; Wei Liu
> ; George Dunlap ;
> Andrew Cooper ; Ian Jackson
> ; Tim (Xen.org) ;
> julien.gr...@arm.com; jbeul...@suse.com
> Subject: [Xen-devel] [PATCH v3 3/8] xen: delay allocation of grant table sub
> structures
> 
> Delay the allocation of the grant table sub structures in order to
> allow modifying parameters needed for sizing of these structures at a
> per domain basis. Either do it from gnttab_setup_table() or just
> before the domain is started the first time.
> 
> Signed-off-by: Juergen Gross 
> ---
> V3:
> - move call of grant_table_init() from gnttab_setup_table() to
>   gnttab_grow_table() (Paul Durrant)

Thanks :-)

Reviewed-by: Paul Durrant 

> ---
>  xen/common/domain.c   |  17 +-
>  xen/common/grant_table.c  | 138 --
> 
>  xen/include/xen/grant_table.h |   2 +
>  3 files changed, 96 insertions(+), 61 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 5aebcf265f..11eb1778a3 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -363,6 +363,9 @@ struct domain *domain_create(domid_t domid,
> unsigned int domcr_flags,
>  goto fail;
>  init_status |= INIT_gnttab;
> 
> +if ( domid == 0 && grant_table_init(d) )
> +goto fail;
> +
>  poolid = 0;
> 
>  err = -ENOMEM;
> @@ -998,7 +1001,8 @@ int __domain_pause_by_systemcontroller(struct
> domain *d,
>  prev = cmpxchg(>controller_pause_count, old, new);
>  } while ( prev != old );
> 
> -pause_fn(d);
> +if ( pause_fn )
> +pause_fn(d);
> 
>  return 0;
>  }
> @@ -1006,6 +1010,7 @@ int __domain_pause_by_systemcontroller(struct
> domain *d,
>  int domain_unpause_by_systemcontroller(struct domain *d)
>  {
>  int old, new, prev = d->controller_pause_count;
> +int ret;
> 
>  do
>  {
> @@ -1029,8 +1034,16 @@ int domain_unpause_by_systemcontroller(struct
> domain *d)
>   * Creation is considered finished when the controller reference count
>   * first drops to 0.
>   */
> -if ( new == 0 )
> +if ( new == 0 && !d->creation_finished )
> +{
> +ret = grant_table_init(d);
> +if ( ret )
> +{
> +__domain_pause_by_systemcontroller(d, NULL);
> +return ret;
> +}
>  d->creation_finished = true;
> +}
> 
>  domain_unpause(d);
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 4520e36d90..29e7fa539b 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1655,6 +1655,78 @@ gnttab_unpopulate_status_frames(struct domain
> *d, struct grant_table *gt)
>  gt->nr_status_frames = 0;
>  }
> 
> +int
> +grant_table_init(struct domain *d)
> +{
> +struct grant_table *gt = d->grant_table;
> +unsigned int i, j;
> +
> +if ( gt->nr_grant_frames )
> +return 0;
> +
> +gt->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
> +
> +/* Active grant table. */
> +if ( (gt->active = xzalloc_array(struct active_grant_entry *,
> + max_nr_active_grant_frames)) == NULL )
> +goto no_mem_1;
> +for ( i = 0;
> +  i <
> num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
> +{
> +if ( (gt->active[i] = alloc_xenheap_page()) == NULL )
> +goto no_mem_2;
> +clear_page(gt->active[i]);
> +for ( j = 0; j < ACGNT_PER_PAGE; j++ )
> +spin_lock_init(>active[i][j].lock);
> +}
> +
> +/* Tracking of mapped foreign frames table */
> +gt->maptrack = vzalloc(max_maptrack_frames * sizeof(*gt->maptrack));
> +if ( gt->maptrack == NULL )
> +goto no_mem_2;
> +
> +/* Shared grant table. */
> +if ( (gt->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL
> )
> +goto no_mem_3;
> +for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> +{
> +if ( (gt->shared_raw[i] = alloc_xenheap_page()) == NULL )
> +goto no_mem_4;
> +clear_page(gt->shared_raw[i]);
> +}
> +
> +/* Status pages for grant table - for version 2 */
> +gt->status = xzalloc_array(grant_status_t *,
> +   grant_to_status_frames(max_grant_frames));
> +if ( gt->status == NULL )
> +goto no_mem_4;
> +
> +for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> +gnttab_create_shared_page(d, gt, i);
> +
> +gt->nr_status_frames = 0;
> +
> +return 0;
> +
> + no_mem_4:
> +for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> +free_xenheap_page(gt->shared_raw[i]);
>