All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v6 01/12] xen: correct gnttab_get_status_frames()
       [not found] ` <20170913154651.2366-2-jgross@suse.com>
@ 2017-09-13 16:01   ` Paul Durrant
  2017-09-13 16:58     ` Juergen Gross
  2017-09-18 15:02   ` Wei Liu
  1 sibling, 1 reply; 14+ messages in thread
From: Paul Durrant @ 2017-09-13 16:01 UTC (permalink / raw)
  To: 'Juergen Gross', xen-devel
  Cc: sstabellini, Wei Liu, Andrew Cooper, Tim (Xen.org),
	George Dunlap, julien.grall, jbeulich, Ian Jackson, dgdegra

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Juergen Gross
> Sent: 13 September 2017 08:47
> To: xen-devel@lists.xen.org
> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> julien.grall@arm.com; jbeulich@suse.com; dgdegra@tycho.nsa.gov
> Subject: [Xen-devel] [PATCH v6 01/12] xen: correct
> gnttab_get_status_frames()
> 
> In gnttab_get_status_frames() all accesses to nr_status_frames should
> be done with the grant table lock held.

Is this true? The value can only increase so what does the increase lock scope actually protect against?

  Paul

> 
> While at it correct coding style: labels should be indented by one
> space.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/grant_table.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index c3895e6201..00ff075bd9 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2866,19 +2866,19 @@
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
> s_frames_t) uop,
> 
>      gt = d->grant_table;
> 
> +    op.status = GNTST_okay;
> +
> +    grant_read_lock(gt);
> +
>      if ( unlikely(op.nr_frames > nr_status_frames(gt)) )
>      {
>          gdprintk(XENLOG_INFO, "Guest requested addresses for %d grant
> status "
>                   "frames, but only %d are available.\n",
>                   op.nr_frames, nr_status_frames(gt));
>          op.status = GNTST_general_error;
> -        goto out2;
> +        goto unlock;
>      }
> 
> -    op.status = GNTST_okay;
> -
> -    grant_read_lock(gt);
> -
>      for ( i = 0; i < op.nr_frames; i++ )
>      {
>          gmfn = gnttab_status_gmfn(d, gt, i);
> @@ -2886,10 +2886,11 @@
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
> s_frames_t) uop,
>              op.status = GNTST_bad_virt_addr;
>      }
> 
> + unlock:
>      grant_read_unlock(gt);
> -out2:
> + out2:
>      rcu_unlock_domain(d);
> -out1:
> + out1:
>      if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
>          return -EFAULT;
> 
> --
> 2.12.3
> 
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 01/12] xen: correct gnttab_get_status_frames()
  2017-09-13 16:01   ` [PATCH v6 01/12] xen: correct gnttab_get_status_frames() Paul Durrant
@ 2017-09-13 16:58     ` Juergen Gross
  2017-09-13 17:36       ` Paul Durrant
  0 siblings, 1 reply; 14+ messages in thread
From: Juergen Gross @ 2017-09-13 16:58 UTC (permalink / raw)
  To: Paul Durrant, xen-devel
  Cc: sstabellini, Wei Liu, Andrew Cooper, Tim (Xen.org),
	George Dunlap, julien.grall, jbeulich, Ian Jackson, dgdegra

On 13/09/17 18:01, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
>> Juergen Gross
>> Sent: 13 September 2017 08:47
>> To: xen-devel@lists.xen.org
>> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu
>> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
>> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
>> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
>> julien.grall@arm.com; jbeulich@suse.com; dgdegra@tycho.nsa.gov
>> Subject: [Xen-devel] [PATCH v6 01/12] xen: correct
>> gnttab_get_status_frames()
>>
>> In gnttab_get_status_frames() all accesses to nr_status_frames should
>> be done with the grant table lock held.
> 
> Is this true? The value can only increase so what does the increase lock scope actually protect against?

The comment above nr_status_frames() says so. Either the comment or the
code is wrong.


Juergen

> 
>   Paul
> 
>>
>> While at it correct coding style: labels should be indented by one
>> space.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  xen/common/grant_table.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index c3895e6201..00ff075bd9 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -2866,19 +2866,19 @@
>> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
>> s_frames_t) uop,
>>
>>      gt = d->grant_table;
>>
>> +    op.status = GNTST_okay;
>> +
>> +    grant_read_lock(gt);
>> +
>>      if ( unlikely(op.nr_frames > nr_status_frames(gt)) )
>>      {
>>          gdprintk(XENLOG_INFO, "Guest requested addresses for %d grant
>> status "
>>                   "frames, but only %d are available.\n",
>>                   op.nr_frames, nr_status_frames(gt));
>>          op.status = GNTST_general_error;
>> -        goto out2;
>> +        goto unlock;
>>      }
>>
>> -    op.status = GNTST_okay;
>> -
>> -    grant_read_lock(gt);
>> -
>>      for ( i = 0; i < op.nr_frames; i++ )
>>      {
>>          gmfn = gnttab_status_gmfn(d, gt, i);
>> @@ -2886,10 +2886,11 @@
>> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
>> s_frames_t) uop,
>>              op.status = GNTST_bad_virt_addr;
>>      }
>>
>> + unlock:
>>      grant_read_unlock(gt);
>> -out2:
>> + out2:
>>      rcu_unlock_domain(d);
>> -out1:
>> + out1:
>>      if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
>>          return -EFAULT;
>>
>> --
>> 2.12.3
>>
>>
>> _______________________________________________
>> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 04/12] xen: add new domctl hypercall to set grant table resource limits
       [not found] ` <20170913154651.2366-5-jgross@suse.com>
@ 2017-09-13 17:32   ` Daniel De Graaf
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel De Graaf @ 2017-09-13 17:32 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, julien.grall, jbeulich

On 09/13/2017 11:46 AM, Juergen Gross wrote:
> Add a domctl hypercall to set the domain's resource limits regarding
> grant tables. It is accepted only as long as neither
> gnttab_setup_table() has been called for the domain, nor the domain
> has started to run.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 01/12] xen: correct gnttab_get_status_frames()
  2017-09-13 16:58     ` Juergen Gross
@ 2017-09-13 17:36       ` Paul Durrant
  0 siblings, 0 replies; 14+ messages in thread
From: Paul Durrant @ 2017-09-13 17:36 UTC (permalink / raw)
  To: 'Juergen Gross', xen-devel
  Cc: sstabellini, Wei Liu, Andrew Cooper, Tim (Xen.org),
	George Dunlap, julien.grall, jbeulich, Ian Jackson, dgdegra

> -----Original Message-----
> From: Juergen Gross [mailto:jgross@suse.com]
> Sent: 13 September 2017 09:58
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xen.org
> Cc: sstabellini@kernel.org; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; julien.grall@arm.com; jbeulich@suse.com;
> dgdegra@tycho.nsa.gov
> Subject: Re: [Xen-devel] [PATCH v6 01/12] xen: correct
> gnttab_get_status_frames()
> 
> On 13/09/17 18:01, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> >> Juergen Gross
> >> Sent: 13 September 2017 08:47
> >> To: xen-devel@lists.xen.org
> >> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu
> >> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> >> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> >> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> >> julien.grall@arm.com; jbeulich@suse.com; dgdegra@tycho.nsa.gov
> >> Subject: [Xen-devel] [PATCH v6 01/12] xen: correct
> >> gnttab_get_status_frames()
> >>
> >> In gnttab_get_status_frames() all accesses to nr_status_frames should
> >> be done with the grant table lock held.
> >
> > Is this true? The value can only increase so what does the increase lock
> scope actually protect against?
> 
> The comment above nr_status_frames() says so. Either the comment or the
> code is wrong.

Ok. I suspect that was cut'n'paste from nr_grant_fames() but I see no particular harm in the increased scope since it's a read lock.

Reviewed-by: Paul Durrant <paul.durrant@citrix.com> 

> 
> 
> Juergen
> 
> >
> >   Paul
> >
> >>
> >> While at it correct coding style: labels should be indented by one
> >> space.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> >> ---
> >>  xen/common/grant_table.c | 15 ++++++++-------
> >>  1 file changed, 8 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> >> index c3895e6201..00ff075bd9 100644
> >> --- a/xen/common/grant_table.c
> >> +++ b/xen/common/grant_table.c
> >> @@ -2866,19 +2866,19 @@
> >>
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
> >> s_frames_t) uop,
> >>
> >>      gt = d->grant_table;
> >>
> >> +    op.status = GNTST_okay;
> >> +
> >> +    grant_read_lock(gt);
> >> +
> >>      if ( unlikely(op.nr_frames > nr_status_frames(gt)) )
> >>      {
> >>          gdprintk(XENLOG_INFO, "Guest requested addresses for %d grant
> >> status "
> >>                   "frames, but only %d are available.\n",
> >>                   op.nr_frames, nr_status_frames(gt));
> >>          op.status = GNTST_general_error;
> >> -        goto out2;
> >> +        goto unlock;
> >>      }
> >>
> >> -    op.status = GNTST_okay;
> >> -
> >> -    grant_read_lock(gt);
> >> -
> >>      for ( i = 0; i < op.nr_frames; i++ )
> >>      {
> >>          gmfn = gnttab_status_gmfn(d, gt, i);
> >> @@ -2886,10 +2886,11 @@
> >>
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
> >> s_frames_t) uop,
> >>              op.status = GNTST_bad_virt_addr;
> >>      }
> >>
> >> + unlock:
> >>      grant_read_unlock(gt);
> >> -out2:
> >> + out2:
> >>      rcu_unlock_domain(d);
> >> -out1:
> >> + out1:
> >>      if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
> >>          return -EFAULT;
> >>
> >> --
> >> 2.12.3
> >>
> >>
> >> _______________________________________________
> >> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 09/12] xen: delay allocation of grant table sub structures
       [not found] ` <20170913154651.2366-10-jgross@suse.com>
@ 2017-09-14  5:41   ` Juergen Gross
  0 siblings, 0 replies; 14+ messages in thread
From: Juergen Gross @ 2017-09-14  5:41 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, julien.grall, jbeulich, dgdegra

On 13/09/17 17:46, Juergen Gross wrote:
> 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_grow_table() or just
> before the domain is started the first time.

Uuh, sorry, I forgot to update the commit message.


Juergen

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 11/12] xen: make grant resource limits per domain
       [not found] ` <20170913154651.2366-12-jgross@suse.com>
@ 2017-09-14  5:42   ` Juergen Gross
  0 siblings, 0 replies; 14+ messages in thread
From: Juergen Gross @ 2017-09-14  5:42 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, julien.grall, jbeulich, dgdegra

On 13/09/17 17:46, Juergen Gross wrote:
> Instead of using the same global resource limits of grant tables (max.
> number of grant frames, max. number of maptrack frames) for all domains
> make these limits per domain. This will allow setting individual limits
> in the future. For now initialize the per domain limits with the global
> values.

Wrong commit message again.


Juergen

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 02/12] xen: move XENMAPSPACE_grant_table code into grant_table.c
       [not found] ` <20170913154651.2366-3-jgross@suse.com>
@ 2017-09-14 17:20   ` Julien Grall
  2017-09-15  7:25     ` Juergen Gross
  0 siblings, 1 reply; 14+ messages in thread
From: Julien Grall @ 2017-09-14 17:20 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, jbeulich, dgdegra

Hi Juergen,

On 13/09/17 16:46, Juergen Gross wrote:
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 00ff075bd9..a462ea7905 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3608,6 +3608,44 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
>   }
>   #endif
>   
> +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> +                     mfn_t *mfn)
> +{
> +    int rc = 0;
> +    struct grant_table *gt = d->grant_table;
> +
> +    grant_write_lock(gt);
> +
> +    if ( gt->gt_version == 0 )
> +        gt->gt_version = 1;
> +
> +    if ( gt->gt_version == 2 &&
> +         (idx & XENMAPIDX_grant_table_status) )
> +    {
> +        idx &= ~XENMAPIDX_grant_table_status;
> +        if ( idx < nr_status_frames(gt) )
> +            *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> +        else
> +            rc = -EINVAL;
> +    }
> +    else
> +    {
> +        if ( (idx >= nr_grant_frames(gt)) && (idx < max_grant_frames) )
> +            gnttab_grow_table(d, idx + 1);
> +
> +        if ( idx < nr_grant_frames(gt) )
> +            *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
> +        else
> +            rc = -EINVAL;
> +    }
> +
> +    gnttab_set_frame_gfn(d, idx, gfn);

This code is slightly different compare to the ARM implementation. The 
gfn is now set even if the rc is non-zero (i.e invalid MFN on the ARM 
implementation).

So I think you need to protect gnttab_set_frame_gfn with if ( !rc ).

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 10/12] xen/arm: move arch specific grant table bits into grant_table.c
       [not found] ` <20170913154651.2366-11-jgross@suse.com>
@ 2017-09-14 17:31   ` Julien Grall
  2017-09-15  7:22     ` Juergen Gross
  0 siblings, 1 reply; 14+ messages in thread
From: Julien Grall @ 2017-09-14 17:31 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, jbeulich, dgdegra

Hi Juergen,

On 13/09/17 16:46, Juergen Gross wrote:
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index df11b31264..f3f2fb9ebc 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -27,7 +27,8 @@
>   #include <xen/rwlock.h>
>   #include <public/grant_table.h>
>   #include <asm/page.h>
> -#include <asm/grant_table.h>

This change looks a bit strange to me. This is the only place where 
asm/grant_table.h is pulled. Because you remove it, it now means that 
the prototype will not be defined first and may result to mismatch in 
the future.

Ideally we should enforce, although it would require some work as we 
didn't really follow that rule in a few places.

Cheers,

> +
> +struct grant_table;
>   
>   /* The maximum size of a grant table. */
>   extern unsigned int max_grant_frames;
> 

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 10/12] xen/arm: move arch specific grant table bits into grant_table.c
  2017-09-14 17:31   ` [PATCH v6 10/12] xen/arm: move arch specific grant table bits " Julien Grall
@ 2017-09-15  7:22     ` Juergen Gross
  2017-09-15  8:49       ` Julien Grall
  0 siblings, 1 reply; 14+ messages in thread
From: Juergen Gross @ 2017-09-15  7:22 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, jbeulich, dgdegra

On 14/09/17 19:31, Julien Grall wrote:
> Hi Juergen,
> 
> On 13/09/17 16:46, Juergen Gross wrote:
>> diff --git a/xen/include/xen/grant_table.h
>> b/xen/include/xen/grant_table.h
>> index df11b31264..f3f2fb9ebc 100644
>> --- a/xen/include/xen/grant_table.h
>> +++ b/xen/include/xen/grant_table.h
>> @@ -27,7 +27,8 @@
>>   #include <xen/rwlock.h>
>>   #include <public/grant_table.h>
>>   #include <asm/page.h>
>> -#include <asm/grant_table.h>
> 
> This change looks a bit strange to me. This is the only place where
> asm/grant_table.h is pulled. Because you remove it, it now means that
> the prototype will not be defined first and may result to mismatch in
> the future.
> 
> Ideally we should enforce, although it would require some work as we
> didn't really follow that rule in a few places.

Aah, I missed the prototypes which relate to functions outside of
grant_table.c. Strange that the compiler didn't barf at me.

All those functions seem to be rather small, so I can add them as inline
to asm-arm/grant_table.h, as they are used only by grant_table.c.


Juergen


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 02/12] xen: move XENMAPSPACE_grant_table code into grant_table.c
  2017-09-14 17:20   ` [PATCH v6 02/12] xen: move XENMAPSPACE_grant_table code into grant_table.c Julien Grall
@ 2017-09-15  7:25     ` Juergen Gross
  0 siblings, 0 replies; 14+ messages in thread
From: Juergen Gross @ 2017-09-15  7:25 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, jbeulich, dgdegra

On 14/09/17 19:20, Julien Grall wrote:
> Hi Juergen,
> 
> On 13/09/17 16:46, Juergen Gross wrote:
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 00ff075bd9..a462ea7905 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -3608,6 +3608,44 @@ int mem_sharing_gref_to_gfn(struct grant_table
>> *gt, grant_ref_t ref,
>>   }
>>   #endif
>>   +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
>> +                     mfn_t *mfn)
>> +{
>> +    int rc = 0;
>> +    struct grant_table *gt = d->grant_table;
>> +
>> +    grant_write_lock(gt);
>> +
>> +    if ( gt->gt_version == 0 )
>> +        gt->gt_version = 1;
>> +
>> +    if ( gt->gt_version == 2 &&
>> +         (idx & XENMAPIDX_grant_table_status) )
>> +    {
>> +        idx &= ~XENMAPIDX_grant_table_status;
>> +        if ( idx < nr_status_frames(gt) )
>> +            *mfn = _mfn(virt_to_mfn(gt->status[idx]));
>> +        else
>> +            rc = -EINVAL;
>> +    }
>> +    else
>> +    {
>> +        if ( (idx >= nr_grant_frames(gt)) && (idx < max_grant_frames) )
>> +            gnttab_grow_table(d, idx + 1);
>> +
>> +        if ( idx < nr_grant_frames(gt) )
>> +            *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
>> +        else
>> +            rc = -EINVAL;
>> +    }
>> +
>> +    gnttab_set_frame_gfn(d, idx, gfn);
> 
> This code is slightly different compare to the ARM implementation. The
> gfn is now set even if the rc is non-zero (i.e invalid MFN on the ARM
> implementation).
> 
> So I think you need to protect gnttab_set_frame_gfn with if ( !rc ).

Right, thanks for noticing.


Juergen

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 10/12] xen/arm: move arch specific grant table bits into grant_table.c
  2017-09-15  7:22     ` Juergen Gross
@ 2017-09-15  8:49       ` Julien Grall
  2017-09-15  9:21         ` Juergen Gross
  0 siblings, 1 reply; 14+ messages in thread
From: Julien Grall @ 2017-09-15  8:49 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, jbeulich, dgdegra, nd

Hi Juergen,

On 15/09/2017 08:22, Juergen Gross wrote:
> On 14/09/17 19:31, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 13/09/17 16:46, Juergen Gross wrote:
>>> diff --git a/xen/include/xen/grant_table.h
>>> b/xen/include/xen/grant_table.h
>>> index df11b31264..f3f2fb9ebc 100644
>>> --- a/xen/include/xen/grant_table.h
>>> +++ b/xen/include/xen/grant_table.h
>>> @@ -27,7 +27,8 @@
>>>   #include <xen/rwlock.h>
>>>   #include <public/grant_table.h>
>>>   #include <asm/page.h>
>>> -#include <asm/grant_table.h>
>>
>> This change looks a bit strange to me. This is the only place where
>> asm/grant_table.h is pulled. Because you remove it, it now means that
>> the prototype will not be defined first and may result to mismatch in
>> the future.
>>
>> Ideally we should enforce, although it would require some work as we
>> didn't really follow that rule in a few places.
>
> Aah, I missed the prototypes which relate to functions outside of
> grant_table.c. Strange that the compiler didn't barf at me.

It is because we don't set -Wmissing-prototypes. This would break 
compilation on Xen because of quite a few missing prototypes within the 
source code.

>
> All those functions seem to be rather small, so I can add them as inline
> to asm-arm/grant_table.h, as they are used only by grant_table.c.

I think you would still need to include asm/grant_table.h for x86 to get 
the prototype defined for create_grant_p2m_mapping & co.

To be honest, I am not sure to fully understand the rationale to not 
include asm/grant_table.h in grant_table.h.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 10/12] xen/arm: move arch specific grant table bits into grant_table.c
  2017-09-15  8:49       ` Julien Grall
@ 2017-09-15  9:21         ` Juergen Gross
  0 siblings, 0 replies; 14+ messages in thread
From: Juergen Gross @ 2017-09-15  9:21 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, tim, jbeulich, dgdegra, nd

On 15/09/17 10:49, Julien Grall wrote:
> Hi Juergen,
> 
> On 15/09/2017 08:22, Juergen Gross wrote:
>> On 14/09/17 19:31, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 13/09/17 16:46, Juergen Gross wrote:
>>>> diff --git a/xen/include/xen/grant_table.h
>>>> b/xen/include/xen/grant_table.h
>>>> index df11b31264..f3f2fb9ebc 100644
>>>> --- a/xen/include/xen/grant_table.h
>>>> +++ b/xen/include/xen/grant_table.h
>>>> @@ -27,7 +27,8 @@
>>>>   #include <xen/rwlock.h>
>>>>   #include <public/grant_table.h>
>>>>   #include <asm/page.h>
>>>> -#include <asm/grant_table.h>
>>>
>>> This change looks a bit strange to me. This is the only place where
>>> asm/grant_table.h is pulled. Because you remove it, it now means that
>>> the prototype will not be defined first and may result to mismatch in
>>> the future.
>>>
>>> Ideally we should enforce, although it would require some work as we
>>> didn't really follow that rule in a few places.
>>
>> Aah, I missed the prototypes which relate to functions outside of
>> grant_table.c. Strange that the compiler didn't barf at me.
> 
> It is because we don't set -Wmissing-prototypes. This would break
> compilation on Xen because of quite a few missing prototypes within the
> source code.

Uuh, I wasn't aware of this!

>> All those functions seem to be rather small, so I can add them as inline
>> to asm-arm/grant_table.h, as they are used only by grant_table.c.
> 
> I think you would still need to include asm/grant_table.h for x86 to get
> the prototype defined for create_grant_p2m_mapping & co.
>
> To be honest, I am not sure to fully understand the rationale to not
> include asm/grant_table.h in grant_table.h.

The idea was that asm/grant_table.h only contains abstractions needed in
grant_table.c. Why include it in a Xen-global header when only one
source needs its contents? Additionally when I started this patch I
tried to use inline functions instead of #defines in asm/grant-table.h
leading to compile errors when struct grant_table was dereferenced.

But with the missing prototypes problem I think it might really be
better to keep the #include in grant_table.h.


Juergen

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 01/12] xen: correct gnttab_get_status_frames()
       [not found] ` <20170913154651.2366-2-jgross@suse.com>
  2017-09-13 16:01   ` [PATCH v6 01/12] xen: correct gnttab_get_status_frames() Paul Durrant
@ 2017-09-18 15:02   ` Wei Liu
  1 sibling, 0 replies; 14+ messages in thread
From: Wei Liu @ 2017-09-18 15:02 UTC (permalink / raw)
  To: Juergen Gross
  Cc: tim, sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, xen-devel, julien.grall, jbeulich, dgdegra

On Wed, Sep 13, 2017 at 05:46:40PM +0200, Juergen Gross wrote:
> In gnttab_get_status_frames() all accesses to nr_status_frames should
> be done with the grant table lock held.
> 
> While at it correct coding style: labels should be indented by one
> space.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6 06/12] tools: set grant limits for xenstore stubdom
       [not found] ` <20170913154651.2366-7-jgross@suse.com>
@ 2017-09-18 15:22   ` Wei Liu
  0 siblings, 0 replies; 14+ messages in thread
From: Wei Liu @ 2017-09-18 15:22 UTC (permalink / raw)
  To: Juergen Gross
  Cc: tim, sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, xen-devel, julien.grall, jbeulich, dgdegra

On Wed, Sep 13, 2017 at 05:46:45PM +0200, Juergen Gross wrote:
> When creating a Xenstore stubdom set the grant limits: the stubdom
> will need to setup a very limited amount of grants only, so 1 grant
> frame is enough. For being able to support up to 32768 domains it
> will need 128 maptrack frames (1 mapping per domain, 256 maptrack
> entries per maptrack frame).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-09-18 15:22 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20170913154651.2366-1-jgross@suse.com>
     [not found] ` <20170913154651.2366-5-jgross@suse.com>
2017-09-13 17:32   ` [PATCH v6 04/12] xen: add new domctl hypercall to set grant table resource limits Daniel De Graaf
     [not found] ` <20170913154651.2366-10-jgross@suse.com>
2017-09-14  5:41   ` [PATCH v6 09/12] xen: delay allocation of grant table sub structures Juergen Gross
     [not found] ` <20170913154651.2366-12-jgross@suse.com>
2017-09-14  5:42   ` [PATCH v6 11/12] xen: make grant resource limits per domain Juergen Gross
     [not found] ` <20170913154651.2366-3-jgross@suse.com>
2017-09-14 17:20   ` [PATCH v6 02/12] xen: move XENMAPSPACE_grant_table code into grant_table.c Julien Grall
2017-09-15  7:25     ` Juergen Gross
     [not found] ` <20170913154651.2366-11-jgross@suse.com>
2017-09-14 17:31   ` [PATCH v6 10/12] xen/arm: move arch specific grant table bits " Julien Grall
2017-09-15  7:22     ` Juergen Gross
2017-09-15  8:49       ` Julien Grall
2017-09-15  9:21         ` Juergen Gross
     [not found] ` <20170913154651.2366-2-jgross@suse.com>
2017-09-13 16:01   ` [PATCH v6 01/12] xen: correct gnttab_get_status_frames() Paul Durrant
2017-09-13 16:58     ` Juergen Gross
2017-09-13 17:36       ` Paul Durrant
2017-09-18 15:02   ` Wei Liu
     [not found] ` <20170913154651.2366-7-jgross@suse.com>
2017-09-18 15:22   ` [PATCH v6 06/12] tools: set grant limits for xenstore stubdom Wei Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.