Skip to contentSkip to conversation

xen-hvm: use new resource mapping API

PATCH Superseded Paul Durrant 3 participants 13 messages May 10, 2018 – May 15, 2018 Checkpatch

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Paul Durrant

author
#0 lore raw

v2 cover xen-hvm: use new resource mapping API

This series modifies QEMU to use the new guest resource mapping API (available in Xen 4.11+) to map ioreq pages.

v2:
- Add a patch to checkpatch to avoid misparsing of Xen stable API handles

Paul Durrant (3):
xen-hvm: create separate function for ioreq server initialization
checkpatch: generalize xen handle matching in the list of types
xen-hvm: try to use xenforeignmemory_map_resource() to map ioreq pages

 configure                   |   5 ++
 hw/i386/xen/trace-events    |   1 +
 hw/i386/xen/xen-hvm.c       | 114 ++++++++++++++++++++++++++++++++------------
 include/hw/xen/xen_common.h |  14 ++++++
 scripts/checkpatch.pl       |   2 +-
 5 files changed, 105 insertions(+), 31 deletions(-)

---
Cc: Anthony Perard <[email protected]>
Cc: Daniel P. Berrange <[email protected]>
Cc: Eric Blake <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Stefano Stabellini <[email protected]>

Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
author
#1 lore raw

PATCH v2 1/3 xen-hvm: create separate function for ioreq server initialization

The code is sufficiently substantial that it improves code readability to put it in a new function called by xen_hvm_init() rather than having it inline.

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Stefano Stabellini <[email protected]>
Cc: Anthony Perard <[email protected]>

---
 hw/i386/xen/xen-hvm.c | 76 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 30 deletions(-)
hw/i386/xen/xen-hvm.c +46 -30
index caa563be3d..6ffa3c22cc 100644
@@ -95,7 +95,8 @@ typedef struct XenIOState {
     CPUState **cpu_by_vcpu_id;
     /* the evtchn port for polling the notification, */
     evtchn_port_t *ioreq_local_port;
-    /* evtchn local port for buffered io */
+    /* evtchn remote and local ports for buffered io */
+    evtchn_port_t bufioreq_remote_port;
     evtchn_port_t bufioreq_local_port;
     /* the evtchn fd for polling */
     xenevtchn_handle *xce_handle;

@@ -1236,12 +1237,52 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
-void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
+static int xen_map_ioreq_server(XenIOState *state)
 {
-    int i, rc;
     xen_pfn_t ioreq_pfn;
     xen_pfn_t bufioreq_pfn;
     evtchn_port_t bufioreq_evtchn;
+    int rc;
+
+    rc = xen_get_ioreq_server_info(xen_domid, state->ioservid,
+                                   &ioreq_pfn, &bufioreq_pfn,
+                                   &bufioreq_evtchn);
+    if (rc < 0) {
+        error_report("failed to get ioreq server info: error %d handle=%p",
+                     errno, xen_xc);
+        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;
+    }
+
+    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;
+    }
+
+    state->bufioreq_remote_port = bufioreq_evtchn;
+
+    return 0;
+}
+
+void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
+{
+    int i, rc;
+    xen_pfn_t ioreq_pfn;
     XenIOState *state;
 
     state = g_malloc0(sizeof (XenIOState));

@@ -1269,25 +1310,8 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
     state->wakeup.notify = xen_wakeup_notifier;
     qemu_register_wakeup_notifier(&state->wakeup);
 
-    rc = xen_get_ioreq_server_info(xen_domid, state->ioservid,
-                                   &ioreq_pfn, &bufioreq_pfn,
-                                   &bufioreq_evtchn);
+    rc = xen_map_ioreq_server(state);
     if (rc < 0) {
-        error_report("failed to get ioreq server info: error %d handle=%p",
-                     errno, xen_xc);
-        goto err;
-    }
-
-    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);
         goto err;
     }
 

@@ -1308,14 +1332,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
         goto err;
     }
 
-    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);
-        goto err;
-    }
-
     /* Note: cpus is empty at this point in init */
     state->cpu_by_vcpu_id = g_malloc0(max_cpus * sizeof(CPUState *));
 

@@ -1340,7 +1356,7 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
     }
 
     rc = xenevtchn_bind_interdomain(state->xce_handle, xen_domid,
-                                    bufioreq_evtchn);
+                                    state->bufioreq_remote_port);
     if (rc == -1) {
         error_report("buffered evtchn bind error %d", errno);
         goto err;
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Reviewed-by on v2 1/3
#2 lore raw

Re: [Qemu-devel] [PATCH v2 1/3] xen-hvm: create separate function for ioreq server initialization

Quoted 5 lines · Paul Durrant

The code is sufficiently substantial that it improves code readability to put it in a new function called by xen_hvm_init() rather than having it inline.

Signed-off-by Paul Durrant <[email protected]>
Reviewed-by Anthony PERARD <[email protected]>
Signature · 6 lines
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
author
#3 lore raw

PATCH v2 2/3 checkpatch: generalize xen handle matching in the list of types

In reply to Paul Durrant

All the xen stable APIs define handle types of the form:

<name of API>_handle

and some define additional handle types of the form:

<name of API>_<purpose of handle>_handle

Examples of these are xenforeignmemory_handle and
xenforeignmemory_resource_handle.

Both of these types will be misparsed by checkpatch if they appear as the first token in a line since, as types defined by an external library, they do not conform to the QEMU CODING_STYLE, which suggests CamelCase.

A previous patch (5ac067a24a8) added xendevicemodel_handle to the list of types. This patch changes that to xen\w+_handle such that it will match all Xen stable API handles of the forms detailed above.

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Eric Blake <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Daniel P. Berrange <[email protected]>

v2:
- New in this version

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
index 5b8735defb..98ed799f29 100755
@@ -266,7 +266,7 @@ our @typeList = (
 	qr{target_(?:u)?long},
 	qr{hwaddr},
 	qr{xml${Ident}},
-	qr{xendevicemodel_handle},
+	qr{xen\w+_handle},
 );
 
 # This can be modified by sub possible.  Since it can be empty, be careful
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Reviewed-by on v2 2/3
#4 lore raw

Re: [PATCH v2 2/3] checkpatch: generalize xen handle matching in the list of types

Quoted 7 lines · Paul Durrant

All the xen stable APIs define handle types of the form:

<name of API>_handle

and some define additional handle types of the form:

<name of API>_<purpose of handle>_handle

Maybe worth mentioning that <name of API> always has a 'xen' prefix, and/or spelling it:

xen<name of object>_handle
xen<name of object>_<purpose>_handle

Quoted 11 lines

Examples of these are xenforeignmemory_handle and
xenforeignmemory_resource_handle.

Both of these types will be misparsed by checkpatch if they appear as the first token in a line since, as types defined by an external library, they do not conform to the QEMU CODING_STYLE, which suggests CamelCase.

A previous patch (5ac067a24a8) added xendevicemodel_handle to the list of types. This patch changes that to xen\w+_handle such that it will match all Xen stable API handles of the forms detailed above.

Nice use of a regex.

Quoted 9 lines
Signed-off-by Paul Durrant <[email protected]>

---
Cc: Eric Blake <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Daniel P. Berrange <[email protected]>

v2:
- New in this version

Reviewed-by Eric Blake <[email protected]>
Quoted 18 lines
---
  scripts/checkpatch.pl | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
index 5b8735defb..98ed799f29 100755
@@ -266,7 +266,7 @@ our @typeList = (
  	qr{target_(?:u)?long},
  	qr{hwaddr},
  	qr{xml${Ident}},
-	qr{xendevicemodel_handle},
+	qr{xen\w+_handle},
  );
 
  # This can be modified by sub possible.  Since it can be empty, be careful
Signature · 8 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Reviewed-by on v2 2/3
#5 lore raw

Re: [Qemu-devel] [PATCH v2 2/3] checkpatch: generalize xen handle matching in the list of types

In reply to Paul Durrant
Quoted 7 lines · Paul Durrant

All the xen stable APIs define handle types of the form:

<name of API>_handle

and some define additional handle types of the form:

<name of API>_<purpose of handle>_handle

Maybe worth mentioning that <name of API> always has a 'xen' prefix, and/or spelling it:

xen<name of object>_handle
xen<name of object>_<purpose>_handle

Quoted 11 lines

Examples of these are xenforeignmemory_handle and
xenforeignmemory_resource_handle.

Both of these types will be misparsed by checkpatch if they appear as the first token in a line since, as types defined by an external library, they do not conform to the QEMU CODING_STYLE, which suggests CamelCase.

A previous patch (5ac067a24a8) added xendevicemodel_handle to the list of types. This patch changes that to xen\w+_handle such that it will match all Xen stable API handles of the forms detailed above.

Nice use of a regex.

Quoted 9 lines
Signed-off-by Paul Durrant <[email protected]>

---
Cc: Eric Blake <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Daniel P. Berrange <[email protected]>

v2:
- New in this version

Reviewed-by Eric Blake <[email protected]>
Quoted 18 lines
---
  scripts/checkpatch.pl | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
index 5b8735defb..98ed799f29 100755
@@ -266,7 +266,7 @@ our @typeList = (
  	qr{target_(?:u)?long},
  	qr{hwaddr},
  	qr{xml${Ident}},
-	qr{xendevicemodel_handle},
+	qr{xen\w+_handle},
  );
 
  # This can be modified by sub possible.  Since it can be empty, be careful
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#6 lore raw

PATCH v2 3/3 xen-hvm: try to use xenforeignmemory_map_resource() to map ioreq pages

In reply to Paul Durrant

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 <[email protected]>

---
Cc: Stefano Stabellini <[email protected]>
Cc: Anthony Perard <[email protected]>

---
 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(-)
configure +5 -0
index 1443422e83..0f9c2f000e 100755
@@ -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
hw/i386/xen/trace-events +1 -0
index 8dab7bcfe0..38616b698f 100644
@@ -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
hw/i386/xen/xen-hvm.c +53 -15

93 lines not on this page · raw message

include/hw/xen/xen_common.h +14 -0
index 5f1402b494..d925751040 100644
@@ -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
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#7 lore raw

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

Quoted 11 lines · Paul Durrant
hw/i386/xen/xen-hvm.c +2 -0
@@ -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;

hw/i386/xen/xen-hvm.c static void xen_wakeup_notifier(Notifier *notifier, void *data) · line 1256

Quoted 10 lines
hw/i386/xen/xen-hvm.c +8 -0
     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

hw/i386/xen/xen-hvm.c static void xen_wakeup_notifier(Notifier *notifier, void *data) · line 1266

Quoted 10 lines
hw/i386/xen/xen-hvm.c +10 -0
+                                         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.

Quoted 23 lines
hw/i386/xen/xen-hvm.c +9 -1
+        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",
include/hw/xen/xen_common.h +1 -0
index 5f1402b494..d925751040 100644
@@ -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;

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr( · line 132

Quoted 10 lines
include/hw/xen/xen_common.h +10 -0
+
+#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.

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr( · line 133

Quoted 1 line
include/hw/xen/xen_common.h +1 -0
+    return -1;

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

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr( · line 138

Quoted 5 lines
include/hw/xen/xen_common.h +2 -0
+}
+
 #endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100 */
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000

Thanks,

Signature · 6 lines
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
author
#8 lore raw

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

Quoted 31 lines

-----Original Message-----
From: Anthony PERARD [mailto:[email protected]]
Sent: 15 May 2018 16:38
To: Paul Durrant <[email protected]>
Cc: [email protected]; [email protected]; Stefano Stabellini <[email protected]>
Subject: Re: [PATCH v2 3/3] xen-hvm: try to use xenforeignmemory_map_resource() to map ioreq pages

hw/i386/xen/xen-hvm.c static void xen_wakeup_notifier(Notifier

Quoted 3 lines · Paul Durrant
hw/i386/xen/xen-hvm.c +0 -0
@@ -1239,13 +1239,41 @@ static void xen_wakeup_notifier(Notifier

*notifier, void *data)

Quoted 5 lines
hw/i386/xen/xen-hvm.c +2 -0
 static int xen_map_ioreq_server(XenIOState *state)
 {
+    void *addr = NULL;
+    xenforeignmemory_resource_handle *fres;

hw/i386/xen/xen-hvm.c static void xen_wakeup_notifier(Notifier *notifier, void *data) · line 1271

Quoted 10 lines
hw/i386/xen/xen-hvm.c +5 -0
     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);

Quoted 7 lines

hw/i386/xen/xen-hvm.c static void xen_wakeup_notifier(Notifier · line 1253

Quoted 1 line
hw/i386/xen/xen-hvm.c +1 -0
+

QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_ioreq(0) != 1);

hw/i386/xen/xen-hvm.c static void xen_wakeup_notifier(Notifier *notifier, void *data) · line 1256

Quoted 2 lines
hw/i386/xen/xen-hvm.c +2 -0
+    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.

Quoted 10 lines

hw/i386/xen/xen-hvm.c static void xen_wakeup_notifier(Notifier *notifier, void *data)

Quoted 10 lines
hw/i386/xen/xen-hvm.c +9 -0
+                                         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
Quoted 7 lines

handle=%p",

hw/i386/xen/xen-hvm.c static void xen_wakeup_notifier(Notifier · line 1265

Quoted 1 line
hw/i386/xen/xen-hvm.c +1 -0
+                     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.

Quoted 32 lines

Review

Quoted 15 lines
hw/i386/xen/xen-hvm.c +9 -1
+        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",
(quoted diff) +0 -0

b/include/hw/xen/xen_common.h

include/hw/xen/xen_common.h static inline int

Quoted 4 lines

index 5f1402b494..d925751040 100644

include/hw/xen/xen_common.h +0 -0
@@ -119,6 +119,20 @@ static inline int

xendevicemodel_pin_memory_cacheattr(

include/hw/xen/xen_common.h static inline int · line 119

Quoted 1 line
include/hw/xen/xen_common.h +0 -0
     return xc_domain_pin_memory_cacheattr(xen_xc, domid, start, end,

type);

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr(

Quoted 8 lines
include/hw/xen/xen_common.h +6 -0
 }
 
+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(

Quoted 8 lines

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr( · line 132

Quoted 5 lines
include/hw/xen/xen_common.h +5 -0
+    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.

Quoted 5 lines

include/hw/xen/xen_common.h static inline int · line 133

Quoted 2 lines
include/hw/xen/xen_common.h +1 -0
+    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.

Quoted 7 lines

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr( · line 138

Quoted 5 lines
include/hw/xen/xen_common.h +2 -0
+}
+
 #endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100 */
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000

Thanks,

Thanks. V3 coming soon.

Paul

Quoted 3 lines

--
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#9 lore raw

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

Quoted 24 lines · Paul Durrant
Quoted 24 lines

Review

Quoted 1 line
(quoted diff) +0 -0

b/include/hw/xen/xen_common.h

include/hw/xen/xen_common.h static inline int

Quoted 4 lines

index 5f1402b494..d925751040 100644

include/hw/xen/xen_common.h +0 -0
@@ -119,6 +119,20 @@ static inline int

xendevicemodel_pin_memory_cacheattr(

include/hw/xen/xen_common.h static inline int · line 119

Quoted 1 line
include/hw/xen/xen_common.h +0 -0
     return xc_domain_pin_memory_cacheattr(xen_xc, domid, start, end,

type);

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr(

Quoted 8 lines
include/hw/xen/xen_common.h +6 -0
 }
 
+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(

Quoted 5 lines
include/hw/xen/xen_common.h +5 -0
+    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;
Quoted 4 lines
Quoted 2 lines

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.

Signature · 6 lines
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
author
#10 lore raw

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

Quoted 44 lines

-----Original Message-----
From: Anthony PERARD [mailto:[email protected]]
Sent: 15 May 2018 17:17
To: Paul Durrant <[email protected]>
Cc: [email protected]; [email protected]; Stefano Stabellini <[email protected]>
Subject: Re: [PATCH v2 3/3] xen-hvm: try to use xenforeignmemory_map_resource() to map ioreq pages

Quoted 8 lines · Paul Durrant
Quoted 8 lines

Review

Quoted 1 line
(quoted diff) +0 -0

b/include/hw/xen/xen_common.h

include/hw/xen/xen_common.h static inline int

Quoted 4 lines

index 5f1402b494..d925751040 100644

include/hw/xen/xen_common.h +0 -0
@@ -119,6 +119,20 @@ static inline int

xendevicemodel_pin_memory_cacheattr(

Quoted 1 line
include/hw/xen/xen_common.h +0 -0
     return xc_domain_pin_memory_cacheattr(xen_xc, domid, start,

end,

Quoted 11 lines
Quoted 11 lines

type);

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr(

Quoted 8 lines
include/hw/xen/xen_common.h +6 -0
 }
 
+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(

Quoted 1 line
include/hw/xen/xen_common.h +1 -0
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int

type,

Quoted 9 lines
Quoted 7 lines

include/hw/xen/xen_common.h static inline int xendevicemodel_pin_memory_cacheattr( · line 132

Quoted 4 lines
include/hw/xen/xen_common.h +4 -0
+    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.

Quoted 6 lines

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

Quoted 3 lines

--
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#11 lore raw

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

In reply to Anthony PERARD
Quoted 10 lines · Anthony PERARD
Quoted 6 lines
Quoted 4 lines
Quoted 1 line

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

Quoted 4 lines

ENOSYS Function not implemented (POSIX.1-2001).

But I guess any of these would work.

Signature · 8 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
author
#12 lore raw
In reply to Paul Durrant

Anthony, Stefano,

Ping?

I'd like to get your feedback before spinning a v3 to incorporate Eric's suggestion regarding the commit comment for #2.

Paul

Quoted 36 lines

-----Original Message-----
From: Paul Durrant [mailto:[email protected]]
Sent: 10 May 2018 10:15
To: [email protected]; [email protected]
Cc: Paul Durrant <[email protected]>; Anthony Perard <[email protected]>; Daniel P . Berrange <[email protected]>; Eric Blake <[email protected]>; Paolo Bonzini <[email protected]>; Stefano Stabellini <[email protected]>
Subject: [PATCH v2 0/2] xen-hvm: use new resource mapping API

This series modifies QEMU to use the new guest resource mapping API (available in Xen 4.11+) to map ioreq pages.

v2:
- Add a patch to checkpatch to avoid misparsing of Xen stable API handles

Paul Durrant (3):
xen-hvm: create separate function for ioreq server initialization
checkpatch: generalize xen handle matching in the list of types
xen-hvm: try to use xenforeignmemory_map_resource() to map ioreq
pages

 configure                   |   5 ++
 hw/i386/xen/trace-events    |   1 +
 hw/i386/xen/xen-hvm.c       | 114 ++++++++++++++++++++++++++++++++-
-----------
 include/hw/xen/xen_common.h |  14 ++++++
 scripts/checkpatch.pl       |   2 +-
 5 files changed, 105 insertions(+), 31 deletions(-)

---
Cc: Anthony Perard <[email protected]>
Cc: Daniel P. Berrange <[email protected]>
Cc: Eric Blake <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Stefano Stabellini <[email protected]>

Quoted 2 lines

--
2.11.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel