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

[Anthony PERARD <anthony.perard@citrix.com>]

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

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

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


> +    return -1;

Should this return NULL instead?  That doesn't build on Xen 4.10 and earlier.

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

Thanks,

-- 
Anthony PERARD