[PATCH v3 00/21] xen: simplify frontend side ring setup

[PATCH v3 00/21] 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 V3:
- new patches 1 and 2, comments addressed

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

Juergen Gross (21):
  xen: update grant_table.h
  xen/grant-table: never put a reserved grant on the free list
  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/grant-table.c                   |  12 +-
 drivers/xen/xen-front-pgdir-shbuf.c         |  18 +--
 drivers/xen/xenbus/xenbus_client.c          |  82 +++++++---
 include/xen/grant_table.h                   |   2 -
 include/xen/interface/grant_table.h         | 161 ++++++++++++--------
 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 --
 18 files changed, 287 insertions(+), 404 deletions(-)

-- 
2.35.3

[PATCH v3 15/21] 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.35.3

Re: [PATCH v3 00/21] xen: simplify frontend side ring setup

[Boris Ostrovsky]

On 5/5/22 4:16 AM, Juergen Gross wrote:
> 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 V3:
> - new patches 1 and 2, comments addressed
>
> Changes in V2:
> - new patch 9 and related changes in patches 10-18


For the patches that I was explicitly copied on:


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Re: [PATCH v3 15/21] xen/tpmfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Jarkko Sakkinen]

On Thu, May 05, 2022 at 10:16:34AM +0200, Juergen Gross wrote:
> Simplify tpmfront's ring creation and removal via xenbus_setup_ring()
> and xenbus_teardown_ring().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Please add to the commit message why these provide an equivalent
functionality.

BR, Jarkko

Re: [PATCH v3 00/21] xen: simplify frontend side ring setup

[Oleksandr]

On 05.05.22 11:16, Juergen Gross wrote:

Hello Juergen.



> 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 V3:
> - new patches 1 and 2, comments addressed
>
> Changes in V2:
> - new patch 9 and related changes in patches 10-18
>
> Juergen Gross (21):
>    xen: update grant_table.h
>    xen/grant-table: never put a reserved grant on the free list
>    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()


For the patches that touch PV display (#07, #16), PV sound (#08, #20) 
and shared buffer framework used by both frontends (#10):

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


Also I didn't see any issues with these frontends while testing on Arm64 
based board.
So, you can also add:

[Arm64 only]
Tested-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


Thanks!


>
>   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/grant-table.c                   |  12 +-
>   drivers/xen/xen-front-pgdir-shbuf.c         |  18 +--
>   drivers/xen/xenbus/xenbus_client.c          |  82 +++++++---
>   include/xen/grant_table.h                   |   2 -
>   include/xen/interface/grant_table.h         | 161 ++++++++++++--------
>   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 --
>   18 files changed, 287 insertions(+), 404 deletions(-)
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH v3 15/21] xen/tpmfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Juergen Gross]

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

On 07.05.22 00:32, Jarkko Sakkinen wrote:
> On Thu, May 05, 2022 at 10:16:34AM +0200, Juergen Gross wrote:
>> Simplify tpmfront's ring creation and removal via xenbus_setup_ring()
>> and xenbus_teardown_ring().
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Please add to the commit message why these provide an equivalent
> functionality.

Would you be fine with:

   Simplify tpmfront's ring creation and removal via xenbus_setup_ring()
   and xenbus_teardown_ring(), which are provided exactly for the use
   pattern as seen in this driver.


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 v3 15/21] xen/tpmfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Jarkko Sakkinen]

On Wed, May 18, 2022 at 11:41:51AM +0200, Juergen Gross wrote:
> On 07.05.22 00:32, Jarkko Sakkinen wrote:
> > On Thu, May 05, 2022 at 10:16:34AM +0200, Juergen Gross wrote:
> > > Simplify tpmfront's ring creation and removal via xenbus_setup_ring()
> > > and xenbus_teardown_ring().
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > Please add to the commit message why these provide an equivalent
> > functionality.
> 
> Would you be fine with:
> 
>   Simplify tpmfront's ring creation and removal via xenbus_setup_ring()
>   and xenbus_teardown_ring(), which are provided exactly for the use
>   pattern as seen in this driver.
> 
> 
> Juergen

Looks fine to me!

BR, Jarkko