Re: [Xen-devel] [PATCH v20 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.08.18 at 14:54, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3860,6 +3860,38 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, 
> grant_ref_t ref,
>  }
>  #endif
>  
> +/* caller must hold read or write lock */
> +static int gnttab_get_status_frame_mfn(struct domain *d,
> +                                       unsigned long idx, mfn_t *mfn)
> +{
> +    struct grant_table *gt = d->grant_table;
> +
> +    if ( idx >= nr_status_frames(gt) )
> +        return -EINVAL;
> +
> +    *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> +    return 0;
> +}
> +
> +/* caller must hold write lock */
> +static int gnttab_get_shared_frame_mfn(struct domain *d,
> +                                       unsigned long idx, mfn_t *mfn)
> +{
> +    struct grant_table *gt = d->grant_table;
> +
> +    if ( gt->gt_version == 0 )
> +        gt->gt_version = 1;
> +
> +    if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
> +        gnttab_grow_table(d, idx + 1);

I guess I had commented on this before, but it has been a while:
While I realize that you're just moving code, I dislike the resulting
asymmetry between the two functions: Why would the former
not want to grow the grant table? And why would a version
adjustment be needed (only) here (I've put "only" in parentheses
because it's not obvious to me why this is needed)?

And this asymmetry would surely be coming as surprise at least
to callers of the new interface you add.

> +int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> +                            mfn_t *mfn)
> +{
> +    struct grant_table *gt = d->grant_table;
> +    int rc;
> +
> +    grant_read_lock(gt);
> +    rc = gnttab_get_status_frame_mfn(d, idx, mfn);
> +    grant_read_unlock(gt);
> +
> +    return rc;
> +}

Along the lines of what gnttab_map_frame() does, I'd expect a
check for gt_version to be 2 here. Or well, maybe that's implicit
by there not being any status entries/pages for version 1 tables.
But it would become a requirement if gnttab_grow_table() was to
be called from gnttab_get_status_frame_mfn().

> @@ -982,6 +983,54 @@ 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,
> +                               xen_pfn_t mfn_list[])
> +{
> +    unsigned int i = nr_frames;
> +
> +    /*
> +     * FIXME: It is not currently safe to map grant status frames if they
> +     *        will be inserted into the caller's P2M, because these
> +     *        insertions are not yet properly reference counted.
> +     *        This restriction can be removed when appropriate reference
> +     *        counting is added.
> +     */
> +    if ( paging_mode_translate(current->domain) &&
> +         (id == XENMEM_resource_grant_table_id_status) )
> +        return -EOPNOTSUPP;

Would you mind reminding me why the P2M insertions are safe for
the "ordinary" (shared) grant frames? I'm puzzled because P2M
management in general lacks refcounting, yet I have the vague
feeling that we had discussed this before and there was a reason.
(If there is, perhaps slightly extending the comment above would
be helpful.)

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -611,16 +611,21 @@ struct xen_mem_acquire_resource {
>      uint16_t type;
>  
>  #define XENMEM_resource_ioreq_server 0
> +#define XENMEM_resource_grant_table 1
>  
>      /*
>       * IN - a type-specific resource identifier, which must be zero
>       *      unless stated otherwise.
>       *
>       * type == XENMEM_resource_ioreq_server -> id == ioreq server id
> +     * type == XENMEM_resource_grant_table -> id defined below
>       */
>      uint32_t id;
> -    /*
> -     * IN/OUT - As an IN parameter number of frames of the resource
> +
> +#define XENMEM_resource_grant_table_id_shared 0
> +#define XENMEM_resource_grant_table_id_status 1
> +
> +    /* IN/OUT - As an IN parameter number of frames of the resource

Please don't break previously correct comment style here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel