Re: [Xen-devel] [PATCH v6 14/14] x86: extend the map and unmap iommu_ops to support grant references

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> +int
> +acquire_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) )

>= (also in the release counterpart)

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

Stray blanks. Further down in a similar construct you even omit the
parentheses, which you could as well do here too. Again also below.

> +        &status_entry(gt, gref) :
> +        &shah->flags;

The whole thing does not fit on a line?

> +    rc = -EACCES;
> +    if ( (shah->flags & GTF_type_mask) != GTF_permit_access ||
> +         (shah->flags & GTF_sub_page) )
> +        goto release;

So transitive grants are okay despite there being no special
handling anywhere in the function?

> +    rc = -ERANGE;
> +    if ( act->pin && ((act->domid != currd->domain_id) ||
> +                      (act->pin & 0x80808080U) != 0) )

You want to check only two of the four top bits, as you only add in
GNTPIN_dev{r,w}_inc below.

> +        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 the two if()-s.

> +    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);
> +        struct page_info *page;
> +
> +        rc =  get_paged_gfn(d, gfn, readonly, NULL, &page);
> +        if ( rc )
> +            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;
> +    }

Don't you also need to also acquire a write ref here if !readonly?

Jan



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