Re: [Xen-devel] [PATCH v7 12/12] x86/hvm/ioreq: add a new mappable resource type...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 26 September 2017 13:59
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH v7 12/12] x86/hvm/ioreq: add a new mappable resource
> type...
> 
> >>> On 18.09.17 at 17:31, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -762,7 +863,8 @@ int hvm_get_ioreq_server_info(struct domain *d,
> ioservid_t id,
> >              goto out;
> >      }
> >
> > -    *ioreq_gfn = gfn_x(s->ioreq.gfn);
> > +    if ( ioreq_gfn )
> > +        *ioreq_gfn = gfn_x(s->ioreq.gfn);
> 
> Ah, this is what actually wants to be in patch 11. Considering what
> you say in the description regarding the XEN_DMOP_no_gfns I
> wonder whether you wouldn't better return "invalid" indicators in
> the GFN output fields of the hypercall when the pages haven't
> been mapped to a GFN.
> 
> > @@ -780,6 +882,33 @@ int hvm_get_ioreq_server_info(struct domain *d,
> ioservid_t id,
> >      return rc;
> >  }
> >
> > +mfn_t hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id,
> > +                                 unsigned int idx)
> > +{
> > +    struct hvm_ioreq_server *s;
> > +    mfn_t mfn = INVALID_MFN;
> > +
> > +    spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
> > +
> > +    s = get_ioreq_server(d, id);
> > +
> > +    if ( id >= MAX_NR_IOREQ_SERVERS || !s || IS_DEFAULT(s) )
> > +        goto out;
> > +
> > +    if ( hvm_ioreq_server_alloc_pages(s) )
> > +        goto out;
> > +
> > +    if ( idx == 0 )
> > +        mfn = _mfn(page_to_mfn(s->bufioreq.page));
> > +    else if ( idx == 1 )
> > +        mfn = _mfn(page_to_mfn(s->ioreq.page));
> 
> else <some sort of error>?

I set mfn to INVALID above. Is that not enough?

> Also with buffered I/O being optional,
> wouldn't it be more natural for index 0 representing the synchronous
> page? And with buffered I/O not enabled, aren't you returning
> rubbish (NULL translated by page_to_mfn())?

Good point. I should leave the mfn set to invalid if the buffered page is not 
there. As for making it zero, and putting the synchronous ones afterwards, that 
was intentional because more synchronous pages will need to be added if we 
needs to support more vCPUs, whereas there should only ever need to be one 
buffered page.

  Paul

> 
> > + out:
> > +    spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
> > +
> > +    return mfn;
> 
> The unspecific error (INVALID_MFN) here makes me wonder ...
> 
> > @@ -4795,6 +4796,27 @@ static int xenmem_acquire_grant_table(struct
> domain *d,
> >      return 0;
> >  }
> >
> > +static int xenmem_acquire_ioreq_server(struct domain *d,
> > +                                       unsigned int id,
> > +                                       unsigned long frame,
> > +                                       unsigned long nr_frames,
> > +                                       unsigned long mfn_list[])
> > +{
> > +    unsigned int i;
> > +
> > +    for ( i = 0; i < nr_frames; i++ )
> > +    {
> > +        mfn_t mfn = hvm_get_ioreq_server_frame(d, id, frame + i);
> > +
> > +        if ( mfn_eq(mfn, INVALID_MFN) )
> > +            return -EINVAL;
> 
> ... how meaningful EINVAL here is. In particular if page allocation
> failed, ENOMEM would certainly be more appropriate (and give the
> caller a better idea of what needs to be done).
> 
> Jan


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