> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 12 September 2018 15:13
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Wei Liu <wei.l...@citrix.com>; George
> Dunlap <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;
> Stefano Stabellini <sstabell...@kernel.org>; xen-devel <xen-
> de...@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.w...@oracle.com>; Tim (Xen.org) <t...@xen.org>
> Subject: Re: [PATCH v6 14/14] x86: extend the map and unmap iommu_ops
> to support grant references
>
> >>> On 23.08.18 at 11:47, <paul.durr...@citrix.com> 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)
>
Yes, good point.
> > + 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.
Ok.
>
> > + &status_entry(gt, gref) :
> > + &shah->flags;
>
> The whole thing does not fit on a line?
I'll check.
>
> > + 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?
>
No, they do need to be avoided.
> > + 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.
>
True.
> > + 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.
>
Ok.
> > + 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?
>
Yes, perhaps I should do a get_page_type() in iommuop_map() regardless of
whether the page comes from a gfn or a gref lookup.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
