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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.10.17 at 15:24, <paul.durrant@xxxxxxxxxx> wrote:
> v12:
>  - Dropped limit checks as requested by Jan.

Thanks, but ...

> +int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> +                            mfn_t *mfn)
> +{
> +    struct grant_table *gt = d->grant_table;
> +    int rc;
> +
> +    /* write lock required as version may change and/or table may grow */
> +    grant_write_lock(gt);
> +    rc = gnttab_get_frame(d, idx | XENMAPIDX_grant_table_status, mfn);

... this ORing won't work then. You need to pass the function a
separate bool, which would likely call for a prereq patch.

> @@ -965,12 +966,49 @@ static long xatp_permission_check(struct domain *d, 
> unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
>  
> +static int acquire_grant_table(struct domain *d, unsigned int id,
> +                               unsigned long frame,
> +                               unsigned int nr_frames,
> +                               unsigned long mfn_list[])
> +{
> +    unsigned int i = nr_frames;
> +
> +    /* Iterate backwards in case table needs to grow */
> +    while ( i-- != 0 )
> +    {
> +        mfn_t mfn = INVALID_MFN;
> +        int rc;
> +
> +        switch ( id )
> +        {
> +        case XENMEM_resource_grant_table_id_grant:
> +            rc = gnttab_get_grant_frame(d, frame + i, &mfn);
> +            break;
> +
> +        case XENMEM_resource_grant_table_id_status:
> +            rc = gnttab_get_status_frame(d, frame + i, &mfn);
> +            break;
> +
> +        default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        if ( rc )
> +            return rc;
> +
> +        mfn_list[i] = mfn_x(mfn);
> +    }
> +
> +    return 0;
> +}
> +
>  static int acquire_resource(
>      XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
>  {
>      struct domain *d, *currd = current->domain;
>      xen_mem_acquire_resource_t xmar;
> -    unsigned long mfn_list[2];
> +    unsigned long mfn_list[32];

Together with the GFN array further down this makes up for 512
bytes of stack space, if my calculation wasn't wrong. That's quite
a lot and certainly something that shouldn't be further increased.
I can accept it remaining this way for now (provided ARM has a
large enough stack as well), but please add a comment indicating
that if further growth is needed, converting to e.g. per-CPU
arrays is going to be necessary.

Jan


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