Re: [PATCH v11 06/11] x86/hvm/ioreq: add a new mappable resource type...

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 16 October 2017 15:07
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v11 06/11] x86/hvm/ioreq: add a new
> mappable resource type...
> 
> >>> On 12.10.17 at 18:25, <paul.durrant@citrix.com> wrote:
> > ... XENMEM_resource_ioreq_server
> >
> > This patch adds support for a new resource type that can be mapped using
> > the XENMEM_acquire_resource memory op.
> >
> > If an emulator makes use of this resource type then, instead of mapping
> > gfns, the IOREQ server will allocate pages from the heap. These pages
> > will never be present in the P2M of the guest at any point and so are
> > not vulnerable to any direct attack by the guest. They are only ever
> > accessible by Xen and any domain that has mapping privilege over the
> > guest (which may or may not be limited to the domain running the
> emulator).
> >
> > NOTE: Use of the new resource type is not compatible with use of
> >       XEN_DMOP_get_ioreq_server_info unless the XEN_DMOP_no_gfns
> flag is
> >       set.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Acked-by: George Dunlap <George.Dunlap@eu.citrix.com>
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> Can you have validly retained this?

I didn't think the structure of this particular patch had changed that fundamentally.

> 
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -281,6 +294,69 @@ static int hvm_map_ioreq_gfn(struct
> hvm_ioreq_server *s, bool buf)
> >      return rc;
> >  }
> >
> > +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 )
> > +    {
> > +        /*
> > +         * If a guest frame has already been mapped (which may happen
> > +         * on demand if hvm_get_ioreq_server_info() is called), then
> > +         * allocating a page is not permitted.
> > +         */
> > +        if ( !gfn_eq(iorp->gfn, INVALID_GFN) )
> > +            return -EPERM;
> > +
> > +        return 0;
> > +    }
> > +
> > +    /*
> > +     * Allocated IOREQ server pages are assigned to the emulating
> > +     * domain, not the target domain. This is because the emulator is
> > +     * likely to be destroyed after the target domain has been torn
> > +     * down, and we must use MEMF_no_refcount otherwise page
> allocation
> > +     * could fail if the emulating domain has already reached its
> > +     * maximum allocation.
> > +     */
> > +    iorp->page = alloc_domheap_page(currd, MEMF_no_refcount);
> > +    if ( !iorp->page )
> > +        return -ENOMEM;
> > +
> > +    if ( !get_page_type(iorp->page, PGT_writable_page) )
> > +    {
> 
> ASSERT_UNREACHABLE() ?

Ok.

> 
> > @@ -777,6 +886,51 @@ int hvm_get_ioreq_server_info(struct domain *d,
> ioservid_t id,
> >      return rc;
> >  }
> >
> > +int hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id,
> > +                               unsigned long idx, mfn_t *mfn)
> > +{
> > +    struct hvm_ioreq_server *s;
> > +    int rc;
> > +
> > +    spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
> > +
> > +    if ( id == DEFAULT_IOSERVID )
> > +        return -EOPNOTSUPP;
> > +
> > +    s = get_ioreq_server(d, id);
> > +
> > +    ASSERT(!IS_DEFAULT(s));
> > +
> > +    rc = hvm_ioreq_server_alloc_pages(s);
> > +    if ( rc )
> > +        goto out;
> > +
> > +    switch ( idx )
> > +    {
> > +    case XENMEM_resource_ioreq_server_frame_bufioreq:
> > +        rc = -ENOENT;
> > +        if ( !HANDLE_BUFIOREQ(s) )
> > +            goto out;
> > +
> > +        *mfn = _mfn(page_to_mfn(s->bufioreq.page));
> > +        rc = 0;
> > +        break;
> 
> How about
> 
>         if ( HANDLE_BUFIOREQ(s) )
>             *mfn = _mfn(page_to_mfn(s->bufioreq.page));
>         else
>             rc = -ENOENT;
>         break;
> 

Looking at the overall structure I prefer it as it is. If I could have got rid of the out label by doing this then it might have been worth the change.

> ?
> 
> > +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;
> 
> This now doesn't match up with the upper bound's type.
> 

Ok.

> > @@ -629,6 +634,10 @@ struct xen_mem_acquire_resource {
> >       *      is optional if nr_frames is 0.
> >       */
> >      uint64_aligned_t frame;
> > +
> > +#define XENMEM_resource_ioreq_server_frame_bufioreq 0
> > +#define XENMEM_resource_ioreq_server_frame_ioreq(n_) (1 + (n_))
> 
> I don't see what you need the trailing underscore for. This is
> normally only needed on local variables defined in (gcc extended)
> macros, which we generally can't use in a public header anyway.
> 

I thought it was generally desirable to attempt to distinguish macro arguments from variable to avoid name clashes. What do you prefer I should do in a public header?

  Paul

> Jan

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