Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.10.17 at 14:25, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3756,14 +3756,13 @@ 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)
> +/* Caller must hold write lock as version may change and table may grow */
> +static int gnttab_get_frame_locked(struct domain *d, unsigned long idx,
> +                                   mfn_t *mfn)

I don't think this helper function needs to be passed d; gt appears
to suffice.

> @@ -3787,6 +3786,19 @@ int gnttab_map_frame(struct domain *d, unsigned long 
> idx, gfn_t gfn,
>              rc = -EINVAL;
>      }
>  
> +    return rc;
> +}
> +
> +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> +                     mfn_t *mfn)
> +{
> +    struct grant_table *gt = d->grant_table;
> +    int rc;
> +
> +    grant_write_lock(gt);
> +
> +    rc = gnttab_get_frame_locked(d, idx, mfn);
> +
>      if ( !rc )
>          gnttab_set_frame_gfn(gt, idx, gfn);
>  
> @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d, unsigned long 
> idx, gfn_t gfn,
>      return rc;
>  }
>  
> +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t *mfn)

const struct domain * (I realize now that the same should have
been done for gnttab_map_frame() when it was introduced;
perhaps you could change that at the same time).

> @@ -966,6 +967,30 @@ static long xatp_permission_check(struct domain *d, 
> unsigned int space)
>  }
>  
>  #ifdef CONFIG_X86
> +static int acquire_grant_table(struct domain *d, unsigned int id,

This clearly isn't x86-specific code.

> +                               unsigned long frame,
> +                               unsigned long nr_frames,
> +                               unsigned long mfn_list[])
> +{
> +    unsigned int i = nr_frames;

Silent truncation just like in the earlier patch.

> +    if ( id != 0 )

Do we want a constant here again? Also acquiring the status page
MFNs via separate ID would seem better than re-using
XENMAPIDX_grant_table_status here, the more that this uses bit
31 while right now your interface structure field is still 64 bits wide.

> @@ -993,6 +1018,11 @@ static int acquire_resource(const 
> xen_mem_acquire_resource_t *xmar)
>                                           xmar->nr_frames, mfn_list);
>          break;
>  
> +    case XENMEM_resource_grant_table:
> +        rc = acquire_grant_table(d, xmar->id, xmar->frame,
> +                                 xmar->nr_frames, mfn_list);
> +        break;

Is this really generally useful with mfn_list[] having just two entries?

Jan


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