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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 12 September 2018 15:13
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> 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.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)
> 

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel