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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.18 at 11:00, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -358,6 +358,12 @@ static inline unsigned int 
> grant_to_status_frames(unsigned int grant_frames)
>      return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE, 
> GRANT_STATUS_PER_PAGE);
>  }
>  
> +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
> +static inline unsigned int status_to_grant_frames(unsigned int status_frames)
> +{
> +    return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE, 
> GRANT_PER_PAGE);
> +}

Seeing no use of the grant table (it's not even passed in) I'm confused
by the comment you add. I guess you've simply copied
grant_to_status_frames()'es, which looks similarly wrong.

> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, 
> grant_ref_t ref,
>  }
>  #endif
>  
> +/* caller must hold 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;

const?

> +    ASSERT(gt->gt_version == 2);
> +
> +    if ( idx >= nr_status_frames(gt) )
> +    {
> +        unsigned long nr = status_to_grant_frames(idx + 1);
> +
> +        if ( nr <= gt->max_grant_frames )
> +            gnttab_grow_table(d, nr);
> +
> +        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;

Again?

> @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d, unsigned long 
> idx, gfn_t gfn,
>      return rc;
>  }
>  
> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> +                            mfn_t *mfn)
> +{
> +    struct grant_table *gt = d->grant_table;
> +    int rc;
> +
> +    grant_write_lock(gt);
> +
> +    if ( gt->gt_version == 0 )
> +        gt->gt_version = 1;

Since you've moved this here instead of dropping it, what requirement
have you found for this to be set (other than the ASSERT() you put in
gnttab_get_shared_frame_mfn()?

> @@ -1046,6 +1089,16 @@ static int acquire_resource(
>          xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
>          unsigned int i;
>  
> +        /*
> +         * FIXME: until foreign pages inserted into the P2M are properly
> +         *        reference counted, it is unsafe to allow mapping of
> +         *        non-caller-owned resource pages unless the caller is
> +         *        the hardware domain.
> +         */
> +        if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
> +            !is_hardware_domain(currd) )

Missing blank and as a result improper indentation. Also the U in
"until" wants to be upper case I think.

The cosmetic aspects could of course be taken care of while
committing, but the version related question needs to be
clarified first.

Jan



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