| Message ID | 20180510091518.28199-4-paul.durrant@citrix.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen-hvm: use new resource mapping API | expand |
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,
> -----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
On Tue, May 15, 2018 at 04:45:25PM +0100, Paul Durrant wrote: > > > 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. In man errno, I have: ENOTSUP Operation not supported (POSIX.1-2001) EOPNOTSUPP Operation not supported on socket (POSIX.1-2001). ENOSYS Function not implemented (POSIX.1-2001). But I guess any of these would work.
> -----Original Message----- > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > Sent: 15 May 2018 17:17 > 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 Tue, May 15, 2018 at 04:45:25PM +0100, Paul Durrant wrote: > > > > 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. > > In man errno, I have: > ENOTSUP Operation not supported (POSIX.1-2001) > EOPNOTSUPP Operation not supported on socket (POSIX.1-2001). > ENOSYS Function not implemented (POSIX.1-2001). > > But I guess any of these would work. My reference is the non-Linux definitions in tools/libs/foreignmemory/private.h in the Xen tree. The one for xenforeignmemory_map_resource() is as follows: static inline int osdep_xenforeignmemory_map_resource( xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres) { errno = EOPNOTSUPP; return -1; } So I'll stick with EOPNOTSUPP. Cheers, Paul > > -- > Anthony PERARD
On 05/15/2018 11:16 AM, Anthony PERARD wrote: >>>> + 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. > > In man errno, I have: > ENOTSUP Operation not supported (POSIX.1-2001) > EOPNOTSUPP Operation not supported on socket (POSIX.1-2001). POSIX allows (and Linux exploits) ENOTSUP and EOPNOTSUPP to be synonyms for the same error value. I somewhat prefer the ENOTSUP spelling; and it's probably a bit nicer between the two when porting to platforms where the two spellings are not synonyms. > ENOSYS Function not implemented (POSIX.1-2001). > > But I guess any of these would work. >
diff --git a/configure b/configure index 1443422e83..0f9c2f000e 100755 --- a/configure +++ b/configure @@ -2229,12 +2229,17 @@ EOF #undef XC_WANT_COMPAT_DEVICEMODEL_API #define __XEN_TOOLS__ #include <xendevicemodel.h> +#include <xenforeignmemory.h> int main(void) { xendevicemodel_handle *xd; + xenforeignmemory_handle *xfmem; xd = xendevicemodel_open(0, 0); xendevicemodel_pin_memory_cacheattr(xd, 0, 0, 0, 0); + xfmem = xenforeignmemory_open(0, 0); + xenforeignmemory_map_resource(xfmem, 0, 0, 0, 0, 0, NULL, 0, 0); + return 0; } EOF diff --git a/hw/i386/xen/trace-events b/hw/i386/xen/trace-events index 8dab7bcfe0..38616b698f 100644 --- a/hw/i386/xen/trace-events +++ b/hw/i386/xen/trace-events @@ -15,6 +15,7 @@ cpu_ioreq_pio(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64 cpu_ioreq_pio_read_reg(void *req, uint64_t data, uint64_t addr, uint32_t size) "I/O=%p pio read reg data=0x%"PRIx64" port=0x%"PRIx64" size=%d" cpu_ioreq_pio_write_reg(void *req, uint64_t data, uint64_t addr, uint32_t size) "I/O=%p pio write reg data=0x%"PRIx64" port=0x%"PRIx64" size=%d" cpu_ioreq_move(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p copy dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d" +xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p" # xen-mapcache.c xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64 diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c index 6ffa3c22cc..664cc52532 100644 --- 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, + 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); + 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", @@ -1253,27 +1281,37 @@ static int xen_map_ioreq_server(XenIOState *state) return rc; } - DPRINTF("shared page at pfn %lx\n", ioreq_pfn); - DPRINTF("buffered io page at pfn %lx\n", bufioreq_pfn); - DPRINTF("buffered io evtchn is %x\n", bufioreq_evtchn); - - state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid, - PROT_READ | PROT_WRITE, - 1, &ioreq_pfn, NULL); if (state->shared_page == NULL) { - error_report("map shared IO page returned error %d handle=%p", - errno, xen_xc); - return -1; + DPRINTF("shared page at pfn %lx\n", ioreq_pfn); + + state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid, + PROT_READ | PROT_WRITE, + 1, &ioreq_pfn, NULL); + if (state->shared_page == NULL) { + error_report("map shared IO page returned error %d handle=%p", + errno, xen_xc); + } } - state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid, - PROT_READ | PROT_WRITE, - 1, &bufioreq_pfn, NULL); if (state->buffered_io_page == NULL) { - error_report("map buffered IO page returned error %d", errno); + DPRINTF("buffered io page at pfn %lx\n", bufioreq_pfn); + + state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid, + PROT_READ | PROT_WRITE, + 1, &bufioreq_pfn, + NULL); + if (state->buffered_io_page == NULL) { + error_report("map buffered IO page returned error %d", errno); + return -1; + } + } + + if (state->shared_page == NULL || state->buffered_io_page == NULL) { return -1; } + DPRINTF("buffered io evtchn is %x\n", bufioreq_evtchn); + state->bufioreq_remote_port = bufioreq_evtchn; return 0; 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; + return -1; +} + #endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100 */ #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
Xen 4.11 has a new API to directly map guest resources. Among the resources that can be mapped using this API are ioreq pages. This patch modifies QEMU to attempt to use the new API should it exist, falling back to the previous mechanism if it is unavailable. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Anthony Perard <anthony.perard@citrix.com> --- configure | 5 ++++ hw/i386/xen/trace-events | 1 + hw/i386/xen/xen-hvm.c | 68 +++++++++++++++++++++++++++++++++++---------- include/hw/xen/xen_common.h | 14 ++++++++++ 4 files changed, 73 insertions(+), 15 deletions(-)