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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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>? 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())?

> + 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