Re: [Xen-devel] [PATCH v7 7/7] x86: extend the map and unmap xen_iommu_ops to support grant references

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.10.18 at 12:35, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3927,6 +3927,157 @@ int gnttab_get_status_frame(struct domain *d, 
> unsigned long idx,
>      return rc;
>  }
>  
> +int
> +acquire_gref_for_iommu(struct domain *d, grant_ref_t gref,
> +                       bool readonly, mfn_t *mfn)

As a non-static function this may deserve a gnttab_ prefix.

> +{
> +    struct domain *currd = current->domain;
> +    struct grant_table *gt = d->grant_table;
> +    grant_entry_header_t *shah;
> +    struct active_grant_entry *act;
> +    uint16_t *status;
> +    int rc;
> +
> +    grant_read_lock(gt);
> +
> +    rc = -ENOENT;
> +    if ( gref > nr_grant_entries(gt) )

>=

> +        goto unlock;
> +
> +    act = active_entry_acquire(gt, gref);
> +    shah = shared_entry_header(gt, gref);
> +    status = ( gt->gt_version == 2 ) ?

Stray blanks. (All three also apply to the release counterpart.)

> +        &status_entry(gt, gref) :
> +        &shah->flags;
> +
> +    rc = -EACCES;
> +    if ( (shah->flags & GTF_type_mask) != GTF_permit_access ||
> +         (shah->flags & GTF_sub_page) )
> +        goto release;
> +
> +    rc = -ERANGE;
> +    if ( act->pin && ((act->domid != currd->domain_id) ||
> +                      (act->pin & 0x80808080U) != 0) )

I realize this follows the pattern used elsewhere, but it's going to far,
and I'd prefer if we didn't spread the problem: You don't mean to
increment all of the four fields, but just one or two of them. For the
others there's no point doing the respective overflow check.

> +        goto release;
> +
> +    rc = -EINVAL;
> +    if ( !act->pin ||
> +         (!readonly && !(act->pin & GNTPIN_devw_mask)) ) {
> +        if ( _set_status(gt->gt_version, currd->domain_id, readonly,
> +                         0, shah, act, status) != GNTST_okay )
> +            goto release;
> +    }

Please combine such directly nested if()s. There's also a misplaced
figure brace, but that'll then go away anyway.

> +    if ( !act->pin )
> +    {
> +        gfn_t gfn = gt->gt_version == 1 ?
> +            _gfn(shared_entry_v1(gt, gref).frame) :
> +            _gfn(shared_entry_v2(gt, gref).full_page.frame);
> +        p2m_type_t p2mt;
> +        struct page_info *page;
> +
> +        rc = check_get_page_from_gfn(d, gfn, readonly, &p2mt, &page);
> +        if ( rc )
> +            goto clear;
> +
> +        rc = -EINVAL;
> +        if ( p2m_is_foreign(p2mt) )

Compared to you allowing p2m_ram_rw only in the previous patch,
this allows basically everything. Is this really intended?

> +        {
> +            put_page(page);
> +            goto clear;
> +        }
> +
> +        act_set_gfn(act, gfn);
> +        act->mfn = page_to_mfn(page);
> +        act->domid = currd->domain_id;
> +        act->start = 0;
> +        act->length = PAGE_SIZE;
> +        act->is_sub_page = false;
> +        act->trans_domain = d;
> +        act->trans_gref = gref;
> +    }
> +    else
> +    {
> +        ASSERT(mfn_valid(act->mfn));
> +        if ( !get_page(mfn_to_page(act->mfn), d) )
> +            goto clear;
> +    }
> +
> +    rc = 0;
> +    act->pin += readonly ? GNTPIN_devr_inc : GNTPIN_devw_inc;

You don't acquire a write ref in the !readonly case.

> +int
> +release_gref_for_iommu(struct domain *d, grant_ref_t gref,
> +                       bool readonly, mfn_t mfn)
> +{
> +    struct domain *currd = current->domain;
> +    struct grant_table *gt = d->grant_table;
> +    grant_entry_header_t *shah;
> +    struct active_grant_entry *act;
> +    uint16_t *status;
> +    int rc;
> +
> +    grant_read_lock(gt);
> +
> +    rc = -ENOENT;
> +    if ( gref > nr_grant_entries(gt) )
> +        goto unlock;
> +
> +    act = active_entry_acquire(gt, gref);
> +    shah = shared_entry_header(gt, gref);
> +    status = ( gt->gt_version == 2 ) ?
> +        &status_entry(gt, gref) :
> +        &shah->flags;
> +
> +    rc = -EINVAL;
> +    if ( !act->pin || (act->domid != currd->domain_id) ||
> +         !mfn_eq(act->mfn, mfn) )
> +        goto release;

Code elsewhere also considers "readonly" in similar checks. Along
the lines of the comment in the acquire function I'd actually prefer
if you did the checks more correctly by checking against
GNTPIN_dev{r,w}_mask depending on the flag.

> @@ -159,16 +159,31 @@ static int iommuop_map(struct xen_iommu_op_map *op)
>      if ( !d )
>          return -ESRCH;
>  
> -    rc = check_get_page_from_gfn(d, _gfn(op->gfn), readonly, &p2mt, &page);
> -    if ( rc )
> -        goto unlock_domain;
> -
> -    rc = -EINVAL;
> -    if ( p2mt != p2m_ram_rw ||
> -         (!readonly && !get_page_type(page, PGT_writable_page)) )
> +    if ( op->flags & XEN_IOMMUOP_map_gref )
>      {
> -        put_page(page);
> -        goto unlock_domain;
> +        rc = acquire_gref_for_iommu(d, op->u.gref, readonly, &mfn);
> +        if ( rc )
> +            goto unlock_domain;
> +    }
> +    else
> +    {
> +        p2m_type_t p2mt;
> +        struct page_info *page;
> +
> +        rc = check_get_page_from_gfn(d, _gfn(op->u.gfn), readonly, &p2mt,
> +                                     &page);
> +        if ( rc )
> +            goto unlock_domain;
> +
> +        rc = -EINVAL;
> +        if ( p2mt != p2m_ram_rw ||
> +             (!readonly && !get_page_type(page, PGT_writable_page)) )

In the description you talk about doing grant maps also by GFN,
but you don't extend the set of permitted bytes here. Is the
description misleading, or is the code incomplete?

Jan


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