Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.07.2020 13:37, Andrew Cooper wrote:
> The existing logic doesn't function in the general case for mapping a guests
> grant table, due to arbitrary 32 frame limit, and the default grant table
> limit being 64.
>
> In order to start addressing this, rework the existing grant table logic by
> implementing a single gnttab_acquire_resource().  This is far more efficient
> than the previous acquire_grant_table() in memory.c because it doesn't take
> the grant table write lock, and attempt to grow the table, for every single
> frame.
Among the code you replace there is a comment "Iterate backwards in case
table needs to grow" explaining why what you say about growing the grant
table didn't actually happen.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>       return 0;
>   }
>   
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;
> +    void **vaddrs;
> +    int rc = 0;
> +
> +    /* Input sanity. */
> +    if ( !nr_frames )
> +        return -EINVAL;
I can't seem to be able to find an equivalent of this in the old logic,
and hence this looks like an unwarranted change in behavior to me. We
have quite a few hypercall ops where some count being zero is simply
a no-op, i.e. yielding success without doing anything.

> +    /* Overflow checks */
> +    if ( frame + nr_frames < frame )
> +        return -EINVAL;
> +
> +    tot_frames = frame + nr_frames;
> +    if ( tot_frames != frame + nr_frames )
> +        return -EINVAL;
I find the naming here quite confusing. I realize part of this stems
from the code you replace, but anyway: "unsigned long frame" typically
represents a memory frame number of some sort, making the calculation
look as if it was wrong. (Initially I merely meant to ask whether this
check isn't redundant with the prior one, or vice versa.)

> +    /* Grow table if necessary. */
> +    grant_write_lock(gt);
> +    switch ( id )
> +    {
> +        mfn_t tmp;
> +
> +    case XENMEM_resource_grant_table_id_shared:
> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        if ( gt->gt_version != 2 )
> +        {
> +    default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +    }
> +
> +    /* Any errors from growing the table? */
> +    if ( rc )
> +        goto out;
> +
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        vaddrs = gt->shared_raw;
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        vaddrs = (void **)gt->status;
Now this is the kind of cast that I consider really dangerous, and hence
worth trying hard to avoid. With the code structure as is, I don't see
an immediate solution though.

> +        break;
> +    }
Worth having an ASSERT_UNREACHABLE() default case here?

Jan