Re: [PATCH v2 REPOST 12/12] x86/hvm/ioreq: add a new mappable resource type...

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 25 August 2017 10:53
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 REPOST 12/12] x86/hvm/ioreq: add a
> new mappable resource type...
> 
> On Fri, Aug 25, 2017 at 10:46:48AM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne
> > > > +static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
> > > > +{
> > > > +    struct domain *currd = current->domain;
> > > > +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> > > > +
> > > > +    if ( iorp->page )
> > > > +    {
> > > > +        /* Make sure the page has not been mapped */
> > > > +        if ( !gfn_eq(iorp->gfn, INVALID_GFN) )
> > > > +            return -EPERM;
> > > > +
> > > > +        return 0;
> > >
> > > Shouldn't this return EEXIST? Page has already been allocated by a
> > > previous call AFAICT, and it seems like a possible error/misbehavior
> > > to try to do it twice.
> > >
> >
> > The checks are there to prevent a caller from trying to mix the legacy and
> new methods of mapping ioreq server pages so EPERM (i.e. 'operation not
> permitted') seems like the correct error. I agree that it's not obvious, at this
> inner level, that I do think this is right. I'm open to debate about this though.
> 
> Oh, I was referring to the 'return 0;', not the 'return -EPERM;' (that
> looks fine to me).
> 
> The 'return 0;' means that the page is already setup (if I understood
> this correctly), that's why I was wondering whether it should return
> -EEXIST instead.
> 

No, it has to be this way because of the 'on demand' nature of the allocation. If an emulator uses the new resource mapping scheme then the pages will be allocated on the first mapping attempt, but if the emulator unmaps and then re-maps that should succeed whereas if I return -EEXIST here then that would propagate back up through the second mapping hypercall.

  Paul

> Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel