[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 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 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