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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.18 at 16:16, <paul.durrant@xxxxxxxxxx> wrote:
> +static int gnttab_get_status_frame_mfn(struct domain *d,
> +                                       unsigned long idx, mfn_t *mfn)
> +{
> +    const struct grant_table *gt = d->grant_table;
> +
> +    ASSERT(gt->gt_version == 2);
> +
> +    if ( idx >= nr_status_frames(gt) )
> +    {
> +        unsigned long nr_status;
> +        unsigned long nr_grant;
> +
> +        nr_status = idx + 1; /* sufficient frames to make idx valid */
> +        nr_grant = status_to_grant_frames(nr_status);
> +
> +        if ( nr_grant <= nr_grant_frames(gt) ) /* overflow check */
> +            return -EINVAL;

If the table is currently empty, this would always fail, wouldn't
it? You haven't grown the table yet by this point.

> +        if ( nr_grant <= gt->max_grant_frames )
> +            gnttab_grow_table(d, nr_grant);

And here (other than originally in gnttab_map_frame()) you
invoke gnttab_grow_table() perhaps pointlessly (when the table
doesn't in fact need growing).

> @@ -1027,6 +1066,11 @@ static int acquire_resource(
>  
>      switch ( xmar.type )
>      {
> +    case XENMEM_resource_grant_table:
> +        rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
> +                                 mfn_list);
> +        break;
> +
>      default:
>          rc = arch_acquire_resource(d, xmar.type, xmar.id, xmar.frame,
>                                     xmar.nr_frames, mfn_list, &xmar.flags);
> @@ -1046,6 +1090,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) )
> +            return -EOPNOTSUPP;
> +

Now that I look at this again - wouldn't this check better live ahead
of the main switch()? I find it odd, for example, that in this case the
grant table would still have got grown.

Jan



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