Re: [Qemu-devel] [PATCH v2 3/3] xen-hvm: try to use xenforeignmemory_map_resource() to map ioreq pages

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

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 15 May 2018 16:38
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v2 3/3] xen-hvm: try to use
> xenforeignmemory_map_resource() to map ioreq pages
> 
> On Thu, May 10, 2018 at 10:15:18AM +0100, Paul Durrant wrote:
> > --- a/hw/i386/xen/xen-hvm.c
> > +++ b/hw/i386/xen/xen-hvm.c
> > @@ -1239,13 +1239,41 @@ static void xen_wakeup_notifier(Notifier
> *notifier, void *data)
> >
> >  static int xen_map_ioreq_server(XenIOState *state)
> >  {
> > +    void *addr = NULL;
> > +    xenforeignmemory_resource_handle *fres;
> >      xen_pfn_t ioreq_pfn;
> >      xen_pfn_t bufioreq_pfn;
> >      evtchn_port_t bufioreq_evtchn;
> >      int rc;
> >
> > +    /*
> > +     * Attempt to map using the resource API and fall back to normal
> > +     * foreign mapping if this is not supported.
> > +     */
> > +
> QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_bufioreq
> != 0);
> > +
> QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_ioreq(0)
> != 1);
> > +    fres = xenforeignmemory_map_resource(xen_fmem, xen_domid,
> > +                                         XENMEM_resource_ioreq_server,
> 
> XENMEM_resource_ioreq_server undeclared with Xen 4.10

Yes, I missed that from my compat definitions. Will add.

> 
> > +                                         state->ioservid, 0, 2,
> > +                                         &addr,
> > +                                         PROT_READ | PROT_WRITE, 0);
> > +    if (fres != NULL) {
> > +        trace_xen_map_resource_ioreq(state->ioservid, addr);
> > +        state->buffered_io_page = addr;
> > +        state->shared_page = addr + TARGET_PAGE_SIZE;
> > +    } else {
> > +        error_report("failed to map ioreq server resources: error %d
> handle=%p",
> > +                     errno, xen_xc);
> 
> Maybe printing the error message only when
> xenforeignmemory_map_resource
> fails, would be better? i.e. after checking errno value.
> 

Yes, that be better. I'll move it lower.

> > +        if (errno != EOPNOTSUPP) {
> > +            return -1;
> > +        }
> > +    }
> > +
> >      rc = xen_get_ioreq_server_info(xen_domid, state->ioservid,
> > -                                   &ioreq_pfn, &bufioreq_pfn,
> > +                                   (state->shared_page == NULL) ?
> > +                                   &ioreq_pfn : NULL,
> > +                                   (state->buffered_io_page == NULL) ?
> > +                                   &bufioreq_pfn : NULL,
> >                                     &bufioreq_evtchn);
> >      if (rc < 0) {
> >          error_report("failed to get ioreq server info: error %d handle=%p",
> > diff --git a/include/hw/xen/xen_common.h
> b/include/hw/xen/xen_common.h
> > index 5f1402b494..d925751040 100644
> > --- a/include/hw/xen/xen_common.h
> > +++ b/include/hw/xen/xen_common.h
> > @@ -119,6 +119,20 @@ static inline int
> xendevicemodel_pin_memory_cacheattr(
> >      return xc_domain_pin_memory_cacheattr(xen_xc, domid, start, end,
> type);
> >  }
> >
> > +typedef void xenforeignmemory_resource_handle;
> > +
> > +#define XENMEM_resource_ioreq_server_frame_bufioreq 0
> > +#define XENMEM_resource_ioreq_server_frame_ioreq(n) (1 + (n))
> > +
> > +static inline xenforeignmemory_resource_handle
> *xenforeignmemory_map_resource(
> > +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> > +    unsigned int id, unsigned long frame, unsigned long nr_frames,
> > +    void **paddr, int prot, int flags)
> > +{
> > +    errno = EOPNOTSUPP;
> 
> I think ENOSYS would be better. EOPNOTSUPP seems to be for sockets.
> 

No, EOPNOTSUPP is more general than that and is convention for unimplemented API operations elsewhere. ENOSYS is supposed to strictly mean 'system call not implemented' but we use it for hypercalls in Xen, leading to occasional fun with Linux checkpatch.pl.

> 
> > +    return -1;
> 
> Should this return NULL instead?  That doesn't build on Xen 4.10 and earlier.
> 

Indeed it should. Not sure how I missed that.

> > +}
> > +
> >  #endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100 */
> >
> >  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
> 
> Thanks,

Thanks. V3 coming soon.

  Paul

> 
> --
> Anthony PERARD