[PATCH v2 00/19] xen: simplify frontend side ring setup

[PATCH v2 00/19] xen: simplify frontend side ring setup

[Juergen Gross]

Many Xen PV frontends share similar code for setting up a ring page
(allocating and granting access for the backend) and for tearing it
down.

Create new service functions doing all needed steps in one go.

This requires all frontends to use a common value for an invalid
grant reference in order to make the functions idempotent.

Changes in V2:
- new patch 9 and related changes in patches 10-18

Juergen Gross (19):
  xen/blkfront: switch blkfront to use INVALID_GRANT_REF
  xen/netfront: switch netfront to use INVALID_GRANT_REF
  xen/scsifront: remove unused GRANT_INVALID_REF definition
  xen/usb: switch xen-hcd to use INVALID_GRANT_REF
  xen/drm: switch xen_drm_front to use INVALID_GRANT_REF
  xen/sound: switch xen_snd_front to use INVALID_GRANT_REF
  xen/dmabuf: switch gntdev-dmabuf to use INVALID_GRANT_REF
  xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF
  xen: update ring.h
  xen/xenbus: add xenbus_setup_ring() service function
  xen/blkfront: use xenbus_setup_ring() and xenbus_teardown_ring()
  xen/netfront: use xenbus_setup_ring() and xenbus_teardown_ring()
  xen/tpmfront: use xenbus_setup_ring() and xenbus_teardown_ring()
  xen/drmfront: use xenbus_setup_ring() and xenbus_teardown_ring()
  xen/pcifront: use xenbus_setup_ring() and xenbus_teardown_ring()
  xen/scsifront: use xenbus_setup_ring() and xenbus_teardown_ring()
  xen/usbfront: use xenbus_setup_ring() and xenbus_teardown_ring()
  xen/sndfront: use xenbus_setup_ring() and xenbus_teardown_ring()
  xen/xenbus: eliminate xenbus_grant_ring()

 drivers/block/xen-blkfront.c                | 57 +++++---------
 drivers/char/tpm/xen-tpmfront.c             | 18 +----
 drivers/gpu/drm/xen/xen_drm_front.h         |  9 ---
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.c | 43 +++--------
 drivers/net/xen-netfront.c                  | 85 +++++++--------------
 drivers/pci/xen-pcifront.c                  | 19 +----
 drivers/scsi/xen-scsifront.c                | 31 ++------
 drivers/usb/host/xen-hcd.c                  | 65 ++++------------
 drivers/xen/gntdev-dmabuf.c                 | 13 +---
 drivers/xen/xen-front-pgdir-shbuf.c         | 17 +----
 drivers/xen/xenbus/xenbus_client.c          | 82 +++++++++++++++-----
 include/xen/interface/io/ring.h             | 19 +++--
 include/xen/xenbus.h                        |  4 +-
 sound/xen/xen_snd_front_evtchnl.c           | 44 +++--------
 sound/xen/xen_snd_front_evtchnl.h           |  9 ---
 15 files changed, 179 insertions(+), 336 deletions(-)

-- 
2.34.1

[PATCH v2 01/19] xen/blkfront: switch blkfront to use INVALID_GRANT_REF

[Juergen Gross]

Instead of using a private macro for an invalid grant reference use
the common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/block/xen-blkfront.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 003056d4f7f5..7f35e30e626a 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -229,8 +229,6 @@ static unsigned int nr_minors;
 static unsigned long *minors;
 static DEFINE_SPINLOCK(minor_lock);
 
-#define GRANT_INVALID_REF	0
-
 #define PARTS_PER_DISK		16
 #define PARTS_PER_EXT_DISK      256
 
@@ -321,7 +319,7 @@ static int fill_grant_buffer(struct blkfront_ring_info *rinfo, int num)
 			gnt_list_entry->page = granted_page;
 		}
 
-		gnt_list_entry->gref = GRANT_INVALID_REF;
+		gnt_list_entry->gref = INVALID_GRANT_REF;
 		list_add(&gnt_list_entry->node, &rinfo->grants);
 		i++;
 	}
@@ -350,7 +348,7 @@ static struct grant *get_free_grant(struct blkfront_ring_info *rinfo)
 					  node);
 	list_del(&gnt_list_entry->node);
 
-	if (gnt_list_entry->gref != GRANT_INVALID_REF)
+	if (gnt_list_entry->gref != INVALID_GRANT_REF)
 		rinfo->persistent_gnts_c--;
 
 	return gnt_list_entry;
@@ -372,7 +370,7 @@ static struct grant *get_grant(grant_ref_t *gref_head,
 	struct grant *gnt_list_entry = get_free_grant(rinfo);
 	struct blkfront_info *info = rinfo->dev_info;
 
-	if (gnt_list_entry->gref != GRANT_INVALID_REF)
+	if (gnt_list_entry->gref != INVALID_GRANT_REF)
 		return gnt_list_entry;
 
 	/* Assign a gref to this page */
@@ -396,7 +394,7 @@ static struct grant *get_indirect_grant(grant_ref_t *gref_head,
 	struct grant *gnt_list_entry = get_free_grant(rinfo);
 	struct blkfront_info *info = rinfo->dev_info;
 
-	if (gnt_list_entry->gref != GRANT_INVALID_REF)
+	if (gnt_list_entry->gref != INVALID_GRANT_REF)
 		return gnt_list_entry;
 
 	/* Assign a gref to this page */
@@ -1221,7 +1219,7 @@ static void blkif_free_ring(struct blkfront_ring_info *rinfo)
 		list_for_each_entry_safe(persistent_gnt, n,
 					 &rinfo->grants, node) {
 			list_del(&persistent_gnt->node);
-			if (persistent_gnt->gref != GRANT_INVALID_REF) {
+			if (persistent_gnt->gref != INVALID_GRANT_REF) {
 				gnttab_end_foreign_access(persistent_gnt->gref,
 							  0UL);
 				rinfo->persistent_gnts_c--;
@@ -1283,9 +1281,9 @@ static void blkif_free_ring(struct blkfront_ring_info *rinfo)
 
 	/* Free resources associated with old device channel. */
 	for (i = 0; i < info->nr_ring_pages; i++) {
-		if (rinfo->ring_ref[i] != GRANT_INVALID_REF) {
+		if (rinfo->ring_ref[i] != INVALID_GRANT_REF) {
 			gnttab_end_foreign_access(rinfo->ring_ref[i], 0);
-			rinfo->ring_ref[i] = GRANT_INVALID_REF;
+			rinfo->ring_ref[i] = INVALID_GRANT_REF;
 		}
 	}
 	free_pages_exact(rinfo->ring.sring,
@@ -1475,7 +1473,7 @@ static int blkif_completion(unsigned long *id,
 			 * to the tail of the list, so it will not be picked
 			 * again unless we run out of persistent grants.
 			 */
-			s->grants_used[i]->gref = GRANT_INVALID_REF;
+			s->grants_used[i]->gref = INVALID_GRANT_REF;
 			list_add_tail(&s->grants_used[i]->node, &rinfo->grants);
 		}
 	}
@@ -1500,7 +1498,7 @@ static int blkif_completion(unsigned long *id,
 					indirect_page = s->indirect_grants[i]->page;
 					list_add(&indirect_page->lru, &rinfo->indirect_pages);
 				}
-				s->indirect_grants[i]->gref = GRANT_INVALID_REF;
+				s->indirect_grants[i]->gref = INVALID_GRANT_REF;
 				list_add_tail(&s->indirect_grants[i]->node, &rinfo->grants);
 			}
 		}
@@ -1687,7 +1685,7 @@ static int setup_blkring(struct xenbus_device *dev,
 	grant_ref_t gref[XENBUS_MAX_RING_GRANTS];
 
 	for (i = 0; i < info->nr_ring_pages; i++)
-		rinfo->ring_ref[i] = GRANT_INVALID_REF;
+		rinfo->ring_ref[i] = INVALID_GRANT_REF;
 
 	sring = alloc_pages_exact(ring_size, GFP_NOIO);
 	if (!sring) {
@@ -2544,13 +2542,13 @@ static void purge_persistent_grants(struct blkfront_info *info)
 
 		list_for_each_entry_safe(gnt_list_entry, tmp, &rinfo->grants,
 					 node) {
-			if (gnt_list_entry->gref == GRANT_INVALID_REF ||
+			if (gnt_list_entry->gref == INVALID_GRANT_REF ||
 			    !gnttab_try_end_foreign_access(gnt_list_entry->gref))
 				continue;
 
 			list_del(&gnt_list_entry->node);
 			rinfo->persistent_gnts_c--;
-			gnt_list_entry->gref = GRANT_INVALID_REF;
+			gnt_list_entry->gref = INVALID_GRANT_REF;
 			list_add_tail(&gnt_list_entry->node, &grants);
 		}
 
-- 
2.34.1

[PATCH v2 02/19] xen/netfront: switch netfront to use INVALID_GRANT_REF

[Juergen Gross]

Instead of using a private macro for an invalid grant reference use
the common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/net/xen-netfront.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e2b4a1893a13..af3d3de7d9fa 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -78,8 +78,6 @@ struct netfront_cb {
 
 #define RX_COPY_THRESHOLD 256
 
-#define GRANT_INVALID_REF	0
-
 #define NET_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, XEN_PAGE_SIZE)
 #define NET_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, XEN_PAGE_SIZE)
 
@@ -224,7 +222,7 @@ static grant_ref_t xennet_get_rx_ref(struct netfront_queue *queue,
 {
 	int i = xennet_rxidx(ri);
 	grant_ref_t ref = queue->grant_rx_ref[i];
-	queue->grant_rx_ref[i] = GRANT_INVALID_REF;
+	queue->grant_rx_ref[i] = INVALID_GRANT_REF;
 	return ref;
 }
 
@@ -432,7 +430,7 @@ static bool xennet_tx_buf_gc(struct netfront_queue *queue)
 			}
 			gnttab_release_grant_reference(
 				&queue->gref_tx_head, queue->grant_tx_ref[id]);
-			queue->grant_tx_ref[id] = GRANT_INVALID_REF;
+			queue->grant_tx_ref[id] = INVALID_GRANT_REF;
 			queue->grant_tx_page[id] = NULL;
 			add_id_to_list(&queue->tx_skb_freelist, queue->tx_link, id);
 			dev_kfree_skb_irq(skb);
@@ -1021,7 +1019,7 @@ static int xennet_get_responses(struct netfront_queue *queue,
 		 * the backend driver. In future this should flag the bad
 		 * situation to the system controller to reboot the backend.
 		 */
-		if (ref == GRANT_INVALID_REF) {
+		if (ref == INVALID_GRANT_REF) {
 			if (net_ratelimit())
 				dev_warn(dev, "Bad rx response id %d.\n",
 					 rx->id);
@@ -1390,7 +1388,7 @@ static void xennet_release_tx_bufs(struct netfront_queue *queue)
 		gnttab_end_foreign_access(queue->grant_tx_ref[i],
 					  (unsigned long)page_address(queue->grant_tx_page[i]));
 		queue->grant_tx_page[i] = NULL;
-		queue->grant_tx_ref[i] = GRANT_INVALID_REF;
+		queue->grant_tx_ref[i] = INVALID_GRANT_REF;
 		add_id_to_list(&queue->tx_skb_freelist, queue->tx_link, i);
 		dev_kfree_skb_irq(skb);
 	}
@@ -1411,7 +1409,7 @@ static void xennet_release_rx_bufs(struct netfront_queue *queue)
 			continue;
 
 		ref = queue->grant_rx_ref[id];
-		if (ref == GRANT_INVALID_REF)
+		if (ref == INVALID_GRANT_REF)
 			continue;
 
 		page = skb_frag_page(&skb_shinfo(skb)->frags[0]);
@@ -1422,7 +1420,7 @@ static void xennet_release_rx_bufs(struct netfront_queue *queue)
 		get_page(page);
 		gnttab_end_foreign_access(ref,
 					  (unsigned long)page_address(page));
-		queue->grant_rx_ref[id] = GRANT_INVALID_REF;
+		queue->grant_rx_ref[id] = INVALID_GRANT_REF;
 
 		kfree_skb(skb);
 	}
@@ -1761,7 +1759,7 @@ static int netfront_probe(struct xenbus_device *dev,
 static void xennet_end_access(int ref, void *page)
 {
 	/* This frees the page as a side-effect */
-	if (ref != GRANT_INVALID_REF)
+	if (ref != INVALID_GRANT_REF)
 		gnttab_end_foreign_access(ref, (unsigned long)page);
 }
 
@@ -1798,8 +1796,8 @@ static void xennet_disconnect_backend(struct netfront_info *info)
 		xennet_end_access(queue->tx_ring_ref, queue->tx.sring);
 		xennet_end_access(queue->rx_ring_ref, queue->rx.sring);
 
-		queue->tx_ring_ref = GRANT_INVALID_REF;
-		queue->rx_ring_ref = GRANT_INVALID_REF;
+		queue->tx_ring_ref = INVALID_GRANT_REF;
+		queue->rx_ring_ref = INVALID_GRANT_REF;
 		queue->tx.sring = NULL;
 		queue->rx.sring = NULL;
 
@@ -1927,8 +1925,8 @@ static int setup_netfront(struct xenbus_device *dev,
 	grant_ref_t gref;
 	int err;
 
-	queue->tx_ring_ref = GRANT_INVALID_REF;
-	queue->rx_ring_ref = GRANT_INVALID_REF;
+	queue->tx_ring_ref = INVALID_GRANT_REF;
+	queue->rx_ring_ref = INVALID_GRANT_REF;
 	queue->rx.sring = NULL;
 	queue->tx.sring = NULL;
 
@@ -1978,17 +1976,17 @@ static int setup_netfront(struct xenbus_device *dev,
 	 * granted pages because backend is not accessing it at this point.
 	 */
  fail:
-	if (queue->rx_ring_ref != GRANT_INVALID_REF) {
+	if (queue->rx_ring_ref != INVALID_GRANT_REF) {
 		gnttab_end_foreign_access(queue->rx_ring_ref,
 					  (unsigned long)rxs);
-		queue->rx_ring_ref = GRANT_INVALID_REF;
+		queue->rx_ring_ref = INVALID_GRANT_REF;
 	} else {
 		free_page((unsigned long)rxs);
 	}
-	if (queue->tx_ring_ref != GRANT_INVALID_REF) {
+	if (queue->tx_ring_ref != INVALID_GRANT_REF) {
 		gnttab_end_foreign_access(queue->tx_ring_ref,
 					  (unsigned long)txs);
-		queue->tx_ring_ref = GRANT_INVALID_REF;
+		queue->tx_ring_ref = INVALID_GRANT_REF;
 	} else {
 		free_page((unsigned long)txs);
 	}
@@ -2020,7 +2018,7 @@ static int xennet_init_queue(struct netfront_queue *queue)
 	queue->tx_pend_queue = TX_LINK_NONE;
 	for (i = 0; i < NET_TX_RING_SIZE; i++) {
 		queue->tx_link[i] = i + 1;
-		queue->grant_tx_ref[i] = GRANT_INVALID_REF;
+		queue->grant_tx_ref[i] = INVALID_GRANT_REF;
 		queue->grant_tx_page[i] = NULL;
 	}
 	queue->tx_link[NET_TX_RING_SIZE - 1] = TX_LINK_NONE;
@@ -2028,7 +2026,7 @@ static int xennet_init_queue(struct netfront_queue *queue)
 	/* Clear out rx_skbs */
 	for (i = 0; i < NET_RX_RING_SIZE; i++) {
 		queue->rx_skbs[i] = NULL;
-		queue->grant_rx_ref[i] = GRANT_INVALID_REF;
+		queue->grant_rx_ref[i] = INVALID_GRANT_REF;
 	}
 
 	/* A grant for every tx ring slot */
-- 
2.34.1

[PATCH v2 03/19] xen/scsifront: remove unused GRANT_INVALID_REF definition

[Juergen Gross]

GRANT_INVALID_REF isn't used in scsifront, so remove it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/scsi/xen-scsifront.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index 56173beecbc6..4c55e479fc36 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -58,9 +58,6 @@
 
 #include <asm/xen/hypervisor.h>
 
-
-#define GRANT_INVALID_REF	0
-
 #define VSCSIFRONT_OP_ADD_LUN	1
 #define VSCSIFRONT_OP_DEL_LUN	2
 #define VSCSIFRONT_OP_READD_LUN	3
-- 
2.34.1

[PATCH v2 04/19] xen/usb: switch xen-hcd to use INVALID_GRANT_REF

[Juergen Gross]

Instead of using a private macro for an invalid grant reference use
the common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/host/xen-hcd.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xen-hcd.c b/drivers/usb/host/xen-hcd.c
index 3e487baf8422..9cbc7c2dab02 100644
--- a/drivers/usb/host/xen-hcd.c
+++ b/drivers/usb/host/xen-hcd.c
@@ -87,8 +87,6 @@ struct xenhcd_info {
 	bool error;
 };
 
-#define GRANT_INVALID_REF 0
-
 #define XENHCD_RING_JIFFIES (HZ/200)
 #define XENHCD_SCAN_JIFFIES 1
 
@@ -1100,17 +1098,17 @@ static void xenhcd_destroy_rings(struct xenhcd_info *info)
 		unbind_from_irqhandler(info->irq, info);
 	info->irq = 0;
 
-	if (info->urb_ring_ref != GRANT_INVALID_REF) {
+	if (info->urb_ring_ref != INVALID_GRANT_REF) {
 		gnttab_end_foreign_access(info->urb_ring_ref,
 					  (unsigned long)info->urb_ring.sring);
-		info->urb_ring_ref = GRANT_INVALID_REF;
+		info->urb_ring_ref = INVALID_GRANT_REF;
 	}
 	info->urb_ring.sring = NULL;
 
-	if (info->conn_ring_ref != GRANT_INVALID_REF) {
+	if (info->conn_ring_ref != INVALID_GRANT_REF) {
 		gnttab_end_foreign_access(info->conn_ring_ref,
 					  (unsigned long)info->conn_ring.sring);
-		info->conn_ring_ref = GRANT_INVALID_REF;
+		info->conn_ring_ref = INVALID_GRANT_REF;
 	}
 	info->conn_ring.sring = NULL;
 }
@@ -1123,8 +1121,8 @@ static int xenhcd_setup_rings(struct xenbus_device *dev,
 	grant_ref_t gref;
 	int err;
 
-	info->urb_ring_ref = GRANT_INVALID_REF;
-	info->conn_ring_ref = GRANT_INVALID_REF;
+	info->urb_ring_ref = INVALID_GRANT_REF;
+	info->conn_ring_ref = INVALID_GRANT_REF;
 
 	urb_sring = (struct xenusb_urb_sring *)get_zeroed_page(
 							GFP_NOIO | __GFP_HIGH);
-- 
2.34.1

[PATCH v2 05/19] xen/drm: switch xen_drm_front to use INVALID_GRANT_REF

[Juergen Gross]

Instead of using a private macro for an invalid grant reference use
the common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/gpu/drm/xen/xen_drm_front.h         | 9 ---------
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.c | 4 ++--
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.h b/drivers/gpu/drm/xen/xen_drm_front.h
index cefafe859aba..a987c78abe41 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.h
+++ b/drivers/gpu/drm/xen/xen_drm_front.h
@@ -80,15 +80,6 @@ struct drm_pending_vblank_event;
 /* timeout in ms to wait for backend to respond */
 #define XEN_DRM_FRONT_WAIT_BACK_MS	3000
 
-#ifndef GRANT_INVALID_REF
-/*
- * Note on usage of grant reference 0 as invalid grant reference:
- * grant reference 0 is valid, but never exposed to a PV driver,
- * because of the fact it is already in use/reserved by the PV console.
- */
-#define GRANT_INVALID_REF	0
-#endif
-
 struct xen_drm_front_info {
 	struct xenbus_device *xb_dev;
 	struct xen_drm_front_drm_info *drm_info;
diff --git a/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c b/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
index 08b526eeec16..4006568b9e32 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
@@ -147,7 +147,7 @@ static void evtchnl_free(struct xen_drm_front_info *front_info,
 		xenbus_free_evtchn(front_info->xb_dev, evtchnl->port);
 
 	/* end access and free the page */
-	if (evtchnl->gref != GRANT_INVALID_REF)
+	if (evtchnl->gref != INVALID_GRANT_REF)
 		gnttab_end_foreign_access(evtchnl->gref, page);
 
 	memset(evtchnl, 0, sizeof(*evtchnl));
@@ -168,7 +168,7 @@ static int evtchnl_alloc(struct xen_drm_front_info *front_info, int index,
 	evtchnl->index = index;
 	evtchnl->front_info = front_info;
 	evtchnl->state = EVTCHNL_STATE_DISCONNECTED;
-	evtchnl->gref = GRANT_INVALID_REF;
+	evtchnl->gref = INVALID_GRANT_REF;
 
 	page = get_zeroed_page(GFP_NOIO | __GFP_HIGH);
 	if (!page) {
-- 
2.34.1

[PATCH v2 06/19] xen/sound: switch xen_snd_front to use INVALID_GRANT_REF

[Juergen Gross]

Instead of using a private macro for an invalid grant reference use
the common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 sound/xen/xen_snd_front_evtchnl.c | 4 ++--
 sound/xen/xen_snd_front_evtchnl.h | 9 ---------
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/sound/xen/xen_snd_front_evtchnl.c b/sound/xen/xen_snd_front_evtchnl.c
index ecbc294fc59a..3e21369c8216 100644
--- a/sound/xen/xen_snd_front_evtchnl.c
+++ b/sound/xen/xen_snd_front_evtchnl.c
@@ -167,7 +167,7 @@ static void evtchnl_free(struct xen_snd_front_info *front_info,
 		xenbus_free_evtchn(front_info->xb_dev, channel->port);
 
 	/* End access and free the page. */
-	if (channel->gref != GRANT_INVALID_REF)
+	if (channel->gref != INVALID_GRANT_REF)
 		gnttab_end_foreign_access(channel->gref, page);
 	else
 		free_page(page);
@@ -207,7 +207,7 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
 	channel->index = index;
 	channel->front_info = front_info;
 	channel->state = EVTCHNL_STATE_DISCONNECTED;
-	channel->gref = GRANT_INVALID_REF;
+	channel->gref = INVALID_GRANT_REF;
 	page = get_zeroed_page(GFP_KERNEL);
 	if (!page) {
 		ret = -ENOMEM;
diff --git a/sound/xen/xen_snd_front_evtchnl.h b/sound/xen/xen_snd_front_evtchnl.h
index cbe51fd1ec15..3675fba70564 100644
--- a/sound/xen/xen_snd_front_evtchnl.h
+++ b/sound/xen/xen_snd_front_evtchnl.h
@@ -15,15 +15,6 @@
 
 struct xen_snd_front_info;
 
-#ifndef GRANT_INVALID_REF
-/*
- * FIXME: usage of grant reference 0 as invalid grant reference:
- * grant reference 0 is valid, but never exposed to a PV driver,
- * because of the fact it is already in use/reserved by the PV console.
- */
-#define GRANT_INVALID_REF	0
-#endif
-
 /* Timeout in ms to wait for backend to respond. */
 #define VSND_WAIT_BACK_MS	3000
 
-- 
2.34.1

[PATCH v2 07/19] xen/dmabuf: switch gntdev-dmabuf to use INVALID_GRANT_REF

[Juergen Gross]

Instead of using a private macro for an invalid grant reference use
the common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/gntdev-dmabuf.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index d5bfd7b867fc..91073b4e4a20 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -24,15 +24,6 @@
 
 MODULE_IMPORT_NS(DMA_BUF);
 
-#ifndef GRANT_INVALID_REF
-/*
- * Note on usage of grant reference 0 as invalid grant reference:
- * grant reference 0 is valid, but never exposed to a driver,
- * because of the fact it is already in use/reserved by the PV console.
- */
-#define GRANT_INVALID_REF	0
-#endif
-
 struct gntdev_dmabuf {
 	struct gntdev_dmabuf_priv *priv;
 	struct dma_buf *dmabuf;
@@ -532,7 +523,7 @@ static void dmabuf_imp_end_foreign_access(u32 *refs, int count)
 	int i;
 
 	for (i = 0; i < count; i++)
-		if (refs[i] != GRANT_INVALID_REF)
+		if (refs[i] != INVALID_GRANT_REF)
 			gnttab_end_foreign_access(refs[i], 0UL);
 }
 
@@ -567,7 +558,7 @@ static struct gntdev_dmabuf *dmabuf_imp_alloc_storage(int count)
 	gntdev_dmabuf->nr_pages = count;
 
 	for (i = 0; i < count; i++)
-		gntdev_dmabuf->u.imp.refs[i] = GRANT_INVALID_REF;
+		gntdev_dmabuf->u.imp.refs[i] = INVALID_GRANT_REF;
 
 	return gntdev_dmabuf;
 
-- 
2.34.1

[PATCH v2 08/19] xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF

[Juergen Gross]

Instead of using a private macro for an invalid grant reference use
the common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-front-pgdir-shbuf.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/xen/xen-front-pgdir-shbuf.c b/drivers/xen/xen-front-pgdir-shbuf.c
index a959dee21134..fa2921d4fbfc 100644
--- a/drivers/xen/xen-front-pgdir-shbuf.c
+++ b/drivers/xen/xen-front-pgdir-shbuf.c
@@ -21,15 +21,6 @@
 
 #include <xen/xen-front-pgdir-shbuf.h>
 
-#ifndef GRANT_INVALID_REF
-/*
- * FIXME: usage of grant reference 0 as invalid grant reference:
- * grant reference 0 is valid, but never exposed to a PV driver,
- * because of the fact it is already in use/reserved by the PV console.
- */
-#define GRANT_INVALID_REF	0
-#endif
-
 /**
  * This structure represents the structure of a shared page
  * that contains grant references to the pages of the shared
@@ -83,7 +74,7 @@ grant_ref_t
 xen_front_pgdir_shbuf_get_dir_start(struct xen_front_pgdir_shbuf *buf)
 {
 	if (!buf->grefs)
-		return GRANT_INVALID_REF;
+		return INVALID_GRANT_REF;
 
 	return buf->grefs[0];
 }
@@ -142,7 +133,7 @@ void xen_front_pgdir_shbuf_free(struct xen_front_pgdir_shbuf *buf)
 		int i;
 
 		for (i = 0; i < buf->num_grefs; i++)
-			if (buf->grefs[i] != GRANT_INVALID_REF)
+			if (buf->grefs[i] != INVALID_GRANT_REF)
 				gnttab_end_foreign_access(buf->grefs[i], 0UL);
 	}
 	kfree(buf->grefs);
@@ -355,7 +346,7 @@ static void backend_fill_page_dir(struct xen_front_pgdir_shbuf *buf)
 	}
 	/* Last page must say there is no more pages. */
 	page_dir = (struct xen_page_directory *)ptr;
-	page_dir->gref_dir_next_page = GRANT_INVALID_REF;
+	page_dir->gref_dir_next_page = INVALID_GRANT_REF;
 }
 
 /**
@@ -384,7 +375,7 @@ static void guest_fill_page_dir(struct xen_front_pgdir_shbuf *buf)
 
 		if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
 			to_copy = grefs_left;
-			page_dir->gref_dir_next_page = GRANT_INVALID_REF;
+			page_dir->gref_dir_next_page = INVALID_GRANT_REF;
 		} else {
 			to_copy = XEN_NUM_GREFS_PER_PAGE;
 			page_dir->gref_dir_next_page = buf->grefs[i + 1];
-- 
2.34.1

Re: [PATCH v2 08/19] xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF

[Oleksandr]

Hello Juergen


On 28.04.22 21:03, Oleksandr Tyshchenko wrote:
>
>
> On Thu, Apr 28, 2022 at 11:28 AM Juergen Gross <jgross@suse.com 
> <mailto:jgross@suse.com>> wrote:
>
> Hello Juergen
>
> [sorry for the possible format issue]
>
>     Instead of using a private macro for an invalid grant reference use
>     the common one.
>
>     Signed-off-by: Juergen Gross <jgross@suse.com
>     <mailto:jgross@suse.com>>
>     ---
>      drivers/xen/xen-front-pgdir-shbuf.c | 17 ++++-------------
>      1 file changed, 4 insertions(+), 13 deletions(-)
>
>     diff --git a/drivers/xen/xen-front-pgdir-shbuf.c
>     b/drivers/xen/xen-front-pgdir-shbuf.c
>     index a959dee21134..fa2921d4fbfc 100644
>     --- a/drivers/xen/xen-front-pgdir-shbuf.c
>     +++ b/drivers/xen/xen-front-pgdir-shbuf.c
>     @@ -21,15 +21,6 @@
>
>      #include <xen/xen-front-pgdir-shbuf.h>
>
>     -#ifndef GRANT_INVALID_REF
>     -/*
>     - * FIXME: usage of grant reference 0 as invalid grant reference:
>     - * grant reference 0 is valid, but never exposed to a PV driver,
>     - * because of the fact it is already in use/reserved by the PV
>     console.
>     - */
>     -#define GRANT_INVALID_REF      0
>     -#endif
>     -
>      /**
>       * This structure represents the structure of a shared page
>       * that contains grant references to the pages of the shared
>     @@ -83,7 +74,7 @@ grant_ref_t
>      xen_front_pgdir_shbuf_get_dir_start(struct xen_front_pgdir_shbuf
>     *buf)
>      {
>             if (!buf->grefs)
>     -               return GRANT_INVALID_REF;
>     +               return INVALID_GRANT_REF;
>
>             return buf->grefs[0];
>      }
>     @@ -142,7 +133,7 @@ void xen_front_pgdir_shbuf_free(struct
>     xen_front_pgdir_shbuf *buf)
>                     int i;
>
>                     for (i = 0; i < buf->num_grefs; i++)
>     -                       if (buf->grefs[i] != GRANT_INVALID_REF)
>     +                       if (buf->grefs[i] != INVALID_GRANT_REF)
>     gnttab_end_foreign_access(buf->grefs[i], 0UL);
>             }
>             kfree(buf->grefs);
>     @@ -355,7 +346,7 @@ static void backend_fill_page_dir(struct
>     xen_front_pgdir_shbuf *buf)
>             }
>             /* Last page must say there is no more pages. */
>             page_dir = (struct xen_page_directory *)ptr;
>     -       page_dir->gref_dir_next_page = GRANT_INVALID_REF;
>     +       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
>      }
>
>      /**
>     @@ -384,7 +375,7 @@ static void guest_fill_page_dir(struct
>     xen_front_pgdir_shbuf *buf)
>
>                     if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
>                             to_copy = grefs_left;
>     -                       page_dir->gref_dir_next_page =
>     GRANT_INVALID_REF;
>     +                       page_dir->gref_dir_next_page =
>     INVALID_GRANT_REF;
>
>
> I faced an issue with testing PV Sound with the current series.
>
> root@salvator-x-h3-4x2g-xt-domu:~# aplay /media/MoodyLoop.wav
> Playing WAVE '/media/MoodyLoop.wav' : Signed 16 bit Little Endian, 
> Rate 44100 Hz, Stereo
> (XEN) common/grant_table.c:1053:d1v2 Bad ref 0xffffffff for d6
>
> Here we have an interesting situation. PV Sound frontend uses this 
> xen-front-pgdir-shbuf framework. Technically, this patch changes 
> page_dir->gref_dir_next_page (reference to the next page describing 
> page directory) from 0 to 0xffffffff here.
> #define INVALID_GRANT_REF  ((grant_ref_t)-1)
>
> But according to the protocol (sndif.h), "0" means that there are no 
> more pages in the list and the user space backend expects only that 
> value. So receiving 0xffffffff it assumes there are pages in the list 
> and trying to process...
> https://elixir.bootlin.com/linux/v5.18-rc4/source/include/xen/interface/io/sndif.h#L650
>
>
> I think, the same is relevant to backend_fill_page_dir() as well.


In addition to what I said yesterday:

PV Display also uses this xen-front-pgdir-shbuf framework. It's protocol 
(displif.h) also mentions the same as sndif.h if the context of 
gref_dir_next_page:

  * gref_dir_next_page - grant_ref_t, reference to the next page describing
  *   page directory. Must be 0 if there are no more pages in the list.


With that local change both PV devices work in my environment.

diff --git a/drivers/xen/xen-front-pgdir-shbuf.c 
b/drivers/xen/xen-front-pgdir-shbuf.c
index fa2921d..ad4a88e 100644
--- a/drivers/xen/xen-front-pgdir-shbuf.c
+++ b/drivers/xen/xen-front-pgdir-shbuf.c
@@ -346,7 +346,7 @@ static void backend_fill_page_dir(struct 
xen_front_pgdir_shbuf *buf)
         }
         /* Last page must say there is no more pages. */
         page_dir = (struct xen_page_directory *)ptr;
-       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
+       page_dir->gref_dir_next_page = 0;
  }

  /**
@@ -375,7 +375,7 @@ static void guest_fill_page_dir(struct 
xen_front_pgdir_shbuf *buf)

                 if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
                         to_copy = grefs_left;
-                       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
+                       page_dir->gref_dir_next_page = 0;
                 } else {
                         to_copy = XEN_NUM_GREFS_PER_PAGE;
                         page_dir->gref_dir_next_page = buf->grefs[i + 1];
(END)



>
>                     } else {
>                             to_copy = XEN_NUM_GREFS_PER_PAGE;
>                             page_dir->gref_dir_next_page =
>     buf->grefs[i + 1];
>     -- 
>     2.34.1
>
>
>
>
> -- 
> Regards,
>
> Oleksandr Tyshchenko

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH v2 08/19] xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 6277 bytes --]

On 29.04.22 17:28, Oleksandr wrote:
> 
> Hello Juergen
> 
> 
> On 28.04.22 21:03, Oleksandr Tyshchenko wrote:
>>
>>
>> On Thu, Apr 28, 2022 at 11:28 AM Juergen Gross <jgross@suse.com 
>> <mailto:jgross@suse.com>> wrote:
>>
>> Hello Juergen
>>
>> [sorry for the possible format issue]
>>
>>     Instead of using a private macro for an invalid grant reference use
>>     the common one.
>>
>>     Signed-off-by: Juergen Gross <jgross@suse.com
>>     <mailto:jgross@suse.com>>
>>     ---
>>      drivers/xen/xen-front-pgdir-shbuf.c | 17 ++++-------------
>>      1 file changed, 4 insertions(+), 13 deletions(-)
>>
>>     diff --git a/drivers/xen/xen-front-pgdir-shbuf.c
>>     b/drivers/xen/xen-front-pgdir-shbuf.c
>>     index a959dee21134..fa2921d4fbfc 100644
>>     --- a/drivers/xen/xen-front-pgdir-shbuf.c
>>     +++ b/drivers/xen/xen-front-pgdir-shbuf.c
>>     @@ -21,15 +21,6 @@
>>
>>      #include <xen/xen-front-pgdir-shbuf.h>
>>
>>     -#ifndef GRANT_INVALID_REF
>>     -/*
>>     - * FIXME: usage of grant reference 0 as invalid grant reference:
>>     - * grant reference 0 is valid, but never exposed to a PV driver,
>>     - * because of the fact it is already in use/reserved by the PV
>>     console.
>>     - */
>>     -#define GRANT_INVALID_REF      0
>>     -#endif
>>     -
>>      /**
>>       * This structure represents the structure of a shared page
>>       * that contains grant references to the pages of the shared
>>     @@ -83,7 +74,7 @@ grant_ref_t
>>      xen_front_pgdir_shbuf_get_dir_start(struct xen_front_pgdir_shbuf
>>     *buf)
>>      {
>>             if (!buf->grefs)
>>     -               return GRANT_INVALID_REF;
>>     +               return INVALID_GRANT_REF;
>>
>>             return buf->grefs[0];
>>      }
>>     @@ -142,7 +133,7 @@ void xen_front_pgdir_shbuf_free(struct
>>     xen_front_pgdir_shbuf *buf)
>>                     int i;
>>
>>                     for (i = 0; i < buf->num_grefs; i++)
>>     -                       if (buf->grefs[i] != GRANT_INVALID_REF)
>>     +                       if (buf->grefs[i] != INVALID_GRANT_REF)
>>     gnttab_end_foreign_access(buf->grefs[i], 0UL);
>>             }
>>             kfree(buf->grefs);
>>     @@ -355,7 +346,7 @@ static void backend_fill_page_dir(struct
>>     xen_front_pgdir_shbuf *buf)
>>             }
>>             /* Last page must say there is no more pages. */
>>             page_dir = (struct xen_page_directory *)ptr;
>>     -       page_dir->gref_dir_next_page = GRANT_INVALID_REF;
>>     +       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
>>      }
>>
>>      /**
>>     @@ -384,7 +375,7 @@ static void guest_fill_page_dir(struct
>>     xen_front_pgdir_shbuf *buf)
>>
>>                     if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
>>                             to_copy = grefs_left;
>>     -                       page_dir->gref_dir_next_page =
>>     GRANT_INVALID_REF;
>>     +                       page_dir->gref_dir_next_page =
>>     INVALID_GRANT_REF;
>>
>>
>> I faced an issue with testing PV Sound with the current series.
>>
>> root@salvator-x-h3-4x2g-xt-domu:~# aplay /media/MoodyLoop.wav
>> Playing WAVE '/media/MoodyLoop.wav' : Signed 16 bit Little Endian, Rate 44100 
>> Hz, Stereo
>> (XEN) common/grant_table.c:1053:d1v2 Bad ref 0xffffffff for d6
>>
>> Here we have an interesting situation. PV Sound frontend uses this 
>> xen-front-pgdir-shbuf framework. Technically, this patch changes 
>> page_dir->gref_dir_next_page (reference to the next page describing page 
>> directory) from 0 to 0xffffffff here.
>> #define INVALID_GRANT_REF  ((grant_ref_t)-1)
>>
>> But according to the protocol (sndif.h), "0" means that there are no more 
>> pages in the list and the user space backend expects only that value. So 
>> receiving 0xffffffff it assumes there are pages in the list and trying to 
>> process...
>> https://elixir.bootlin.com/linux/v5.18-rc4/source/include/xen/interface/io/sndif.h#L650 
>>
>>
>>
>> I think, the same is relevant to backend_fill_page_dir() as well.
> 
> 
> In addition to what I said yesterday:
> 
> PV Display also uses this xen-front-pgdir-shbuf framework. It's protocol 
> (displif.h) also mentions the same as sndif.h if the context of gref_dir_next_page:
> 
>   * gref_dir_next_page - grant_ref_t, reference to the next page describing
>   *   page directory. Must be 0 if there are no more pages in the list.
> 
> 
> With that local change both PV devices work in my environment.
> 
> diff --git a/drivers/xen/xen-front-pgdir-shbuf.c 
> b/drivers/xen/xen-front-pgdir-shbuf.c
> index fa2921d..ad4a88e 100644
> --- a/drivers/xen/xen-front-pgdir-shbuf.c
> +++ b/drivers/xen/xen-front-pgdir-shbuf.c
> @@ -346,7 +346,7 @@ static void backend_fill_page_dir(struct 
> xen_front_pgdir_shbuf *buf)
>          }
>          /* Last page must say there is no more pages. */
>          page_dir = (struct xen_page_directory *)ptr;
> -       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
> +       page_dir->gref_dir_next_page = 0;
>   }
> 
>   /**
> @@ -375,7 +375,7 @@ static void guest_fill_page_dir(struct xen_front_pgdir_shbuf 
> *buf)
> 
>                  if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
>                          to_copy = grefs_left;
> -                       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
> +                       page_dir->gref_dir_next_page = 0;
>                  } else {
>                          to_copy = XEN_NUM_GREFS_PER_PAGE;
>                          page_dir->gref_dir_next_page = buf->grefs[i + 1];

I think I'll introduce a proper define for that purpose.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 08/19] xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF

[Oleksandr]

On 02.05.22 16:31, Juergen Gross wrote:


Hello Juergen


> On 29.04.22 17:28, Oleksandr wrote:
>>
>> Hello Juergen
>>
>>
>> On 28.04.22 21:03, Oleksandr Tyshchenko wrote:
>>>
>>>
>>> On Thu, Apr 28, 2022 at 11:28 AM Juergen Gross <jgross@suse.com 
>>> <mailto:jgross@suse.com>> wrote:
>>>
>>> Hello Juergen
>>>
>>> [sorry for the possible format issue]
>>>
>>>     Instead of using a private macro for an invalid grant reference use
>>>     the common one.
>>>
>>>     Signed-off-by: Juergen Gross <jgross@suse.com
>>>     <mailto:jgross@suse.com>>
>>>     ---
>>>      drivers/xen/xen-front-pgdir-shbuf.c | 17 ++++-------------
>>>      1 file changed, 4 insertions(+), 13 deletions(-)
>>>
>>>     diff --git a/drivers/xen/xen-front-pgdir-shbuf.c
>>>     b/drivers/xen/xen-front-pgdir-shbuf.c
>>>     index a959dee21134..fa2921d4fbfc 100644
>>>     --- a/drivers/xen/xen-front-pgdir-shbuf.c
>>>     +++ b/drivers/xen/xen-front-pgdir-shbuf.c
>>>     @@ -21,15 +21,6 @@
>>>
>>>      #include <xen/xen-front-pgdir-shbuf.h>
>>>
>>>     -#ifndef GRANT_INVALID_REF
>>>     -/*
>>>     - * FIXME: usage of grant reference 0 as invalid grant reference:
>>>     - * grant reference 0 is valid, but never exposed to a PV driver,
>>>     - * because of the fact it is already in use/reserved by the PV
>>>     console.
>>>     - */
>>>     -#define GRANT_INVALID_REF      0
>>>     -#endif
>>>     -
>>>      /**
>>>       * This structure represents the structure of a shared page
>>>       * that contains grant references to the pages of the shared
>>>     @@ -83,7 +74,7 @@ grant_ref_t
>>>      xen_front_pgdir_shbuf_get_dir_start(struct xen_front_pgdir_shbuf
>>>     *buf)
>>>      {
>>>             if (!buf->grefs)
>>>     -               return GRANT_INVALID_REF;
>>>     +               return INVALID_GRANT_REF;
>>>
>>>             return buf->grefs[0];
>>>      }
>>>     @@ -142,7 +133,7 @@ void xen_front_pgdir_shbuf_free(struct
>>>     xen_front_pgdir_shbuf *buf)
>>>                     int i;
>>>
>>>                     for (i = 0; i < buf->num_grefs; i++)
>>>     -                       if (buf->grefs[i] != GRANT_INVALID_REF)
>>>     +                       if (buf->grefs[i] != INVALID_GRANT_REF)
>>>     gnttab_end_foreign_access(buf->grefs[i], 0UL);
>>>             }
>>>             kfree(buf->grefs);
>>>     @@ -355,7 +346,7 @@ static void backend_fill_page_dir(struct
>>>     xen_front_pgdir_shbuf *buf)
>>>             }
>>>             /* Last page must say there is no more pages. */
>>>             page_dir = (struct xen_page_directory *)ptr;
>>>     -       page_dir->gref_dir_next_page = GRANT_INVALID_REF;
>>>     +       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
>>>      }
>>>
>>>      /**
>>>     @@ -384,7 +375,7 @@ static void guest_fill_page_dir(struct
>>>     xen_front_pgdir_shbuf *buf)
>>>
>>>                     if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
>>>                             to_copy = grefs_left;
>>>     -                       page_dir->gref_dir_next_page =
>>>     GRANT_INVALID_REF;
>>>     +                       page_dir->gref_dir_next_page =
>>>     INVALID_GRANT_REF;
>>>
>>>
>>> I faced an issue with testing PV Sound with the current series.
>>>
>>> root@salvator-x-h3-4x2g-xt-domu:~# aplay /media/MoodyLoop.wav
>>> Playing WAVE '/media/MoodyLoop.wav' : Signed 16 bit Little Endian, 
>>> Rate 44100 Hz, Stereo
>>> (XEN) common/grant_table.c:1053:d1v2 Bad ref 0xffffffff for d6
>>>
>>> Here we have an interesting situation. PV Sound frontend uses this 
>>> xen-front-pgdir-shbuf framework. Technically, this patch changes 
>>> page_dir->gref_dir_next_page (reference to the next page describing 
>>> page directory) from 0 to 0xffffffff here.
>>> #define INVALID_GRANT_REF  ((grant_ref_t)-1)
>>>
>>> But according to the protocol (sndif.h), "0" means that there are no 
>>> more pages in the list and the user space backend expects only that 
>>> value. So receiving 0xffffffff it assumes there are pages in the 
>>> list and trying to process...
>>> https://elixir.bootlin.com/linux/v5.18-rc4/source/include/xen/interface/io/sndif.h#L650 
>>>
>>>
>>>
>>> I think, the same is relevant to backend_fill_page_dir() as well.
>>
>>
>> In addition to what I said yesterday:
>>
>> PV Display also uses this xen-front-pgdir-shbuf framework. It's 
>> protocol (displif.h) also mentions the same as sndif.h if the context 
>> of gref_dir_next_page:
>>
>>   * gref_dir_next_page - grant_ref_t, reference to the next page 
>> describing
>>   *   page directory. Must be 0 if there are no more pages in the list.
>>
>>
>> With that local change both PV devices work in my environment.
>>
>> diff --git a/drivers/xen/xen-front-pgdir-shbuf.c 
>> b/drivers/xen/xen-front-pgdir-shbuf.c
>> index fa2921d..ad4a88e 100644
>> --- a/drivers/xen/xen-front-pgdir-shbuf.c
>> +++ b/drivers/xen/xen-front-pgdir-shbuf.c
>> @@ -346,7 +346,7 @@ static void backend_fill_page_dir(struct 
>> xen_front_pgdir_shbuf *buf)
>>          }
>>          /* Last page must say there is no more pages. */
>>          page_dir = (struct xen_page_directory *)ptr;
>> -       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
>> +       page_dir->gref_dir_next_page = 0;
>>   }
>>
>>   /**
>> @@ -375,7 +375,7 @@ static void guest_fill_page_dir(struct 
>> xen_front_pgdir_shbuf *buf)
>>
>>                  if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
>>                          to_copy = grefs_left;
>> -                       page_dir->gref_dir_next_page = 
>> INVALID_GRANT_REF;
>> +                       page_dir->gref_dir_next_page = 0;
>>                  } else {
>>                          to_copy = XEN_NUM_GREFS_PER_PAGE;
>>                          page_dir->gref_dir_next_page = buf->grefs[i 
>> + 1];
>
> I think I'll introduce a proper define for that purpose.


I think it would be the best option.



>
>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH v2 08/19] xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4339 bytes --]

On 28.04.22 20:03, Oleksandr Tyshchenko wrote:
> 
> 
> On Thu, Apr 28, 2022 at 11:28 AM Juergen Gross <jgross@suse.com 
> <mailto:jgross@suse.com>> wrote:
> 
> Hello Juergen
> 
> [sorry for the possible format issue]
> 
>     Instead of using a private macro for an invalid grant reference use
>     the common one.
> 
>     Signed-off-by: Juergen Gross <jgross@suse.com <mailto:jgross@suse.com>>
>     ---
>       drivers/xen/xen-front-pgdir-shbuf.c | 17 ++++-------------
>       1 file changed, 4 insertions(+), 13 deletions(-)
> 
>     diff --git a/drivers/xen/xen-front-pgdir-shbuf.c
>     b/drivers/xen/xen-front-pgdir-shbuf.c
>     index a959dee21134..fa2921d4fbfc 100644
>     --- a/drivers/xen/xen-front-pgdir-shbuf.c
>     +++ b/drivers/xen/xen-front-pgdir-shbuf.c
>     @@ -21,15 +21,6 @@
> 
>       #include <xen/xen-front-pgdir-shbuf.h>
> 
>     -#ifndef GRANT_INVALID_REF
>     -/*
>     - * FIXME: usage of grant reference 0 as invalid grant reference:
>     - * grant reference 0 is valid, but never exposed to a PV driver,
>     - * because of the fact it is already in use/reserved by the PV console.
>     - */
>     -#define GRANT_INVALID_REF      0
>     -#endif
>     -
>       /**
>        * This structure represents the structure of a shared page
>        * that contains grant references to the pages of the shared
>     @@ -83,7 +74,7 @@ grant_ref_t
>       xen_front_pgdir_shbuf_get_dir_start(struct xen_front_pgdir_shbuf *buf)
>       {
>              if (!buf->grefs)
>     -               return GRANT_INVALID_REF;
>     +               return INVALID_GRANT_REF;
> 
>              return buf->grefs[0];
>       }
>     @@ -142,7 +133,7 @@ void xen_front_pgdir_shbuf_free(struct
>     xen_front_pgdir_shbuf *buf)
>                      int i;
> 
>                      for (i = 0; i < buf->num_grefs; i++)
>     -                       if (buf->grefs[i] != GRANT_INVALID_REF)
>     +                       if (buf->grefs[i] != INVALID_GRANT_REF)
>                                      gnttab_end_foreign_access(buf->grefs[i], 0UL);
>              }
>              kfree(buf->grefs);
>     @@ -355,7 +346,7 @@ static void backend_fill_page_dir(struct
>     xen_front_pgdir_shbuf *buf)
>              }
>              /* Last page must say there is no more pages. */
>              page_dir = (struct xen_page_directory *)ptr;
>     -       page_dir->gref_dir_next_page = GRANT_INVALID_REF;
>     +       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
>       }
> 
>       /**
>     @@ -384,7 +375,7 @@ static void guest_fill_page_dir(struct
>     xen_front_pgdir_shbuf *buf)
> 
>                      if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
>                              to_copy = grefs_left;
>     -                       page_dir->gref_dir_next_page = GRANT_INVALID_REF;
>     +                       page_dir->gref_dir_next_page = INVALID_GRANT_REF;
> 
> 
> I faced an issue with testing PV Sound with the current series.
> 
> root@salvator-x-h3-4x2g-xt-domu:~# aplay /media/MoodyLoop.wav
> Playing WAVE '/media/MoodyLoop.wav' : Signed 16 bit Little Endian, Rate 44100 
> Hz, Stereo
> (XEN) common/grant_table.c:1053:d1v2 Bad ref 0xffffffff for d6
> 
> Here we have an interesting situation. PV Sound frontend uses this 
> xen-front-pgdir-shbuf framework. Technically, this patch changes 
> page_dir->gref_dir_next_page (reference to the next page describing page 
> directory) from 0 to 0xffffffff here.
> #define INVALID_GRANT_REF  ((grant_ref_t)-1)
> 
> But according to the protocol (sndif.h), "0" means that there are no more pages 
> in the list and the user space backend expects only that value. So 
> receiving 0xffffffff it assumes there are pages in the list and trying to 
> process...

Hmm, that's unfortunate.

> https://elixir.bootlin.com/linux/v5.18-rc4/source/include/xen/interface/io/sndif.h#L650 
> <https://elixir.bootlin.com/linux/v5.18-rc4/source/include/xen/interface/io/sndif.h#L650>
> 
> 
> I think, the same is relevant to backend_fill_page_dir() as well.

Thanks for finding this.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH v2 09/19] xen: update ring.h

[Juergen Gross]

Update include/xen/interface/io/ring.h to its newest version.

Switch the two improper use cases of RING_HAS_UNCONSUMED_RESPONSES() to
XEN_RING_NR_UNCONSUMED_RESPONSES() in order to avoid the nasty
XEN_RING_HAS_UNCONSUMED_IS_BOOL #define.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 drivers/net/xen-netfront.c      |  4 ++--
 include/xen/interface/io/ring.h | 19 ++++++++++++++-----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index af3d3de7d9fa..966bee2a6902 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -866,7 +866,7 @@ static void xennet_set_rx_rsp_cons(struct netfront_queue *queue, RING_IDX val)
 
 	spin_lock_irqsave(&queue->rx_cons_lock, flags);
 	queue->rx.rsp_cons = val;
-	queue->rx_rsp_unconsumed = RING_HAS_UNCONSUMED_RESPONSES(&queue->rx);
+	queue->rx_rsp_unconsumed = XEN_RING_NR_UNCONSUMED_RESPONSES(&queue->rx);
 	spin_unlock_irqrestore(&queue->rx_cons_lock, flags);
 }
 
@@ -1498,7 +1498,7 @@ static bool xennet_handle_rx(struct netfront_queue *queue, unsigned int *eoi)
 		return false;
 
 	spin_lock_irqsave(&queue->rx_cons_lock, flags);
-	work_queued = RING_HAS_UNCONSUMED_RESPONSES(&queue->rx);
+	work_queued = XEN_RING_NR_UNCONSUMED_RESPONSES(&queue->rx);
 	if (work_queued > queue->rx_rsp_unconsumed) {
 		queue->rx_rsp_unconsumed = work_queued;
 		*eoi = 0;
diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 2470ec45ebb2..ba4c4274b714 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -72,9 +72,8 @@ typedef unsigned int RING_IDX;
  * of the shared memory area (PAGE_SIZE, for instance). To initialise
  * the front half:
  *
- *     mytag_front_ring_t front_ring;
- *     SHARED_RING_INIT((mytag_sring_t *)shared_page);
- *     FRONT_RING_INIT(&front_ring, (mytag_sring_t *)shared_page, PAGE_SIZE);
+ *     mytag_front_ring_t ring;
+ *     XEN_FRONT_RING_INIT(&ring, (mytag_sring_t *)shared_page, PAGE_SIZE);
  *
  * Initializing the back follows similarly (note that only the front
  * initializes the shared ring):
@@ -146,6 +145,11 @@ struct __name##_back_ring {                                             \
 
 #define FRONT_RING_INIT(_r, _s, __size) FRONT_RING_ATTACH(_r, _s, 0, __size)
 
+#define XEN_FRONT_RING_INIT(r, s, size) do {                            \
+    SHARED_RING_INIT(s);                                                \
+    FRONT_RING_INIT(r, s, size);                                        \
+} while (0)
+
 #define BACK_RING_ATTACH(_r, _s, _i, __size) do {                       \
     (_r)->rsp_prod_pvt = (_i);                                          \
     (_r)->req_cons = (_i);                                              \
@@ -170,16 +174,21 @@ struct __name##_back_ring {                                             \
     (RING_FREE_REQUESTS(_r) == 0)
 
 /* Test if there are outstanding messages to be processed on a ring. */
-#define RING_HAS_UNCONSUMED_RESPONSES(_r)                               \
+#define XEN_RING_NR_UNCONSUMED_RESPONSES(_r)                            \
     ((_r)->sring->rsp_prod - (_r)->rsp_cons)
 
-#define RING_HAS_UNCONSUMED_REQUESTS(_r) ({                             \
+#define XEN_RING_NR_UNCONSUMED_REQUESTS(_r) ({                          \
     unsigned int req = (_r)->sring->req_prod - (_r)->req_cons;          \
     unsigned int rsp = RING_SIZE(_r) -                                  \
         ((_r)->req_cons - (_r)->rsp_prod_pvt);                          \
     req < rsp ? req : rsp;                                              \
 })
 
+#define RING_HAS_UNCONSUMED_RESPONSES(_r) \
+    (!!XEN_RING_NR_UNCONSUMED_RESPONSES(_r))
+#define RING_HAS_UNCONSUMED_REQUESTS(_r)  \
+    (!!XEN_RING_NR_UNCONSUMED_REQUESTS(_r))
+
 /* Direct access to individual ring elements, by index. */
 #define RING_GET_REQUEST(_r, _idx)                                      \
     (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
-- 
2.34.1

[PATCH v2 10/19] xen/xenbus: add xenbus_setup_ring() service function

[Juergen Gross]

Most PV device frontends share very similar code for setting up shared
ring buffers:

- allocate page(s)
- init the ring admin data
- give the backend access to the ring via grants

Tearing down the ring requires similar actions in all frontends again:

- remove grants
- free the page(s)

Provide service functions xenbus_setup_ring() and xenbus_teardown_ring()
for that purpose.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xenbus/xenbus_client.c | 69 ++++++++++++++++++++++++++++++
 include/xen/xenbus.h               |  4 ++
 2 files changed, 73 insertions(+)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index df6890681231..1a2e0d94ccd1 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -407,6 +407,75 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
 }
 EXPORT_SYMBOL_GPL(xenbus_grant_ring);
 
+/*
+ * xenbus_setup_ring
+ * @dev: xenbus device
+ * @vaddr: pointer to starting virtual address of the ring
+ * @nr_pages: number of pages to be granted
+ * @grefs: grant reference array to be filled in
+ *
+ * Allocate physically contiguous pages for a shared ring buffer and grant it
+ * to the peer of the given device. The ring buffer is initially filled with
+ * zeroes. The virtual address of the ring is stored at @vaddr and the
+ * grant references are stored in the @grefs array. In case of error @vaddr
+ * will be set to NULL and @grefs will be filled with INVALID_GRANT_REF.
+ */
+int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
+		      unsigned int nr_pages, grant_ref_t *grefs)
+{
+	unsigned long ring_size = nr_pages * XEN_PAGE_SIZE;
+	unsigned int i;
+	int ret;
+
+	*vaddr = alloc_pages_exact(ring_size, gfp | __GFP_ZERO);
+	if (!*vaddr) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	ret = xenbus_grant_ring(dev, *vaddr, nr_pages, grefs);
+	if (ret)
+		goto err;
+
+	return 0;
+
+ err:
+	if (*vaddr)
+		free_pages_exact(*vaddr, ring_size);
+	for (i = 0; i < nr_pages; i++)
+		grefs[i] = INVALID_GRANT_REF;
+	*vaddr = NULL;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(xenbus_setup_ring);
+
+/*
+ * xenbus_teardown_ring
+ * @vaddr: starting virtual address of the ring
+ * @nr_pages: number of pages
+ * @grefs: grant reference array
+ *
+ * Remove grants for the shared ring buffer and free the associated memory.
+ * On return the grant reference array is filled with INVALID_GRANT_REF.
+ */
+void xenbus_teardown_ring(void **vaddr, unsigned int nr_pages,
+			  grant_ref_t *grefs)
+{
+	unsigned int i;
+
+	for (i = 0; i < nr_pages; i++) {
+		if (grefs[i] != INVALID_GRANT_REF) {
+			gnttab_end_foreign_access(grefs[i], 0);
+			grefs[i] = INVALID_GRANT_REF;
+		}
+	}
+
+	if (*vaddr)
+		free_pages_exact(*vaddr, nr_pages * XEN_PAGE_SIZE);
+	*vaddr = NULL;
+}
+EXPORT_SYMBOL_GPL(xenbus_teardown_ring);
 
 /**
  * Allocate an event channel for the given xenbus_device, assigning the newly
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index b13eb86395e0..b533b4adc835 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -226,6 +226,10 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
 int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
 int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
 		      unsigned int nr_pages, grant_ref_t *grefs);
+int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
+		      unsigned int nr_pages, grant_ref_t *grefs);
+void xenbus_teardown_ring(void **vaddr, unsigned int nr_pages,
+			  grant_ref_t *grefs);
 int xenbus_map_ring_valloc(struct xenbus_device *dev, grant_ref_t *gnt_refs,
 			   unsigned int nr_grefs, void **vaddr);
 
-- 
2.34.1

[PATCH v2 11/19] xen/blkfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

Simplify blkfront's ring creation and removal via xenbus_setup_ring()
and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/block/xen-blkfront.c | 37 ++++++++----------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 7f35e30e626a..bd7b34f29193 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1280,15 +1280,8 @@ static void blkif_free_ring(struct blkfront_ring_info *rinfo)
 	flush_work(&rinfo->work);
 
 	/* Free resources associated with old device channel. */
-	for (i = 0; i < info->nr_ring_pages; i++) {
-		if (rinfo->ring_ref[i] != INVALID_GRANT_REF) {
-			gnttab_end_foreign_access(rinfo->ring_ref[i], 0);
-			rinfo->ring_ref[i] = INVALID_GRANT_REF;
-		}
-	}
-	free_pages_exact(rinfo->ring.sring,
-			 info->nr_ring_pages * XEN_PAGE_SIZE);
-	rinfo->ring.sring = NULL;
+	xenbus_teardown_ring((void **)&rinfo->ring.sring, info->nr_ring_pages,
+			     rinfo->ring_ref);
 
 	if (rinfo->irq)
 		unbind_from_irqhandler(rinfo->irq, rinfo);
@@ -1679,30 +1672,16 @@ static int setup_blkring(struct xenbus_device *dev,
 			 struct blkfront_ring_info *rinfo)
 {
 	struct blkif_sring *sring;
-	int err, i;
+	int err;
 	struct blkfront_info *info = rinfo->dev_info;
 	unsigned long ring_size = info->nr_ring_pages * XEN_PAGE_SIZE;
-	grant_ref_t gref[XENBUS_MAX_RING_GRANTS];
-
-	for (i = 0; i < info->nr_ring_pages; i++)
-		rinfo->ring_ref[i] = INVALID_GRANT_REF;
 
-	sring = alloc_pages_exact(ring_size, GFP_NOIO);
-	if (!sring) {
-		xenbus_dev_fatal(dev, -ENOMEM, "allocating shared ring");
-		return -ENOMEM;
-	}
-	SHARED_RING_INIT(sring);
-	FRONT_RING_INIT(&rinfo->ring, sring, ring_size);
-
-	err = xenbus_grant_ring(dev, rinfo->ring.sring, info->nr_ring_pages, gref);
-	if (err < 0) {
-		free_pages_exact(sring, ring_size);
-		rinfo->ring.sring = NULL;
+	err = xenbus_setup_ring(dev, GFP_NOIO, (void **)&sring,
+				info->nr_ring_pages, rinfo->ring_ref);
+	if (err)
 		goto fail;
-	}
-	for (i = 0; i < info->nr_ring_pages; i++)
-		rinfo->ring_ref[i] = gref[i];
+
+	XEN_FRONT_RING_INIT(&rinfo->ring, sring, ring_size);
 
 	err = xenbus_alloc_evtchn(dev, &rinfo->evtchn);
 	if (err)
-- 
2.34.1

[PATCH v2 12/19] xen/netfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

Simplify netfront's ring creation and removal via xenbus_setup_ring()
and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/net/xen-netfront.c | 53 +++++++++-----------------------------
 1 file changed, 12 insertions(+), 41 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 966bee2a6902..65ab907aca5a 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1921,8 +1921,7 @@ static int setup_netfront(struct xenbus_device *dev,
 			struct netfront_queue *queue, unsigned int feature_split_evtchn)
 {
 	struct xen_netif_tx_sring *txs;
-	struct xen_netif_rx_sring *rxs = NULL;
-	grant_ref_t gref;
+	struct xen_netif_rx_sring *rxs;
 	int err;
 
 	queue->tx_ring_ref = INVALID_GRANT_REF;
@@ -1930,33 +1929,19 @@ static int setup_netfront(struct xenbus_device *dev,
 	queue->rx.sring = NULL;
 	queue->tx.sring = NULL;
 
-	txs = (struct xen_netif_tx_sring *)get_zeroed_page(GFP_NOIO | __GFP_HIGH);
-	if (!txs) {
-		err = -ENOMEM;
-		xenbus_dev_fatal(dev, err, "allocating tx ring page");
+	err = xenbus_setup_ring(dev, GFP_NOIO | __GFP_HIGH, (void **)&txs,
+				1, &queue->tx_ring_ref);
+	if (err)
 		goto fail;
-	}
-	SHARED_RING_INIT(txs);
-	FRONT_RING_INIT(&queue->tx, txs, XEN_PAGE_SIZE);
 
-	err = xenbus_grant_ring(dev, txs, 1, &gref);
-	if (err < 0)
-		goto fail;
-	queue->tx_ring_ref = gref;
+	XEN_FRONT_RING_INIT(&queue->tx, txs, XEN_PAGE_SIZE);
 
-	rxs = (struct xen_netif_rx_sring *)get_zeroed_page(GFP_NOIO | __GFP_HIGH);
-	if (!rxs) {
-		err = -ENOMEM;
-		xenbus_dev_fatal(dev, err, "allocating rx ring page");
+	err = xenbus_setup_ring(dev, GFP_NOIO | __GFP_HIGH, (void **)&rxs,
+				1, &queue->rx_ring_ref);
+	if (err)
 		goto fail;
-	}
-	SHARED_RING_INIT(rxs);
-	FRONT_RING_INIT(&queue->rx, rxs, XEN_PAGE_SIZE);
 
-	err = xenbus_grant_ring(dev, rxs, 1, &gref);
-	if (err < 0)
-		goto fail;
-	queue->rx_ring_ref = gref;
+	XEN_FRONT_RING_INIT(&queue->rx, rxs, XEN_PAGE_SIZE);
 
 	if (feature_split_evtchn)
 		err = setup_netfront_split(queue);
@@ -1972,24 +1957,10 @@ static int setup_netfront(struct xenbus_device *dev,
 
 	return 0;
 
-	/* If we fail to setup netfront, it is safe to just revoke access to
-	 * granted pages because backend is not accessing it at this point.
-	 */
  fail:
-	if (queue->rx_ring_ref != INVALID_GRANT_REF) {
-		gnttab_end_foreign_access(queue->rx_ring_ref,
-					  (unsigned long)rxs);
-		queue->rx_ring_ref = INVALID_GRANT_REF;
-	} else {
-		free_page((unsigned long)rxs);
-	}
-	if (queue->tx_ring_ref != INVALID_GRANT_REF) {
-		gnttab_end_foreign_access(queue->tx_ring_ref,
-					  (unsigned long)txs);
-		queue->tx_ring_ref = INVALID_GRANT_REF;
-	} else {
-		free_page((unsigned long)txs);
-	}
+	xenbus_teardown_ring((void **)&queue->rx.sring, 1, &queue->rx_ring_ref);
+	xenbus_teardown_ring((void **)&queue->tx.sring, 1, &queue->tx_ring_ref);
+
 	return err;
 }
 
-- 
2.34.1

[PATCH v2 13/19] xen/tpmfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

Simplify tpmfront's ring creation and removal via xenbus_setup_ring()
and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/char/tpm/xen-tpmfront.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index 69df04ae2401..379291826261 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -253,20 +253,12 @@ static int setup_ring(struct xenbus_device *dev, struct tpm_private *priv)
 	struct xenbus_transaction xbt;
 	const char *message = NULL;
 	int rv;
-	grant_ref_t gref;
 
-	priv->shr = (void *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
-	if (!priv->shr) {
-		xenbus_dev_fatal(dev, -ENOMEM, "allocating shared ring");
-		return -ENOMEM;
-	}
-
-	rv = xenbus_grant_ring(dev, priv->shr, 1, &gref);
+	rv = xenbus_setup_ring(dev, GFP_KERNEL, (void **)&priv->shr, 1,
+			       &priv->ring_ref);
 	if (rv < 0)
 		return rv;
 
-	priv->ring_ref = gref;
-
 	rv = xenbus_alloc_evtchn(dev, &priv->evtchn);
 	if (rv)
 		return rv;
@@ -331,11 +323,7 @@ static void ring_free(struct tpm_private *priv)
 	if (!priv)
 		return;
 
-	if (priv->ring_ref)
-		gnttab_end_foreign_access(priv->ring_ref,
-				(unsigned long)priv->shr);
-	else
-		free_page((unsigned long)priv->shr);
+	xenbus_teardown_ring((void **)&priv->shr, 1, &priv->ring_ref);
 
 	if (priv->irq)
 		unbind_from_irqhandler(priv->irq, priv);
-- 
2.34.1

[PATCH v2 14/19] xen/drmfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

Simplify drmfront's ring creation and removal via xenbus_setup_ring()
and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.c | 43 ++++++---------------
 1 file changed, 11 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c b/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
index 4006568b9e32..e52afd792346 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
@@ -123,12 +123,12 @@ static irqreturn_t evtchnl_interrupt_evt(int irq, void *dev_id)
 static void evtchnl_free(struct xen_drm_front_info *front_info,
 			 struct xen_drm_front_evtchnl *evtchnl)
 {
-	unsigned long page = 0;
+	void *page = NULL;
 
 	if (evtchnl->type == EVTCHNL_TYPE_REQ)
-		page = (unsigned long)evtchnl->u.req.ring.sring;
+		page = evtchnl->u.req.ring.sring;
 	else if (evtchnl->type == EVTCHNL_TYPE_EVT)
-		page = (unsigned long)evtchnl->u.evt.page;
+		page = evtchnl->u.evt.page;
 	if (!page)
 		return;
 
@@ -147,8 +147,7 @@ static void evtchnl_free(struct xen_drm_front_info *front_info,
 		xenbus_free_evtchn(front_info->xb_dev, evtchnl->port);
 
 	/* end access and free the page */
-	if (evtchnl->gref != INVALID_GRANT_REF)
-		gnttab_end_foreign_access(evtchnl->gref, page);
+	xenbus_teardown_ring(&page, 1, &evtchnl->gref);
 
 	memset(evtchnl, 0, sizeof(*evtchnl));
 }
@@ -158,8 +157,7 @@ static int evtchnl_alloc(struct xen_drm_front_info *front_info, int index,
 			 enum xen_drm_front_evtchnl_type type)
 {
 	struct xenbus_device *xb_dev = front_info->xb_dev;
-	unsigned long page;
-	grant_ref_t gref;
+	void *page;
 	irq_handler_t handler;
 	int ret;
 
@@ -168,44 +166,25 @@ static int evtchnl_alloc(struct xen_drm_front_info *front_info, int index,
 	evtchnl->index = index;
 	evtchnl->front_info = front_info;
 	evtchnl->state = EVTCHNL_STATE_DISCONNECTED;
-	evtchnl->gref = INVALID_GRANT_REF;
 
-	page = get_zeroed_page(GFP_NOIO | __GFP_HIGH);
-	if (!page) {
-		ret = -ENOMEM;
+	ret = xenbus_setup_ring(xb_dev, GFP_NOIO | __GFP_HIGH, &page,
+				1, &evtchnl->gref);
+	if (ret)
 		goto fail;
-	}
 
 	if (type == EVTCHNL_TYPE_REQ) {
 		struct xen_displif_sring *sring;
 
 		init_completion(&evtchnl->u.req.completion);
 		mutex_init(&evtchnl->u.req.req_io_lock);
-		sring = (struct xen_displif_sring *)page;
-		SHARED_RING_INIT(sring);
-		FRONT_RING_INIT(&evtchnl->u.req.ring, sring, XEN_PAGE_SIZE);
-
-		ret = xenbus_grant_ring(xb_dev, sring, 1, &gref);
-		if (ret < 0) {
-			evtchnl->u.req.ring.sring = NULL;
-			free_page(page);
-			goto fail;
-		}
+		sring = page;
+		XEN_FRONT_RING_INIT(&evtchnl->u.req.ring, sring, XEN_PAGE_SIZE);
 
 		handler = evtchnl_interrupt_ctrl;
 	} else {
-		ret = gnttab_grant_foreign_access(xb_dev->otherend_id,
-						  virt_to_gfn((void *)page), 0);
-		if (ret < 0) {
-			free_page(page);
-			goto fail;
-		}
-
-		evtchnl->u.evt.page = (struct xendispl_event_page *)page;
-		gref = ret;
+		evtchnl->u.evt.page = page;
 		handler = evtchnl_interrupt_evt;
 	}
-	evtchnl->gref = gref;
 
 	ret = xenbus_alloc_evtchn(xb_dev, &evtchnl->port);
 	if (ret < 0)
-- 
2.34.1

Re: [PATCH v2 14/19] xen/drmfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Oleksandr]

On 28.04.22 11:27, Juergen Gross wrote:

Hello Juergen, all

> Simplify drmfront's ring creation and removal via xenbus_setup_ring()
> and xenbus_teardown_ring().
>
> Signed-off-by: Juergen Gross <jgross@suse.com>


I am not familiar with DRM bits of this driver, but a little bit 
familiar with Xen bits this patch only touches and I have environment to 
test.

Xen specific changes looks good to me. Also I didn't see any specific to 
this series issues when testing virtulized display driver except one I 
have already pointed out in PATCH v2 08/19.

root@salvator-x-h3-4x2g-xt-domu:~# dmesg | grep drm
[    0.158190] [drm] Registering XEN PV vdispl
[    0.159620] [drm] Connector device/vdispl/0/0: resolution 1920x1080
[    0.159888] [drm] Have 1 connector(s)
[    0.289069] [drm] Creating Xen PV DRM Display Unit
[    0.289873] [drm] Initialized xendrm-du 1.0.0 20180221 for vdispl-0 
on minor 0
[    0.289918] [drm] Initialized xendrm-du 1.0.0 20180221 on minor 0


root@generic-armv8-xt-dom0:~# xenstore-ls -f | grep vdispl
/local/domain/1/backend/vdispl = ""
/local/domain/1/backend/vdispl/2 = ""
/local/domain/1/backend/vdispl/2/0 = ""
/local/domain/1/backend/vdispl/2/0/frontend = 
"/local/domain/2/device/vdispl/0"
/local/domain/1/backend/vdispl/2/0/frontend-id = "2"
/local/domain/1/backend/vdispl/2/0/online = "1"
/local/domain/1/backend/vdispl/2/0/state = "4"
/local/domain/2/device/vdispl = ""
/local/domain/2/device/vdispl/0 = ""
/local/domain/2/device/vdispl/0/backend = 
"/local/domain/1/backend/vdispl/2/0"
/local/domain/2/device/vdispl/0/backend-id = "1"
/local/domain/2/device/vdispl/0/state = "4"
/local/domain/2/device/vdispl/0/be-alloc = "0"
/local/domain/2/device/vdispl/0/0 = ""
/local/domain/2/device/vdispl/0/0/resolution = "1920x1080"
/local/domain/2/device/vdispl/0/0/unique-id = "HDMI-A-1"
/local/domain/2/device/vdispl/0/0/req-ring-ref = "8"
/local/domain/2/device/vdispl/0/0/req-event-channel = "7"
/local/domain/2/device/vdispl/0/0/evt-ring-ref = "9"
/local/domain/2/device/vdispl/0/0/evt-event-channel = "8"
/libxl/2/device/vdispl = ""
/libxl/2/device/vdispl/0 = ""
/libxl/2/device/vdispl/0/frontend = "/local/domain/2/device/vdispl/0"
/libxl/2/device/vdispl/0/backend = "/local/domain/1/backend/vdispl/2/0"
/libxl/2/device/vdispl/0/frontend-id = "2"
/libxl/2/device/vdispl/0/online = "1"
/libxl/2/device/vdispl/0/state = "1"


It worth mentioning I noticed one issue, but I believe it is not related 
to your series.

root@salvator-x-h3-4x2g-xt-domu:~# modetest -M xendrm-du -s 31:1920x1080
[   62.431887] ------------[ cut here ]------------
[   62.431940] WARNING: CPU: 0 PID: 244 at 
drivers/gpu/drm/drm_gem.c:1055 drm_gem_mmap_obj+0x16c/0x180
[   62.432000] Modules linked in:
[   62.432025] CPU: 0 PID: 244 Comm: modetest Tainted: G W         
5.18.0-rc4-yocto-standard-00096-g936342d8fae2 #1
[   62.432067] Hardware name: XENVM-4.17 (DT)
[   62.432089] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[   62.432126] pc : drm_gem_mmap_obj+0x16c/0x180
[   62.432153] lr : drm_gem_mmap_obj+0x78/0x180
[   62.432178] sp : ffff800009da3bb0
[   62.432196] x29: ffff800009da3bb0 x28: 0000000000000008 x27: 
ffff0001c3a56780
[   62.432237] x26: ffff0001c3a56f00 x25: 00000000000007e9 x24: 
ffff0001c23dec00
[   62.432279] x23: ffff0001c0c98000 x22: ffff0001c2162b80 x21: 
0000000000000000
[   62.432320] x20: ffff0001c3a56780 x19: ffff0001c23dea00 x18: 
0000000000000001
[   62.432355] x17: 0000000000000000 x16: 0000000000000000 x15: 
000000000003603c
[   62.432394] x14: 0000000000000000 x13: 0000000000000000 x12: 
0000000000000000
[   62.432430] x11: 0000000000100000 x10: 0000ffff88071000 x9 : 
ffff0001c0f17e70
[   62.432470] x8 : ffff8001f65ce000 x7 : 0000000000000001 x6 : 
ffff0001c388a000
[   62.432505] x5 : ffff800009da3a10 x4 : 0000000000000090 x3 : 
0000000010046400
[   62.432539] x2 : 00000000000007e9 x1 : 9b0023a536f4f400 x0 : 
00000000100000fb
[   62.432579] Call trace:
[   62.432593]  drm_gem_mmap_obj+0x16c/0x180
[   62.432617]  drm_gem_mmap+0x128/0x228
[   62.432641]  mmap_region+0x384/0x5a0
[   62.432667]  do_mmap+0x354/0x4f0
[   62.432687]  vm_mmap_pgoff+0xdc/0x108
[   62.432710]  ksys_mmap_pgoff+0x1b8/0x208
[   62.432734]  __arm64_sys_mmap+0x30/0x48
[   62.432760]  invoke_syscall+0x44/0x108
[   62.432783]  el0_svc_common.constprop.0+0xcc/0xf0
[   62.432811]  do_el0_svc+0x24/0x88
[   62.432831]  el0_svc+0x2c/0x88
[   62.432855]  el0t_64_sync_handler+0xb0/0xb8
[   62.432875]  el0t_64_sync+0x18c/0x190
[   62.432898] ---[ end trace 0000000000000000 ]---
setting mode 1920x1080-60.00Hz@XR24 on connectors 31, crtc 34


Although we see that WARNING, the application still works. Looking into 
the code, I assume that problem is that frontend doesn't set the 
VM_DONTEXPAND flag in mmap callback.

This diff fixes an issue in my environment:

index 5a5bf4e..e31554d 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -71,7 +71,7 @@ static int xen_drm_front_gem_object_mmap(struct 
drm_gem_object *gem_obj,
          * the whole buffer.
          */
         vma->vm_flags &= ~VM_PFNMAP;
-       vma->vm_flags |= VM_MIXEDMAP;
+       vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
         vma->vm_pgoff = 0;

         /*


I am not 100% sure whether it is a proper fix, so I would kindly ask DRM 
folks to confirm. I will be able to send a formal patch then.


> ---
>   drivers/gpu/drm/xen/xen_drm_front_evtchnl.c | 43 ++++++---------------
>   1 file changed, 11 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c b/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
> index 4006568b9e32..e52afd792346 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
> @@ -123,12 +123,12 @@ static irqreturn_t evtchnl_interrupt_evt(int irq, void *dev_id)
>   static void evtchnl_free(struct xen_drm_front_info *front_info,
>   			 struct xen_drm_front_evtchnl *evtchnl)
>   {
> -	unsigned long page = 0;
> +	void *page = NULL;
>   
>   	if (evtchnl->type == EVTCHNL_TYPE_REQ)
> -		page = (unsigned long)evtchnl->u.req.ring.sring;
> +		page = evtchnl->u.req.ring.sring;
>   	else if (evtchnl->type == EVTCHNL_TYPE_EVT)
> -		page = (unsigned long)evtchnl->u.evt.page;
> +		page = evtchnl->u.evt.page;
>   	if (!page)
>   		return;
>   
> @@ -147,8 +147,7 @@ static void evtchnl_free(struct xen_drm_front_info *front_info,
>   		xenbus_free_evtchn(front_info->xb_dev, evtchnl->port);
>   
>   	/* end access and free the page */
> -	if (evtchnl->gref != INVALID_GRANT_REF)
> -		gnttab_end_foreign_access(evtchnl->gref, page);
> +	xenbus_teardown_ring(&page, 1, &evtchnl->gref);
>   
>   	memset(evtchnl, 0, sizeof(*evtchnl));
>   }
> @@ -158,8 +157,7 @@ static int evtchnl_alloc(struct xen_drm_front_info *front_info, int index,
>   			 enum xen_drm_front_evtchnl_type type)
>   {
>   	struct xenbus_device *xb_dev = front_info->xb_dev;
> -	unsigned long page;
> -	grant_ref_t gref;
> +	void *page;
>   	irq_handler_t handler;
>   	int ret;
>   
> @@ -168,44 +166,25 @@ static int evtchnl_alloc(struct xen_drm_front_info *front_info, int index,
>   	evtchnl->index = index;
>   	evtchnl->front_info = front_info;
>   	evtchnl->state = EVTCHNL_STATE_DISCONNECTED;
> -	evtchnl->gref = INVALID_GRANT_REF;
>   
> -	page = get_zeroed_page(GFP_NOIO | __GFP_HIGH);
> -	if (!page) {
> -		ret = -ENOMEM;
> +	ret = xenbus_setup_ring(xb_dev, GFP_NOIO | __GFP_HIGH, &page,
> +				1, &evtchnl->gref);
> +	if (ret)
>   		goto fail;
> -	}
>   
>   	if (type == EVTCHNL_TYPE_REQ) {
>   		struct xen_displif_sring *sring;
>   
>   		init_completion(&evtchnl->u.req.completion);
>   		mutex_init(&evtchnl->u.req.req_io_lock);
> -		sring = (struct xen_displif_sring *)page;
> -		SHARED_RING_INIT(sring);
> -		FRONT_RING_INIT(&evtchnl->u.req.ring, sring, XEN_PAGE_SIZE);
> -
> -		ret = xenbus_grant_ring(xb_dev, sring, 1, &gref);
> -		if (ret < 0) {
> -			evtchnl->u.req.ring.sring = NULL;
> -			free_page(page);
> -			goto fail;
> -		}
> +		sring = page;
> +		XEN_FRONT_RING_INIT(&evtchnl->u.req.ring, sring, XEN_PAGE_SIZE);
>   
>   		handler = evtchnl_interrupt_ctrl;
>   	} else {
> -		ret = gnttab_grant_foreign_access(xb_dev->otherend_id,
> -						  virt_to_gfn((void *)page), 0);
> -		if (ret < 0) {
> -			free_page(page);
> -			goto fail;
> -		}
> -
> -		evtchnl->u.evt.page = (struct xendispl_event_page *)page;
> -		gref = ret;
> +		evtchnl->u.evt.page = page;
>   		handler = evtchnl_interrupt_evt;
>   	}
> -	evtchnl->gref = gref;
>   
>   	ret = xenbus_alloc_evtchn(xb_dev, &evtchnl->port);
>   	if (ret < 0)


-- 
Regards,

Oleksandr Tyshchenko

[PATCH v2 15/19] xen/pcifront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

Simplify pcifront's shared page creation and removal via
xenbus_setup_ring() and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/pci/xen-pcifront.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 3edc1565a27c..689271c4245c 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -709,9 +709,8 @@ static struct pcifront_device *alloc_pdev(struct xenbus_device *xdev)
 	if (pdev == NULL)
 		goto out;
 
-	pdev->sh_info =
-	    (struct xen_pci_sharedinfo *)__get_free_page(GFP_KERNEL);
-	if (pdev->sh_info == NULL) {
+	if (xenbus_setup_ring(xdev, GFP_KERNEL, (void **)&pdev->sh_info, 1,
+			      &pdev->gnt_ref)) {
 		kfree(pdev);
 		pdev = NULL;
 		goto out;
@@ -729,7 +728,6 @@ static struct pcifront_device *alloc_pdev(struct xenbus_device *xdev)
 	spin_lock_init(&pdev->sh_info_lock);
 
 	pdev->evtchn = INVALID_EVTCHN;
-	pdev->gnt_ref = INVALID_GRANT_REF;
 	pdev->irq = -1;
 
 	INIT_WORK(&pdev->op_work, pcifront_do_aer);
@@ -754,11 +752,7 @@ static void free_pdev(struct pcifront_device *pdev)
 	if (pdev->evtchn != INVALID_EVTCHN)
 		xenbus_free_evtchn(pdev->xdev, pdev->evtchn);
 
-	if (pdev->gnt_ref != INVALID_GRANT_REF)
-		gnttab_end_foreign_access(pdev->gnt_ref,
-					  (unsigned long)pdev->sh_info);
-	else
-		free_page((unsigned long)pdev->sh_info);
+	xenbus_teardown_ring((void **)&pdev->sh_info, 1, &pdev->gnt_ref);
 
 	dev_set_drvdata(&pdev->xdev->dev, NULL);
 
@@ -769,13 +763,6 @@ static int pcifront_publish_info(struct pcifront_device *pdev)
 {
 	int err = 0;
 	struct xenbus_transaction trans;
-	grant_ref_t gref;
-
-	err = xenbus_grant_ring(pdev->xdev, pdev->sh_info, 1, &gref);
-	if (err < 0)
-		goto out;
-
-	pdev->gnt_ref = gref;
 
 	err = xenbus_alloc_evtchn(pdev->xdev, &pdev->evtchn);
 	if (err)
-- 
2.34.1

[PATCH v2 16/19] xen/scsifront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

Simplify scsifront's ring creation and removal via xenbus_setup_ring()
and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/scsi/xen-scsifront.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index 4c55e479fc36..51afc66e839d 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -798,27 +798,15 @@ static int scsifront_alloc_ring(struct vscsifrnt_info *info)
 {
 	struct xenbus_device *dev = info->dev;
 	struct vscsiif_sring *sring;
-	grant_ref_t gref;
-	int err = -ENOMEM;
+	int err;
 
 	/***** Frontend to Backend ring start *****/
-	sring = (struct vscsiif_sring *)__get_free_page(GFP_KERNEL);
-	if (!sring) {
-		xenbus_dev_fatal(dev, err,
-			"fail to allocate shared ring (Front to Back)");
+	err = xenbus_setup_ring(dev, GFP_KERNEL, (void **)&sring, 1,
+				&info->ring_ref);
+	if (err)
 		return err;
-	}
-	SHARED_RING_INIT(sring);
-	FRONT_RING_INIT(&info->ring, sring, PAGE_SIZE);
 
-	err = xenbus_grant_ring(dev, sring, 1, &gref);
-	if (err < 0) {
-		free_page((unsigned long)sring);
-		xenbus_dev_fatal(dev, err,
-			"fail to grant shared ring (Front to Back)");
-		return err;
-	}
-	info->ring_ref = gref;
+	XEN_FRONT_RING_INIT(&info->ring, sring, PAGE_SIZE);
 
 	err = xenbus_alloc_evtchn(dev, &info->evtchn);
 	if (err) {
@@ -847,8 +835,7 @@ static int scsifront_alloc_ring(struct vscsifrnt_info *info)
 free_irq:
 	unbind_from_irqhandler(info->irq, info);
 free_gnttab:
-	gnttab_end_foreign_access(info->ring_ref,
-				  (unsigned long)info->ring.sring);
+	xenbus_teardown_ring((void **)&sring, 1, &info->ring_ref);
 
 	return err;
 }
@@ -856,8 +843,7 @@ static int scsifront_alloc_ring(struct vscsifrnt_info *info)
 static void scsifront_free_ring(struct vscsifrnt_info *info)
 {
 	unbind_from_irqhandler(info->irq, info);
-	gnttab_end_foreign_access(info->ring_ref,
-				  (unsigned long)info->ring.sring);
+	xenbus_teardown_ring((void **)&info->ring.sring, 1, &info->ring_ref);
 }
 
 static int scsifront_init_ring(struct vscsifrnt_info *info)
-- 
2.34.1

[PATCH v2 17/19] xen/usbfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

Simplify xen-hcd's ring creation and removal via xenbus_setup_ring()
and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/host/xen-hcd.c | 61 ++++++++++----------------------------
 1 file changed, 15 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/host/xen-hcd.c b/drivers/usb/host/xen-hcd.c
index 9cbc7c2dab02..de1b09158318 100644
--- a/drivers/usb/host/xen-hcd.c
+++ b/drivers/usb/host/xen-hcd.c
@@ -1098,19 +1098,10 @@ static void xenhcd_destroy_rings(struct xenhcd_info *info)
 		unbind_from_irqhandler(info->irq, info);
 	info->irq = 0;
 
-	if (info->urb_ring_ref != INVALID_GRANT_REF) {
-		gnttab_end_foreign_access(info->urb_ring_ref,
-					  (unsigned long)info->urb_ring.sring);
-		info->urb_ring_ref = INVALID_GRANT_REF;
-	}
-	info->urb_ring.sring = NULL;
-
-	if (info->conn_ring_ref != INVALID_GRANT_REF) {
-		gnttab_end_foreign_access(info->conn_ring_ref,
-					  (unsigned long)info->conn_ring.sring);
-		info->conn_ring_ref = INVALID_GRANT_REF;
-	}
-	info->conn_ring.sring = NULL;
+	xenbus_teardown_ring((void **)&info->urb_ring.sring, 1,
+			     &info->urb_ring_ref);
+	xenbus_teardown_ring((void **)&info->conn_ring.sring, 1,
+			     &info->conn_ring_ref);
 }
 
 static int xenhcd_setup_rings(struct xenbus_device *dev,
@@ -1118,46 +1109,24 @@ static int xenhcd_setup_rings(struct xenbus_device *dev,
 {
 	struct xenusb_urb_sring *urb_sring;
 	struct xenusb_conn_sring *conn_sring;
-	grant_ref_t gref;
 	int err;
 
-	info->urb_ring_ref = INVALID_GRANT_REF;
 	info->conn_ring_ref = INVALID_GRANT_REF;
-
-	urb_sring = (struct xenusb_urb_sring *)get_zeroed_page(
-							GFP_NOIO | __GFP_HIGH);
-	if (!urb_sring) {
-		xenbus_dev_fatal(dev, -ENOMEM, "allocating urb ring");
-		return -ENOMEM;
-	}
-	SHARED_RING_INIT(urb_sring);
-	FRONT_RING_INIT(&info->urb_ring, urb_sring, PAGE_SIZE);
-
-	err = xenbus_grant_ring(dev, urb_sring, 1, &gref);
-	if (err < 0) {
-		free_page((unsigned long)urb_sring);
-		info->urb_ring.sring = NULL;
-		goto fail;
-	}
-	info->urb_ring_ref = gref;
-
-	conn_sring = (struct xenusb_conn_sring *)get_zeroed_page(
-							GFP_NOIO | __GFP_HIGH);
-	if (!conn_sring) {
-		xenbus_dev_fatal(dev, -ENOMEM, "allocating conn ring");
-		err = -ENOMEM;
-		goto fail;
+	err = xenbus_setup_ring(dev, GFP_NOIO | __GFP_HIGH,
+				(void **)&urb_sring, 1, &info->urb_ring_ref);
+	if (err) {
+		xenbus_dev_fatal(dev, err, "allocating urb ring");
+		return err;
 	}
-	SHARED_RING_INIT(conn_sring);
-	FRONT_RING_INIT(&info->conn_ring, conn_sring, PAGE_SIZE);
+	XEN_FRONT_RING_INIT(&info->urb_ring, urb_sring, PAGE_SIZE);
 
-	err = xenbus_grant_ring(dev, conn_sring, 1, &gref);
-	if (err < 0) {
-		free_page((unsigned long)conn_sring);
-		info->conn_ring.sring = NULL;
+	err = xenbus_setup_ring(dev, GFP_NOIO | __GFP_HIGH,
+				(void **)&conn_sring, 1, &info->conn_ring_ref);
+	if (err) {
+		xenbus_dev_fatal(dev, err, "allocating conn ring");
 		goto fail;
 	}
-	info->conn_ring_ref = gref;
+	XEN_FRONT_RING_INIT(&info->conn_ring, conn_sring, PAGE_SIZE);
 
 	err = xenbus_alloc_evtchn(dev, &info->evtchn);
 	if (err) {
-- 
2.34.1

[PATCH v2 18/19] xen/sndfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

Simplify sndfront's ring creation and removal via xenbus_setup_ring()
and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 sound/xen/xen_snd_front_evtchnl.c | 44 +++++++------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/sound/xen/xen_snd_front_evtchnl.c b/sound/xen/xen_snd_front_evtchnl.c
index 3e21369c8216..26d1b3987887 100644
--- a/sound/xen/xen_snd_front_evtchnl.c
+++ b/sound/xen/xen_snd_front_evtchnl.c
@@ -143,12 +143,12 @@ void xen_snd_front_evtchnl_flush(struct xen_snd_front_evtchnl *channel)
 static void evtchnl_free(struct xen_snd_front_info *front_info,
 			 struct xen_snd_front_evtchnl *channel)
 {
-	unsigned long page = 0;
+	void *page = NULL;
 
 	if (channel->type == EVTCHNL_TYPE_REQ)
-		page = (unsigned long)channel->u.req.ring.sring;
+		page = channel->u.req.ring.sring;
 	else if (channel->type == EVTCHNL_TYPE_EVT)
-		page = (unsigned long)channel->u.evt.page;
+		page = channel->u.evt.page;
 
 	if (!page)
 		return;
@@ -167,10 +167,7 @@ static void evtchnl_free(struct xen_snd_front_info *front_info,
 		xenbus_free_evtchn(front_info->xb_dev, channel->port);
 
 	/* End access and free the page. */
-	if (channel->gref != INVALID_GRANT_REF)
-		gnttab_end_foreign_access(channel->gref, page);
-	else
-		free_page(page);
+	xenbus_teardown_ring(&page, 1, &channel->gref);
 
 	memset(channel, 0, sizeof(*channel));
 }
@@ -196,8 +193,7 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
 			 enum xen_snd_front_evtchnl_type type)
 {
 	struct xenbus_device *xb_dev = front_info->xb_dev;
-	unsigned long page;
-	grant_ref_t gref;
+	void *page;
 	irq_handler_t handler;
 	char *handler_name = NULL;
 	int ret;
@@ -207,12 +203,9 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
 	channel->index = index;
 	channel->front_info = front_info;
 	channel->state = EVTCHNL_STATE_DISCONNECTED;
-	channel->gref = INVALID_GRANT_REF;
-	page = get_zeroed_page(GFP_KERNEL);
-	if (!page) {
-		ret = -ENOMEM;
+	ret = xenbus_setup_ring(xb_dev, GFP_KERNEL, &page, 1, &channel->gref);
+	if (ret)
 		goto fail;
-	}
 
 	handler_name = kasprintf(GFP_KERNEL, "%s-%s", XENSND_DRIVER_NAME,
 				 type == EVTCHNL_TYPE_REQ ?
@@ -226,33 +219,18 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
 	mutex_init(&channel->ring_io_lock);
 
 	if (type == EVTCHNL_TYPE_REQ) {
-		struct xen_sndif_sring *sring = (struct xen_sndif_sring *)page;
+		struct xen_sndif_sring *sring = page;
 
 		init_completion(&channel->u.req.completion);
 		mutex_init(&channel->u.req.req_io_lock);
-		SHARED_RING_INIT(sring);
-		FRONT_RING_INIT(&channel->u.req.ring, sring, XEN_PAGE_SIZE);
-
-		ret = xenbus_grant_ring(xb_dev, sring, 1, &gref);
-		if (ret < 0) {
-			channel->u.req.ring.sring = NULL;
-			goto fail;
-		}
+		XEN_FRONT_RING_INIT(&channel->u.req.ring, sring, XEN_PAGE_SIZE);
 
 		handler = evtchnl_interrupt_req;
 	} else {
-		ret = gnttab_grant_foreign_access(xb_dev->otherend_id,
-						  virt_to_gfn((void *)page), 0);
-		if (ret < 0)
-			goto fail;
-
-		channel->u.evt.page = (struct xensnd_event_page *)page;
-		gref = ret;
+		channel->u.evt.page = page;
 		handler = evtchnl_interrupt_evt;
 	}
 
-	channel->gref = gref;
-
 	ret = xenbus_alloc_evtchn(xb_dev, &channel->port);
 	if (ret < 0)
 		goto fail;
@@ -279,8 +257,6 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
 	return 0;
 
 fail:
-	if (page)
-		free_page(page);
 	kfree(handler_name);
 	dev_err(&xb_dev->dev, "Failed to allocate ring: %d\n", ret);
 	return ret;
-- 
2.34.1

Re: [PATCH v2 18/19] xen/sndfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Oleksandr]

On 28.04.22 11:27, Juergen Gross wrote:

Hello Juergen, all

> Simplify sndfront's ring creation and removal via xenbus_setup_ring()
> and xenbus_teardown_ring().
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

I am not familiar with SOUND bits of this driver, but a little bit
familiar with Xen bits this patch only touches and I have environment to
test.

Xen specific changes looks good to me. Also I didn't see any issues when 
testing virtulized sound driver with current series except one I have 
already pointed out in PATCH v2 08/19.


root@salvator-x-h3-4x2g-xt-domu:~# dmesg | grep vsnd
[    0.432181] Initialising Xen vsnd frontend driver


root@salvator-x-h3-4x2g-xt-domu:~# aplay -l
**** List of PLAYBACK Hardware Devices ****
card 0: vsnd [], device 0: dev1 [Virtual card PCM]
   Subdevices: 1/1
   Subdevice #0: subdevice #0

root@generic-armv8-xt-dom0:~# xenstore-ls -f | grep vsnd
/local/domain/1/backend/vsnd = ""
/local/domain/1/backend/vsnd/6 = ""
/local/domain/1/backend/vsnd/6/0 = ""
/local/domain/1/backend/vsnd/6/0/frontend = "/local/domain/6/device/vsnd/0"
/local/domain/1/backend/vsnd/6/0/frontend-id = "6"
/local/domain/1/backend/vsnd/6/0/online = "1"
/local/domain/1/backend/vsnd/6/0/state = "4"
/local/domain/6/device/vsnd = ""
/local/domain/6/device/vsnd/0 = ""
/local/domain/6/device/vsnd/0/backend = "/local/domain/1/backend/vsnd/6/0"
/local/domain/6/device/vsnd/0/backend-id = "1"
/local/domain/6/device/vsnd/0/state = "4"
/local/domain/6/device/vsnd/0/long-name = "Virtual sound card"
/local/domain/6/device/vsnd/0/short-name = "VCard"
/local/domain/6/device/vsnd/0/sample-rates = 
"8000,11025,16000,22050,32000,44100,48000"
/local/domain/6/device/vsnd/0/sample-formats = "s16_le"
/local/domain/6/device/vsnd/0/buffer-size = "65536"
/local/domain/6/device/vsnd/0/0 = ""
/local/domain/6/device/vsnd/0/0/name = "dev1"
/local/domain/6/device/vsnd/0/0/0 = ""
/local/domain/6/device/vsnd/0/0/0/unique-id = "pulse"
/local/domain/6/device/vsnd/0/0/0/type = "p"
/local/domain/6/device/vsnd/0/0/0/ring-ref = "2070"
/local/domain/6/device/vsnd/0/0/0/event-channel = "18"
/local/domain/6/device/vsnd/0/0/0/evt-ring-ref = "2071"
/local/domain/6/device/vsnd/0/0/0/evt-event-channel = "19"
/libxl/6/device/vsnd = ""
/libxl/6/device/vsnd/0 = ""
/libxl/6/device/vsnd/0/frontend = "/local/domain/6/device/vsnd/0"
/libxl/6/device/vsnd/0/backend = "/local/domain/1/backend/vsnd/6/0"
/libxl/6/device/vsnd/0/frontend-id = "6"
/libxl/6/device/vsnd/0/online = "1"
/libxl/6/device/vsnd/0/state = "1"


> ---
>   sound/xen/xen_snd_front_evtchnl.c | 44 +++++++------------------------
>   1 file changed, 10 insertions(+), 34 deletions(-)
>
> diff --git a/sound/xen/xen_snd_front_evtchnl.c b/sound/xen/xen_snd_front_evtchnl.c
> index 3e21369c8216..26d1b3987887 100644
> --- a/sound/xen/xen_snd_front_evtchnl.c
> +++ b/sound/xen/xen_snd_front_evtchnl.c
> @@ -143,12 +143,12 @@ void xen_snd_front_evtchnl_flush(struct xen_snd_front_evtchnl *channel)
>   static void evtchnl_free(struct xen_snd_front_info *front_info,
>   			 struct xen_snd_front_evtchnl *channel)
>   {
> -	unsigned long page = 0;
> +	void *page = NULL;
>   
>   	if (channel->type == EVTCHNL_TYPE_REQ)
> -		page = (unsigned long)channel->u.req.ring.sring;
> +		page = channel->u.req.ring.sring;
>   	else if (channel->type == EVTCHNL_TYPE_EVT)
> -		page = (unsigned long)channel->u.evt.page;
> +		page = channel->u.evt.page;
>   
>   	if (!page)
>   		return;
> @@ -167,10 +167,7 @@ static void evtchnl_free(struct xen_snd_front_info *front_info,
>   		xenbus_free_evtchn(front_info->xb_dev, channel->port);
>   
>   	/* End access and free the page. */
> -	if (channel->gref != INVALID_GRANT_REF)
> -		gnttab_end_foreign_access(channel->gref, page);
> -	else
> -		free_page(page);
> +	xenbus_teardown_ring(&page, 1, &channel->gref);
>   
>   	memset(channel, 0, sizeof(*channel));
>   }
> @@ -196,8 +193,7 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
>   			 enum xen_snd_front_evtchnl_type type)
>   {
>   	struct xenbus_device *xb_dev = front_info->xb_dev;
> -	unsigned long page;
> -	grant_ref_t gref;
> +	void *page;
>   	irq_handler_t handler;
>   	char *handler_name = NULL;
>   	int ret;
> @@ -207,12 +203,9 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
>   	channel->index = index;
>   	channel->front_info = front_info;
>   	channel->state = EVTCHNL_STATE_DISCONNECTED;
> -	channel->gref = INVALID_GRANT_REF;
> -	page = get_zeroed_page(GFP_KERNEL);
> -	if (!page) {
> -		ret = -ENOMEM;
> +	ret = xenbus_setup_ring(xb_dev, GFP_KERNEL, &page, 1, &channel->gref);
> +	if (ret)
>   		goto fail;
> -	}
>   
>   	handler_name = kasprintf(GFP_KERNEL, "%s-%s", XENSND_DRIVER_NAME,
>   				 type == EVTCHNL_TYPE_REQ ?
> @@ -226,33 +219,18 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
>   	mutex_init(&channel->ring_io_lock);
>   
>   	if (type == EVTCHNL_TYPE_REQ) {
> -		struct xen_sndif_sring *sring = (struct xen_sndif_sring *)page;
> +		struct xen_sndif_sring *sring = page;
>   
>   		init_completion(&channel->u.req.completion);
>   		mutex_init(&channel->u.req.req_io_lock);
> -		SHARED_RING_INIT(sring);
> -		FRONT_RING_INIT(&channel->u.req.ring, sring, XEN_PAGE_SIZE);
> -
> -		ret = xenbus_grant_ring(xb_dev, sring, 1, &gref);
> -		if (ret < 0) {
> -			channel->u.req.ring.sring = NULL;
> -			goto fail;
> -		}
> +		XEN_FRONT_RING_INIT(&channel->u.req.ring, sring, XEN_PAGE_SIZE);
>   
>   		handler = evtchnl_interrupt_req;
>   	} else {
> -		ret = gnttab_grant_foreign_access(xb_dev->otherend_id,
> -						  virt_to_gfn((void *)page), 0);
> -		if (ret < 0)
> -			goto fail;
> -
> -		channel->u.evt.page = (struct xensnd_event_page *)page;
> -		gref = ret;
> +		channel->u.evt.page = page;
>   		handler = evtchnl_interrupt_evt;
>   	}
>   
> -	channel->gref = gref;
> -
>   	ret = xenbus_alloc_evtchn(xb_dev, &channel->port);
>   	if (ret < 0)
>   		goto fail;
> @@ -279,8 +257,6 @@ static int evtchnl_alloc(struct xen_snd_front_info *front_info, int index,
>   	return 0;
>   
>   fail:
> -	if (page)
> -		free_page(page);
>   	kfree(handler_name);
>   	dev_err(&xb_dev->dev, "Failed to allocate ring: %d\n", ret);
>   	return ret;

-- 
Regards,

Oleksandr Tyshchenko

[PATCH v2 19/19] xen/xenbus: eliminate xenbus_grant_ring()

[Juergen Gross]

There is no external user of xenbus_grant_ring() left, so merge it into
the only caller xenbus_setup_ring().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- make error message more precise (Andrew Cooper)
---
 drivers/xen/xenbus/xenbus_client.c | 65 +++++++++---------------------
 include/xen/xenbus.h               |  2 -
 2 files changed, 19 insertions(+), 48 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 1a2e0d94ccd1..d6fdd2d209d3 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -363,50 +363,6 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
 		__xenbus_switch_state(dev, XenbusStateClosing, 1);
 }
 
-/**
- * xenbus_grant_ring
- * @dev: xenbus device
- * @vaddr: starting virtual address of the ring
- * @nr_pages: number of pages to be granted
- * @grefs: grant reference array to be filled in
- *
- * Grant access to the given @vaddr to the peer of the given device.
- * Then fill in @grefs with grant references.  Return 0 on success, or
- * -errno on error.  On error, the device will switch to
- * XenbusStateClosing, and the error will be saved in the store.
- */
-int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
-		      unsigned int nr_pages, grant_ref_t *grefs)
-{
-	int err;
-	unsigned int i;
-	grant_ref_t gref_head;
-
-	err = gnttab_alloc_grant_references(nr_pages, &gref_head);
-	if (err) {
-		xenbus_dev_fatal(dev, err, "granting access to ring page");
-		return err;
-	}
-
-	for (i = 0; i < nr_pages; i++) {
-		unsigned long gfn;
-
-		if (is_vmalloc_addr(vaddr))
-			gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
-		else
-			gfn = virt_to_gfn(vaddr);
-
-		grefs[i] = gnttab_claim_grant_reference(&gref_head);
-		gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
-						gfn, 0);
-
-		vaddr = vaddr + XEN_PAGE_SIZE;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(xenbus_grant_ring);
-
 /*
  * xenbus_setup_ring
  * @dev: xenbus device
@@ -424,6 +380,7 @@ int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
 		      unsigned int nr_pages, grant_ref_t *grefs)
 {
 	unsigned long ring_size = nr_pages * XEN_PAGE_SIZE;
+	grant_ref_t gref_head;
 	unsigned int i;
 	int ret;
 
@@ -433,9 +390,25 @@ int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
 		goto err;
 	}
 
-	ret = xenbus_grant_ring(dev, *vaddr, nr_pages, grefs);
-	if (ret)
+	ret = gnttab_alloc_grant_references(nr_pages, &gref_head);
+	if (ret) {
+		xenbus_dev_fatal(dev, ret, "granting access to %u ring pages",
+				 nr_pages);
 		goto err;
+	}
+
+	for (i = 0; i < nr_pages; i++) {
+		unsigned long gfn;
+
+		if (is_vmalloc_addr(*vaddr))
+			gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr[i]));
+		else
+			gfn = virt_to_gfn(vaddr[i]);
+
+		grefs[i] = gnttab_claim_grant_reference(&gref_head);
+		gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
+						gfn, 0);
+	}
 
 	return 0;
 
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index b533b4adc835..eaa932b99d8a 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -224,8 +224,6 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
 			 const char *pathfmt, ...);
 
 int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
-int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
-		      unsigned int nr_pages, grant_ref_t *grefs);
 int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
 		      unsigned int nr_pages, grant_ref_t *grefs);
 void xenbus_teardown_ring(void **vaddr, unsigned int nr_pages,
-- 
2.34.1

Re: [PATCH v2 19/19] xen/xenbus: eliminate xenbus_grant_ring()

[Oleksandr]

On 28.04.22 11:27, Juergen Gross wrote:


Hello Juergen


> There is no external user of xenbus_grant_ring() left, so merge it into
> the only caller xenbus_setup_ring().
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - make error message more precise (Andrew Cooper)
> ---
>   drivers/xen/xenbus/xenbus_client.c | 65 +++++++++---------------------
>   include/xen/xenbus.h               |  2 -
>   2 files changed, 19 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index 1a2e0d94ccd1..d6fdd2d209d3 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -363,50 +363,6 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
>   		__xenbus_switch_state(dev, XenbusStateClosing, 1);
>   }
>   
> -/**
> - * xenbus_grant_ring
> - * @dev: xenbus device
> - * @vaddr: starting virtual address of the ring
> - * @nr_pages: number of pages to be granted
> - * @grefs: grant reference array to be filled in
> - *
> - * Grant access to the given @vaddr to the peer of the given device.
> - * Then fill in @grefs with grant references.  Return 0 on success, or
> - * -errno on error.  On error, the device will switch to
> - * XenbusStateClosing, and the error will be saved in the store.
> - */
> -int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
> -		      unsigned int nr_pages, grant_ref_t *grefs)
> -{
> -	int err;
> -	unsigned int i;
> -	grant_ref_t gref_head;
> -
> -	err = gnttab_alloc_grant_references(nr_pages, &gref_head);
> -	if (err) {
> -		xenbus_dev_fatal(dev, err, "granting access to ring page");
> -		return err;
> -	}
> -
> -	for (i = 0; i < nr_pages; i++) {
> -		unsigned long gfn;
> -
> -		if (is_vmalloc_addr(vaddr))
> -			gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
> -		else
> -			gfn = virt_to_gfn(vaddr);
> -
> -		grefs[i] = gnttab_claim_grant_reference(&gref_head);
> -		gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
> -						gfn, 0);
> -
> -		vaddr = vaddr + XEN_PAGE_SIZE;
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(xenbus_grant_ring);
> -
>   /*
>    * xenbus_setup_ring
>    * @dev: xenbus device
> @@ -424,6 +380,7 @@ int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
>   		      unsigned int nr_pages, grant_ref_t *grefs)
>   {
>   	unsigned long ring_size = nr_pages * XEN_PAGE_SIZE;
> +	grant_ref_t gref_head;
>   	unsigned int i;
>   	int ret;
>   
> @@ -433,9 +390,25 @@ int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
>   		goto err;
>   	}
>   
> -	ret = xenbus_grant_ring(dev, *vaddr, nr_pages, grefs);
> -	if (ret)
> +	ret = gnttab_alloc_grant_references(nr_pages, &gref_head);
> +	if (ret) {
> +		xenbus_dev_fatal(dev, ret, "granting access to %u ring pages",
> +				 nr_pages);
>   		goto err;
> +	}
> +
> +	for (i = 0; i < nr_pages; i++) {
> +		unsigned long gfn;
> +
> +		if (is_vmalloc_addr(*vaddr))
> +			gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr[i]));
> +		else
> +			gfn = virt_to_gfn(vaddr[i]);
> +
> +		grefs[i] = gnttab_claim_grant_reference(&gref_head);

gnttab_claim_grant_reference() can return error if no free grant 
reference remains.

I understand this patch only moves the code, but probably it would be 
better to add a missing check here (and likely rollback already 
processed grants if any?).



> +		gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
> +						gfn, 0);
> +	}
>   
>   	return 0;
>   
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index b533b4adc835..eaa932b99d8a 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -224,8 +224,6 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
>   			 const char *pathfmt, ...);
>   
>   int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> -int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
> -		      unsigned int nr_pages, grant_ref_t *grefs);
>   int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
>   		      unsigned int nr_pages, grant_ref_t *grefs);
>   void xenbus_teardown_ring(void **vaddr, unsigned int nr_pages,

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH v2 19/19] xen/xenbus: eliminate xenbus_grant_ring()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4426 bytes --]

On 29.04.22 17:10, Oleksandr wrote:
> 
> On 28.04.22 11:27, Juergen Gross wrote:
> 
> 
> Hello Juergen
> 
> 
>> There is no external user of xenbus_grant_ring() left, so merge it into
>> the only caller xenbus_setup_ring().
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - make error message more precise (Andrew Cooper)
>> ---
>>   drivers/xen/xenbus/xenbus_client.c | 65 +++++++++---------------------
>>   include/xen/xenbus.h               |  2 -
>>   2 files changed, 19 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_client.c 
>> b/drivers/xen/xenbus/xenbus_client.c
>> index 1a2e0d94ccd1..d6fdd2d209d3 100644
>> --- a/drivers/xen/xenbus/xenbus_client.c
>> +++ b/drivers/xen/xenbus/xenbus_client.c
>> @@ -363,50 +363,6 @@ static void xenbus_switch_fatal(struct xenbus_device 
>> *dev, int depth, int err,
>>           __xenbus_switch_state(dev, XenbusStateClosing, 1);
>>   }
>> -/**
>> - * xenbus_grant_ring
>> - * @dev: xenbus device
>> - * @vaddr: starting virtual address of the ring
>> - * @nr_pages: number of pages to be granted
>> - * @grefs: grant reference array to be filled in
>> - *
>> - * Grant access to the given @vaddr to the peer of the given device.
>> - * Then fill in @grefs with grant references.  Return 0 on success, or
>> - * -errno on error.  On error, the device will switch to
>> - * XenbusStateClosing, and the error will be saved in the store.
>> - */
>> -int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
>> -              unsigned int nr_pages, grant_ref_t *grefs)
>> -{
>> -    int err;
>> -    unsigned int i;
>> -    grant_ref_t gref_head;
>> -
>> -    err = gnttab_alloc_grant_references(nr_pages, &gref_head);
>> -    if (err) {
>> -        xenbus_dev_fatal(dev, err, "granting access to ring page");
>> -        return err;
>> -    }
>> -
>> -    for (i = 0; i < nr_pages; i++) {
>> -        unsigned long gfn;
>> -
>> -        if (is_vmalloc_addr(vaddr))
>> -            gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
>> -        else
>> -            gfn = virt_to_gfn(vaddr);
>> -
>> -        grefs[i] = gnttab_claim_grant_reference(&gref_head);
>> -        gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
>> -                        gfn, 0);
>> -
>> -        vaddr = vaddr + XEN_PAGE_SIZE;
>> -    }
>> -
>> -    return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(xenbus_grant_ring);
>> -
>>   /*
>>    * xenbus_setup_ring
>>    * @dev: xenbus device
>> @@ -424,6 +380,7 @@ int xenbus_setup_ring(struct xenbus_device *dev, gfp_t 
>> gfp, void **vaddr,
>>                 unsigned int nr_pages, grant_ref_t *grefs)
>>   {
>>       unsigned long ring_size = nr_pages * XEN_PAGE_SIZE;
>> +    grant_ref_t gref_head;
>>       unsigned int i;
>>       int ret;
>> @@ -433,9 +390,25 @@ int xenbus_setup_ring(struct xenbus_device *dev, gfp_t 
>> gfp, void **vaddr,
>>           goto err;
>>       }
>> -    ret = xenbus_grant_ring(dev, *vaddr, nr_pages, grefs);
>> -    if (ret)
>> +    ret = gnttab_alloc_grant_references(nr_pages, &gref_head);
>> +    if (ret) {
>> +        xenbus_dev_fatal(dev, ret, "granting access to %u ring pages",
>> +                 nr_pages);
>>           goto err;
>> +    }
>> +
>> +    for (i = 0; i < nr_pages; i++) {
>> +        unsigned long gfn;
>> +
>> +        if (is_vmalloc_addr(*vaddr))
>> +            gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr[i]));
>> +        else
>> +            gfn = virt_to_gfn(vaddr[i]);
>> +
>> +        grefs[i] = gnttab_claim_grant_reference(&gref_head);
> 
> gnttab_claim_grant_reference() can return error if no free grant reference remains.

This can happen only in case gnttab_alloc_grant_references() didn't
allocate enough grants but told us it succeeded doing so.

> I understand this patch only moves the code, but probably it would be better to 
> add a missing check here (and likely rollback already processed grants if any?).

I don't think this is needed, as this would be a clear bug in the code.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 19/19] xen/xenbus: eliminate xenbus_grant_ring()

[Oleksandr]

On 02.05.22 16:30, Juergen Gross wrote:

Hello Juergen


> On 29.04.22 17:10, Oleksandr wrote:
>>
>> On 28.04.22 11:27, Juergen Gross wrote:
>>
>>
>> Hello Juergen
>>
>>
>>> There is no external user of xenbus_grant_ring() left, so merge it into
>>> the only caller xenbus_setup_ring().
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V2:
>>> - make error message more precise (Andrew Cooper)
>>> ---
>>>   drivers/xen/xenbus/xenbus_client.c | 65 
>>> +++++++++---------------------
>>>   include/xen/xenbus.h               |  2 -
>>>   2 files changed, 19 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/drivers/xen/xenbus/xenbus_client.c 
>>> b/drivers/xen/xenbus/xenbus_client.c
>>> index 1a2e0d94ccd1..d6fdd2d209d3 100644
>>> --- a/drivers/xen/xenbus/xenbus_client.c
>>> +++ b/drivers/xen/xenbus/xenbus_client.c
>>> @@ -363,50 +363,6 @@ static void xenbus_switch_fatal(struct 
>>> xenbus_device *dev, int depth, int err,
>>>           __xenbus_switch_state(dev, XenbusStateClosing, 1);
>>>   }
>>> -/**
>>> - * xenbus_grant_ring
>>> - * @dev: xenbus device
>>> - * @vaddr: starting virtual address of the ring
>>> - * @nr_pages: number of pages to be granted
>>> - * @grefs: grant reference array to be filled in
>>> - *
>>> - * Grant access to the given @vaddr to the peer of the given device.
>>> - * Then fill in @grefs with grant references.  Return 0 on success, or
>>> - * -errno on error.  On error, the device will switch to
>>> - * XenbusStateClosing, and the error will be saved in the store.
>>> - */
>>> -int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
>>> -              unsigned int nr_pages, grant_ref_t *grefs)
>>> -{
>>> -    int err;
>>> -    unsigned int i;
>>> -    grant_ref_t gref_head;
>>> -
>>> -    err = gnttab_alloc_grant_references(nr_pages, &gref_head);
>>> -    if (err) {
>>> -        xenbus_dev_fatal(dev, err, "granting access to ring page");
>>> -        return err;
>>> -    }
>>> -
>>> -    for (i = 0; i < nr_pages; i++) {
>>> -        unsigned long gfn;
>>> -
>>> -        if (is_vmalloc_addr(vaddr))
>>> -            gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
>>> -        else
>>> -            gfn = virt_to_gfn(vaddr);
>>> -
>>> -        grefs[i] = gnttab_claim_grant_reference(&gref_head);
>>> -        gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
>>> -                        gfn, 0);
>>> -
>>> -        vaddr = vaddr + XEN_PAGE_SIZE;
>>> -    }
>>> -
>>> -    return 0;
>>> -}
>>> -EXPORT_SYMBOL_GPL(xenbus_grant_ring);
>>> -
>>>   /*
>>>    * xenbus_setup_ring
>>>    * @dev: xenbus device
>>> @@ -424,6 +380,7 @@ int xenbus_setup_ring(struct xenbus_device *dev, 
>>> gfp_t gfp, void **vaddr,
>>>                 unsigned int nr_pages, grant_ref_t *grefs)
>>>   {
>>>       unsigned long ring_size = nr_pages * XEN_PAGE_SIZE;
>>> +    grant_ref_t gref_head;
>>>       unsigned int i;
>>>       int ret;
>>> @@ -433,9 +390,25 @@ int xenbus_setup_ring(struct xenbus_device 
>>> *dev, gfp_t gfp, void **vaddr,
>>>           goto err;
>>>       }
>>> -    ret = xenbus_grant_ring(dev, *vaddr, nr_pages, grefs);
>>> -    if (ret)
>>> +    ret = gnttab_alloc_grant_references(nr_pages, &gref_head);
>>> +    if (ret) {
>>> +        xenbus_dev_fatal(dev, ret, "granting access to %u ring pages",
>>> +                 nr_pages);
>>>           goto err;
>>> +    }
>>> +
>>> +    for (i = 0; i < nr_pages; i++) {
>>> +        unsigned long gfn;
>>> +
>>> +        if (is_vmalloc_addr(*vaddr))
>>> +            gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr[i]));
>>> +        else
>>> +            gfn = virt_to_gfn(vaddr[i]);
>>> +
>>> +        grefs[i] = gnttab_claim_grant_reference(&gref_head);
>>
>> gnttab_claim_grant_reference() can return error if no free grant 
>> reference remains.
>
> This can happen only in case gnttab_alloc_grant_references() didn't
> allocate enough grants but told us it succeeded doing so.
>
>> I understand this patch only moves the code, but probably it would be 
>> better to add a missing check here (and likely rollback already 
>> processed grants if any?).
>
> I don't think this is needed, as this would be a clear bug in the code.

I would put WARN_ON_ONCE if ref is an error value then like xen-netfront 
does. Either way, with or without it you can add my:

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>



>
>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko