drm/xen-front: Add support for Xen PV display frontend

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by Oleksandr Andrushchenko <[email protected]>

---
 drivers/gpu/drm/Kconfig             |  2 +
 drivers/gpu/drm/Makefile            |  1 +
 drivers/gpu/drm/xen/Kconfig         | 17 ++++++++
 drivers/gpu/drm/xen/Makefile        |  5 +++
 drivers/gpu/drm/xen/xen_drm_front.c | 83 +++++++++++++++++++++++++++++++++++++
 5 files changed, 108 insertions(+)
 create mode 100644 drivers/gpu/drm/xen/Kconfig
 create mode 100644 drivers/gpu/drm/xen/Makefile
 create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +2 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"
 
 source "drivers/gpu/drm/tve200/Kconfig"
 
+source "drivers/gpu/drm/xen/Kconfig"
+
 # Keep legacy drivers last
 
 menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)	+= mxsfb/
 obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
 obj-$(CONFIG_DRM_PL111) += pl111/
 obj-$(CONFIG_DRM_TVE200) += tve200/
+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+	bool "DRM Support for Xen guest OS"
+	depends on XEN
+	help
+	  Choose this option if you want to enable DRM support
+	  for Xen.
+
+config DRM_XEN_FRONTEND
+	tristate "Para-virtualized frontend driver for Xen guest OS"
+	depends on DRM_XEN
+	depends on DRM
+	select DRM_KMS_HELPER
+	select VIDEOMODE_HELPERS
+	select XEN_XENBUS_FRONTEND
+	help
+	  Choose this option if you want to enable a para-virtualized
+	  frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

drivers/gpu/drm/xen/xen_drm_front.c new file +41 -0

+ *
+ * Copyright (C) 2016-2018 EPAM Systems Inc.
+ *
+ * Author: Oleksandr Andrushchenko <[email protected]>
+ */
+
+#include <drm/drmP.h>
+
+#include <xen/platform_pci.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+
+#include <xen/interface/io/displif.h>
+
+static void backend_on_changed(struct xenbus_device *xb_dev,
+		enum xenbus_state backend_state)
+{
+}
+
+static int xen_drv_probe(struct xenbus_device *xb_dev,
+		const struct xenbus_device_id *id)
+{
+	return 0;
+}
+
+static int xen_drv_remove(struct xenbus_device *dev)
+{
+	return 0;
+}
+
+static const struct xenbus_device_id xen_drv_ids[] = {
+	{ XENDISPL_DRIVER_NAME },
+	{ "" }
+};
+
+static struct xenbus_driver xen_driver = {
+	.ids = xen_drv_ids,
+	.probe = xen_drv_probe,
+	.remove = xen_drv_remove,
+	.otherend_changed = backend_on_changed,
+};

Quoted 82 lines · Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by Oleksandr Andrushchenko <[email protected]>

---
  drivers/gpu/drm/Kconfig             |  2 +
  drivers/gpu/drm/Makefile            |  1 +
  drivers/gpu/drm/xen/Kconfig         | 17 ++++++++
  drivers/gpu/drm/xen/Makefile        |  5 +++
  drivers/gpu/drm/xen/xen_drm_front.c | 83 +++++++++++++++++++++++++++++++++++++
  5 files changed, 108 insertions(+)

create mode 100644 drivers/gpu/drm/xen/Kconfig create mode 100644 drivers/gpu/drm/xen/Makefile create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +2 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"
 
  source "drivers/gpu/drm/tve200/Kconfig"
 
+source "drivers/gpu/drm/xen/Kconfig"
+
  # Keep legacy drivers last
 
  menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)	+= mxsfb/
  obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
  obj-$(CONFIG_DRM_PL111) += pl111/
  obj-$(CONFIG_DRM_TVE200) += tve200/
+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+	bool "DRM Support for Xen guest OS"
+	depends on XEN
+	help
+	  Choose this option if you want to enable DRM support
+	  for Xen.
+
+config DRM_XEN_FRONTEND
+	tristate "Para-virtualized frontend driver for Xen guest OS"
+	depends on DRM_XEN
+	depends on DRM
+	select DRM_KMS_HELPER
+	select VIDEOMODE_HELPERS
+	select XEN_XENBUS_FRONTEND
+	help
+	  Choose this option if you want to enable a para-virtualized
+	  frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

drivers/gpu/drm/xen/xen_drm_front.c line 12

Quoted 10 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +10 -0

+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Use SPDX identifier instead (same applies for all other new sources):

// SPDX-License-Identifier: GPL-2.0

Quoted 45 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +45 -0

+ *
+ * Copyright (C) 2016-2018 EPAM Systems Inc.
+ *
+ * Author: Oleksandr Andrushchenko <[email protected]>
+ */
+
+#include <drm/drmP.h>
+
+#include <xen/platform_pci.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+
+#include <xen/interface/io/displif.h>
+
+static void backend_on_changed(struct xenbus_device *xb_dev,
+		enum xenbus_state backend_state)
+{
+}
+
+static int xen_drv_probe(struct xenbus_device *xb_dev,
+		const struct xenbus_device_id *id)
+{
+	return 0;
+}
+
+static int xen_drv_remove(struct xenbus_device *dev)
+{
+	return 0;
+}
+
+static const struct xenbus_device_id xen_drv_ids[] = {
+	{ XENDISPL_DRIVER_NAME },
+	{ "" }
+};
+
+static struct xenbus_driver xen_driver = {
+	.ids = xen_drv_ids,
+	.probe = xen_drv_probe,
+	.remove = xen_drv_remove,
+	.otherend_changed = backend_on_changed,
+};
+
+static int __init xen_drv_init(void)
+{
+	if (!xen_domain())

drivers/gpu/drm/xen/xen_drm_front.c line 63

Quoted 6 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +6 -0

+		return -ENODEV;
+
+	if (xen_initial_domain()) {
+		DRM_ERROR(XENDISPL_DRIVER_NAME " cannot run in initial domain\n");
+		return -ENODEV;
+	}

Why not? Wouldn't that be possible in case of the backend living in a driver domain?

Juergen

Quoted 98 lines · Juergen Gross

Quoted 86 lines · Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by: Oleksandr Andrushchenko <[email protected]>
---
  drivers/gpu/drm/Kconfig             |  2 + drivers/gpu/drm/Makefile            |  1 + drivers/gpu/drm/xen/Kconfig         | 17 ++++++++ drivers/gpu/drm/xen/Makefile        |  5 +++ drivers/gpu/drm/xen/xen_drm_front.c | 83
+++++++++++++++++++++++++++++++++++++

  5 files changed, 108 insertions(+)

  create mode 100644 drivers/gpu/drm/xen/Kconfig create mode 100644 drivers/gpu/drm/xen/Makefile create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +0 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"

    source "drivers/gpu/drm/tve200/Kconfig"
  +source "drivers/gpu/drm/xen/Kconfig"
+
  # Keep legacy drivers last
    menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +0 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)    += mxsfb/

  obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
  obj-$(CONFIG_DRM_PL111) += pl111/
  obj-$(CONFIG_DRM_TVE200) += tve200/

drivers/gpu/drm/Makefile +1 -0

+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+    bool "DRM Support for Xen guest OS"
+    depends on XEN
+    help
+      Choose this option if you want to enable DRM support
+      for Xen.
+
+config DRM_XEN_FRONTEND
+    tristate "Para-virtualized frontend driver for Xen guest OS"
+    depends on DRM_XEN
+    depends on DRM
+    select DRM_KMS_HELPER
+    select VIDEOMODE_HELPERS
+    select XEN_XENBUS_FRONTEND
+    help
+      Choose this option if you want to enable a para-virtualized
+      frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +4 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device
+ *
+ *   This program is free software; you can redistribute it and/or

modify
+ *   it under the terms of the GNU General Public License as

Quoted 8 lines

published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Use SPDX identifier instead (same applies for all other new sources):

Quoted 1 line

// SPDX-License-Identifier: GPL-2.0

Will update, thank you

Quoted 46 lines

Quoted 46 lines

+ *
+ * Copyright (C) 2016-2018 EPAM Systems Inc.
+ *
+ * Author: Oleksandr Andrushchenko <[email protected]>
+ */

drivers/gpu/drm/xen/xen_drm_front.c new file +41 -0

+
+#include <drm/drmP.h>
+
+#include <xen/platform_pci.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+
+#include <xen/interface/io/displif.h>
+
+static void backend_on_changed(struct xenbus_device *xb_dev,
+        enum xenbus_state backend_state)
+{
+}
+
+static int xen_drv_probe(struct xenbus_device *xb_dev,
+        const struct xenbus_device_id *id)
+{
+    return 0;
+}
+
+static int xen_drv_remove(struct xenbus_device *dev)
+{
+    return 0;
+}
+
+static const struct xenbus_device_id xen_drv_ids[] = {
+    { XENDISPL_DRIVER_NAME },
+    { "" }
+};
+
+static struct xenbus_driver xen_driver = {
+    .ids = xen_drv_ids,
+    .probe = xen_drv_probe,
+    .remove = xen_drv_remove,
+    .otherend_changed = backend_on_changed,
+};
+
+static int __init xen_drv_init(void)
+{
+    if (!xen_domain())
+        return -ENODEV;

Quoted 8 lines

Quoted 6 lines

+
+    if (xen_initial_domain()) {
+        DRM_ERROR(XENDISPL_DRIVER_NAME " cannot run in initial domain\n");
+        return -ENODEV;
+    }

Why not? Wouldn't that be possible in case of the backend living in a driver domain?

It is possible (and in my use-case backend indeed runs in a driver domain). I was just not sure if it is really a good idea to allow that. If you think this is ok, then I'll remove this check

Quoted 155 lines · Oleksandr Andrushchenko

Quoted 98 lines · Juergen Gross

Quoted 86 lines · Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by: Oleksandr Andrushchenko
<[email protected]>

---
   drivers/gpu/drm/Kconfig             |  2 +
   drivers/gpu/drm/Makefile            |  1 +
   drivers/gpu/drm/xen/Kconfig         | 17 ++++++++
   drivers/gpu/drm/xen/Makefile        |  5 +++
   drivers/gpu/drm/xen/xen_drm_front.c | 83
+++++++++++++++++++++++++++++++++++++
   5 files changed, 108 insertions(+)

  create mode 100644 drivers/gpu/drm/xen/Kconfig create mode 100644 drivers/gpu/drm/xen/Makefile create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +1 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"
     source "drivers/gpu/drm/tve200/Kconfig"
   +source "drivers/gpu/drm/xen/Kconfig"
+
   # Keep legacy drivers last
     menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)    += mxsfb/
   obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
   obj-$(CONFIG_DRM_PL111) += pl111/
   obj-$(CONFIG_DRM_TVE200) += tve200/
+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+    bool "DRM Support for Xen guest OS"
+    depends on XEN
+    help
+      Choose this option if you want to enable DRM support
+      for Xen.
+
+config DRM_XEN_FRONTEND
+    tristate "Para-virtualized frontend driver for Xen guest OS"
+    depends on DRM_XEN
+    depends on DRM
+    select DRM_KMS_HELPER
+    select VIDEOMODE_HELPERS
+    select XEN_XENBUS_FRONTEND
+    help
+      Choose this option if you want to enable a para-virtualized
+      frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +4 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device
+ *
+ *   This program is free software; you can redistribute it and/or

modify
+ *   it under the terms of the GNU General Public License as

Quoted 8 lines

published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Use SPDX identifier instead (same applies for all other new sources):

Quoted 1 line

// SPDX-License-Identifier: GPL-2.0

Will update, thank you

Quoted 54 lines

Quoted 49 lines

+ *
+ * Copyright (C) 2016-2018 EPAM Systems Inc.
+ *
+ * Author: Oleksandr Andrushchenko <[email protected]>
+ */

drivers/gpu/drm/xen/xen_drm_front.c new file +44 -0

+
+#include <drm/drmP.h>
+
+#include <xen/platform_pci.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+
+#include <xen/interface/io/displif.h>
+
+static void backend_on_changed(struct xenbus_device *xb_dev,
+        enum xenbus_state backend_state)
+{
+}
+
+static int xen_drv_probe(struct xenbus_device *xb_dev,
+        const struct xenbus_device_id *id)
+{
+    return 0;
+}
+
+static int xen_drv_remove(struct xenbus_device *dev)
+{
+    return 0;
+}
+
+static const struct xenbus_device_id xen_drv_ids[] = {
+    { XENDISPL_DRIVER_NAME },
+    { "" }
+};
+
+static struct xenbus_driver xen_driver = {
+    .ids = xen_drv_ids,
+    .probe = xen_drv_probe,
+    .remove = xen_drv_remove,
+    .otherend_changed = backend_on_changed,
+};
+
+static int __init xen_drv_init(void)
+{
+    if (!xen_domain())
+        return -ENODEV;
+
+    if (xen_initial_domain()) {
+        DRM_ERROR(XENDISPL_DRIVER_NAME " cannot run in initial

Quoted 3 lines

domain\n");
+        return -ENODEV;
+    }

Why not? Wouldn't that be possible in case of the backend living in a driver domain?

Quoted 4 lines

It is possible (and in my use-case backend indeed runs in a driver domain). I was just not sure if it is really a good idea to allow that. If you think this is ok, then I'll remove this check

I don't think the driver should decide that. This would be the job of Xen tools IMO.

Juergen

Quoted 155 lines · Oleksandr Andrushchenko

Quoted 98 lines · Juergen Gross

Quoted 86 lines · Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by: Oleksandr Andrushchenko
<[email protected]>

---
   drivers/gpu/drm/Kconfig             |  2 +
   drivers/gpu/drm/Makefile            |  1 +
   drivers/gpu/drm/xen/Kconfig         | 17 ++++++++
   drivers/gpu/drm/xen/Makefile        |  5 +++
   drivers/gpu/drm/xen/xen_drm_front.c | 83
+++++++++++++++++++++++++++++++++++++
   5 files changed, 108 insertions(+)

  create mode 100644 drivers/gpu/drm/xen/Kconfig create mode 100644 drivers/gpu/drm/xen/Makefile create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +1 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"
     source "drivers/gpu/drm/tve200/Kconfig"
   +source "drivers/gpu/drm/xen/Kconfig"
+
   # Keep legacy drivers last
     menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)    += mxsfb/
   obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
   obj-$(CONFIG_DRM_PL111) += pl111/
   obj-$(CONFIG_DRM_TVE200) += tve200/
+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+    bool "DRM Support for Xen guest OS"
+    depends on XEN
+    help
+      Choose this option if you want to enable DRM support
+      for Xen.
+
+config DRM_XEN_FRONTEND
+    tristate "Para-virtualized frontend driver for Xen guest OS"
+    depends on DRM_XEN
+    depends on DRM
+    select DRM_KMS_HELPER
+    select VIDEOMODE_HELPERS
+    select XEN_XENBUS_FRONTEND
+    help
+      Choose this option if you want to enable a para-virtualized
+      frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +4 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device
+ *
+ *   This program is free software; you can redistribute it and/or

modify
+ *   it under the terms of the GNU General Public License as

Quoted 8 lines

published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Use SPDX identifier instead (same applies for all other new sources):

Quoted 1 line

// SPDX-License-Identifier: GPL-2.0

Will update, thank you

Quoted 54 lines

Quoted 49 lines

+ *
+ * Copyright (C) 2016-2018 EPAM Systems Inc.
+ *
+ * Author: Oleksandr Andrushchenko <[email protected]>
+ */

drivers/gpu/drm/xen/xen_drm_front.c new file +44 -0

+
+#include <drm/drmP.h>
+
+#include <xen/platform_pci.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+
+#include <xen/interface/io/displif.h>
+
+static void backend_on_changed(struct xenbus_device *xb_dev,
+        enum xenbus_state backend_state)
+{
+}
+
+static int xen_drv_probe(struct xenbus_device *xb_dev,
+        const struct xenbus_device_id *id)
+{
+    return 0;
+}
+
+static int xen_drv_remove(struct xenbus_device *dev)
+{
+    return 0;
+}
+
+static const struct xenbus_device_id xen_drv_ids[] = {
+    { XENDISPL_DRIVER_NAME },
+    { "" }
+};
+
+static struct xenbus_driver xen_driver = {
+    .ids = xen_drv_ids,
+    .probe = xen_drv_probe,
+    .remove = xen_drv_remove,
+    .otherend_changed = backend_on_changed,
+};
+
+static int __init xen_drv_init(void)
+{
+    if (!xen_domain())
+        return -ENODEV;
+
+    if (xen_initial_domain()) {
+        DRM_ERROR(XENDISPL_DRIVER_NAME " cannot run in initial

Quoted 3 lines

domain\n");
+        return -ENODEV;
+    }

Why not? Wouldn't that be possible in case of the backend living in a driver domain?

Quoted 4 lines

It is possible (and in my use-case backend indeed runs in a driver domain). I was just not sure if it is really a good idea to allow that. If you think this is ok, then I'll remove this check

I don't think the driver should decide that. This would be the job of Xen tools IMO.

Juergen

Quoted 82 lines · Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by Oleksandr Andrushchenko <[email protected]>

---
  drivers/gpu/drm/Kconfig             |  2 +
  drivers/gpu/drm/Makefile            |  1 +
  drivers/gpu/drm/xen/Kconfig         | 17 ++++++++
  drivers/gpu/drm/xen/Makefile        |  5 +++
  drivers/gpu/drm/xen/xen_drm_front.c | 83 +++++++++++++++++++++++++++++++++++++
  5 files changed, 108 insertions(+)

create mode 100644 drivers/gpu/drm/xen/Kconfig create mode 100644 drivers/gpu/drm/xen/Makefile create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +2 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"
 
  source "drivers/gpu/drm/tve200/Kconfig"
 
+source "drivers/gpu/drm/xen/Kconfig"
+
  # Keep legacy drivers last
 
  menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)	+= mxsfb/
  obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
  obj-$(CONFIG_DRM_PL111) += pl111/
  obj-$(CONFIG_DRM_TVE200) += tve200/
+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+	bool "DRM Support for Xen guest OS"
+	depends on XEN
+	help
+	  Choose this option if you want to enable DRM support
+	  for Xen.
+
+config DRM_XEN_FRONTEND
+	tristate "Para-virtualized frontend driver for Xen guest OS"
+	depends on DRM_XEN
+	depends on DRM
+	select DRM_KMS_HELPER
+	select VIDEOMODE_HELPERS
+	select XEN_XENBUS_FRONTEND
+	help
+	  Choose this option if you want to enable a para-virtualized
+	  frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

drivers/gpu/drm/xen/xen_drm_front.c line 12

Quoted 10 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +10 -0

+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Use SPDX identifier instead (same applies for all other new sources):

// SPDX-License-Identifier: GPL-2.0

Quoted 45 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +45 -0

+ *
+ * Copyright (C) 2016-2018 EPAM Systems Inc.
+ *
+ * Author: Oleksandr Andrushchenko <[email protected]>
+ */
+
+#include <drm/drmP.h>
+
+#include <xen/platform_pci.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+
+#include <xen/interface/io/displif.h>
+
+static void backend_on_changed(struct xenbus_device *xb_dev,
+		enum xenbus_state backend_state)
+{
+}
+
+static int xen_drv_probe(struct xenbus_device *xb_dev,
+		const struct xenbus_device_id *id)
+{
+	return 0;
+}
+
+static int xen_drv_remove(struct xenbus_device *dev)
+{
+	return 0;
+}
+
+static const struct xenbus_device_id xen_drv_ids[] = {
+	{ XENDISPL_DRIVER_NAME },
+	{ "" }
+};
+
+static struct xenbus_driver xen_driver = {
+	.ids = xen_drv_ids,
+	.probe = xen_drv_probe,
+	.remove = xen_drv_remove,
+	.otherend_changed = backend_on_changed,
+};
+
+static int __init xen_drv_init(void)
+{
+	if (!xen_domain())

drivers/gpu/drm/xen/xen_drm_front.c line 63

Quoted 6 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +6 -0

+		return -ENODEV;
+
+	if (xen_initial_domain()) {
+		DRM_ERROR(XENDISPL_DRIVER_NAME " cannot run in initial domain\n");
+		return -ENODEV;
+	}

Why not? Wouldn't that be possible in case of the backend living in a driver domain?

Juergen

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by Oleksandr Andrushchenko <[email protected]>

---
 drivers/gpu/drm/Kconfig             |  2 +
 drivers/gpu/drm/Makefile            |  1 +
 drivers/gpu/drm/xen/Kconfig         | 17 ++++++++
 drivers/gpu/drm/xen/Makefile        |  5 +++
 drivers/gpu/drm/xen/xen_drm_front.c | 83 +++++++++++++++++++++++++++++++++++++
 5 files changed, 108 insertions(+)
 create mode 100644 drivers/gpu/drm/xen/Kconfig
 create mode 100644 drivers/gpu/drm/xen/Makefile
 create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +2 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"
 
 source "drivers/gpu/drm/tve200/Kconfig"
 
+source "drivers/gpu/drm/xen/Kconfig"
+
 # Keep legacy drivers last
 
 menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)	+= mxsfb/
 obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
 obj-$(CONFIG_DRM_PL111) += pl111/
 obj-$(CONFIG_DRM_TVE200) += tve200/
+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+	bool "DRM Support for Xen guest OS"
+	depends on XEN
+	help
+	  Choose this option if you want to enable DRM support
+	  for Xen.
+
+config DRM_XEN_FRONTEND
+	tristate "Para-virtualized frontend driver for Xen guest OS"
+	depends on DRM_XEN
+	depends on DRM
+	select DRM_KMS_HELPER
+	select VIDEOMODE_HELPERS
+	select XEN_XENBUS_FRONTEND
+	help
+	  Choose this option if you want to enable a para-virtualized
+	  frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

Quoted 82 lines · Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by Oleksandr Andrushchenko <[email protected]>

---
  drivers/gpu/drm/Kconfig             |  2 +
  drivers/gpu/drm/Makefile            |  1 +
  drivers/gpu/drm/xen/Kconfig         | 17 ++++++++
  drivers/gpu/drm/xen/Makefile        |  5 +++
  drivers/gpu/drm/xen/xen_drm_front.c | 83 +++++++++++++++++++++++++++++++++++++
  5 files changed, 108 insertions(+)

create mode 100644 drivers/gpu/drm/xen/Kconfig create mode 100644 drivers/gpu/drm/xen/Makefile create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +2 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"
 
  source "drivers/gpu/drm/tve200/Kconfig"
 
+source "drivers/gpu/drm/xen/Kconfig"
+
  # Keep legacy drivers last
 
  menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)	+= mxsfb/
  obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
  obj-$(CONFIG_DRM_PL111) += pl111/
  obj-$(CONFIG_DRM_TVE200) += tve200/
+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+	bool "DRM Support for Xen guest OS"
+	depends on XEN
+	help
+	  Choose this option if you want to enable DRM support
+	  for Xen.
+
+config DRM_XEN_FRONTEND
+	tristate "Para-virtualized frontend driver for Xen guest OS"
+	depends on DRM_XEN
+	depends on DRM
+	select DRM_KMS_HELPER
+	select VIDEOMODE_HELPERS
+	select XEN_XENBUS_FRONTEND
+	help
+	  Choose this option if you want to enable a para-virtualized
+	  frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

drivers/gpu/drm/xen/xen_drm_front.c line 12

Quoted 10 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +10 -0

+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Most Xen drivers in Linux use a dual GPL/BSD license, so that they can be imported into other non GPL OSes:

This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License version 2 as published by the Free Software Foundation; or, when distributed separately from the Linux kernel or incorporated into other software packages, subject to the following license:

Permission is hereby granted, free of charge, to any person obtaining a copy of this source file (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

IMO it would be good to release this driver under the same license, so it can be incorporated into other OSes.

Thanks, Roger.

Quoted 43 lines · Roger Pau Monné

Quoted 5 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

drivers/gpu/drm/xen/xen_drm_front.c line 12

Quoted 10 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +10 -0

+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Most Xen drivers in Linux use a dual GPL/BSD license, so that they can be imported into other non GPL OSes:

This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License version 2 as published by the Free Software Foundation; or, when distributed separately from the Linux kernel or incorporated into other software packages, subject to the following license:

Permission is hereby granted, free of charge, to any person obtaining a copy of this source file (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

Quoted 2 lines

IMO it would be good to release this driver under the same license, so it can be incorporated into other OSes.

I am in any way expert in licensing, but the above seems to be /* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
At least this is what I see at [1] for MIT.
Could you please tell which license(s) as listed at [1] would be appropriate for Xen drivers in terms of how it is expected to appear in the kernel code, e.g. expected SPDX-License-Identifier?

Quoted 46 lines · Oleksandr Andrushchenko

Quoted 43 lines · Roger Pau Monné

Quoted 5 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

drivers/gpu/drm/xen/xen_drm_front.c line 12

Quoted 10 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +10 -0

+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Most Xen drivers in Linux use a dual GPL/BSD license, so that they can be imported into other non GPL OSes:

This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License version 2 as published by the Free Software Foundation; or, when distributed separately from the Linux kernel or incorporated into other software packages, subject to the following license:

Permission is hereby granted, free of charge, to any person obtaining a copy of this source file (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

Quoted 2 lines

IMO it would be good to release this driver under the same license, so it can be incorporated into other OSes.

Quoted 7 lines

I am in any way expert in licensing, but the above seems to be /* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
At least this is what I see at [1] for MIT.
Could you please tell which license(s) as listed at [1] would be appropriate for Xen drivers in terms of how it is expected to appear in the kernel code, e.g. expected SPDX-License-Identifier?

I would be fine with anything MIT/BSD-*/Apache-* like. In the Xen community we have generally done dual GPL-2.0 MIT, so your proposed tag looks fine IMO (I would also personally be fine with MIT/BSD-*/Apache-* only).

The point is that it would be good to have the code under a more permissive license so it can be integrated into non GPL OSes in the future if needed, and that your code could be used as a reference for that.

Thanks, Roger.

Quoted 46 lines · Oleksandr Andrushchenko

Quoted 43 lines · Roger Pau Monné

Quoted 5 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

drivers/gpu/drm/xen/xen_drm_front.c line 12

Quoted 10 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +10 -0

+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Most Xen drivers in Linux use a dual GPL/BSD license, so that they can be imported into other non GPL OSes:

This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License version 2 as published by the Free Software Foundation; or, when distributed separately from the Linux kernel or incorporated into other software packages, subject to the following license:

Permission is hereby granted, free of charge, to any person obtaining a copy of this source file (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

Quoted 2 lines

IMO it would be good to release this driver under the same license, so it can be incorporated into other OSes.

Quoted 7 lines

I am in any way expert in licensing, but the above seems to be /* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
At least this is what I see at [1] for MIT.
Could you please tell which license(s) as listed at [1] would be appropriate for Xen drivers in terms of how it is expected to appear in the kernel code, e.g. expected SPDX-License-Identifier?

I would be fine with anything MIT/BSD-*/Apache-* like. In the Xen community we have generally done dual GPL-2.0 MIT, so your proposed tag looks fine IMO (I would also personally be fine with MIT/BSD-*/Apache-* only).

The point is that it would be good to have the code under a more permissive license so it can be integrated into non GPL OSes in the future if needed, and that your code could be used as a reference for that.

Thanks, Roger.

Quoted 82 lines · Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <[email protected]>

Introduce skeleton of the para-virtualized Xen display frontend driver. This patch only adds required essential stubs.

Signed-off-by Oleksandr Andrushchenko <[email protected]>

---
  drivers/gpu/drm/Kconfig             |  2 +
  drivers/gpu/drm/Makefile            |  1 +
  drivers/gpu/drm/xen/Kconfig         | 17 ++++++++
  drivers/gpu/drm/xen/Makefile        |  5 +++
  drivers/gpu/drm/xen/xen_drm_front.c | 83 +++++++++++++++++++++++++++++++++++++
  5 files changed, 108 insertions(+)

create mode 100644 drivers/gpu/drm/xen/Kconfig create mode 100644 drivers/gpu/drm/xen/Makefile create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c

drivers/gpu/drm/Kconfig +2 -0

index deeefa7a1773..757825ac60df 100644
@@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig"
 
  source "drivers/gpu/drm/tve200/Kconfig"
 
+source "drivers/gpu/drm/xen/Kconfig"
+
  # Keep legacy drivers last
 
  menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 50093ff4479b..9d66657ea117 100644
@@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB)	+= mxsfb/
  obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
  obj-$(CONFIG_DRM_PL111) += pl111/
  obj-$(CONFIG_DRM_TVE200) += tve200/
+obj-$(CONFIG_DRM_XEN) += xen/

drivers/gpu/drm/xen/Kconfig new file +17 -0

new file mode 100644
index 000000000000..4cca160782ab
@@ -0,0 +1,17 @@
+config DRM_XEN
+	bool "DRM Support for Xen guest OS"
+	depends on XEN
+	help
+	  Choose this option if you want to enable DRM support
+	  for Xen.
+
+config DRM_XEN_FRONTEND
+	tristate "Para-virtualized frontend driver for Xen guest OS"
+	depends on DRM_XEN
+	depends on DRM
+	select DRM_KMS_HELPER
+	select VIDEOMODE_HELPERS
+	select XEN_XENBUS_FRONTEND
+	help
+	  Choose this option if you want to enable a para-virtualized
+	  frontend DRM/KMS driver for Xen guest OSes.

drivers/gpu/drm/xen/Makefile new file +5 -0

new file mode 100644
index 000000000000..967074d348f6
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+drm_xen_front-objs := xen_drm_front.o
+
+obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o

drivers/gpu/drm/xen/xen_drm_front.c new file +2 -0

new file mode 100644
index 000000000000..fd372fb464a1
@@ -0,0 +1,83 @@
+/*
+ *  Xen para-virtual DRM device

drivers/gpu/drm/xen/xen_drm_front.c line 12

Quoted 10 lines

drivers/gpu/drm/xen/xen_drm_front.c new file +10 -0

+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.

Most Xen drivers in Linux use a dual GPL/BSD license, so that they can be imported into other non GPL OSes:

This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License version 2 as published by the Free Software Foundation; or, when distributed separately from the Linux kernel or incorporated into other software packages, subject to the following license:

Permission is hereby granted, free of charge, to any person obtaining a copy of this source file (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

IMO it would be good to release this driver under the same license, so it can be incorporated into other OSes.

Thanks, Roger.

drivers/gpu/drm/xen/xen_drm_front.c line 52

Quoted 5 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front.c new file +5 -0

+static struct xenbus_driver xen_driver = {
+	.ids = xen_drv_ids,
+	.probe = xen_drv_probe,
+	.remove = xen_drv_remove,
+	.otherend_changed = backend_on_changed,

What does "_on_" stand for?

-boris

Quoted 7 lines · Boris Ostrovsky

drivers/gpu/drm/xen/xen_drm_front.c line 52

Quoted 5 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front.c new file +5 -0

+static struct xenbus_driver xen_driver = {
+    .ids = xen_drv_ids,
+    .probe = xen_drv_probe,
+    .remove = xen_drv_remove,
+    .otherend_changed = backend_on_changed,

What does "_on_" stand for?

Well, it is somewhat like a hint that this is called "on" event, e.g. event when the other end state has changed, backend in this case. It could be something like "backend_on_state_changed"

Quoted 7 lines · Oleksandr Andrushchenko

Quoted 6 lines · Boris Ostrovsky

Quoted 5 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front.c new file +5 -0

+static struct xenbus_driver xen_driver = {
+    .ids = xen_drv_ids,
+    .probe = xen_drv_probe,
+    .remove = xen_drv_remove,
+    .otherend_changed = backend_on_changed,

Quoted 4 lines

Quoted 1 line

What does "_on_" stand for?

Well, it is somewhat like a hint that this is called "on" event, e.g. event when the other end state has changed, backend in this case. It could be something like "backend_on_state_changed"

If you look at other xenbus_drivers none of the uses this so I think we should stick to conventional naming. (and the same applies to other backend_on_* routines).

-boris

Quoted 7 lines · Oleksandr Andrushchenko

Quoted 6 lines · Boris Ostrovsky

Quoted 5 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front.c new file +5 -0

+static struct xenbus_driver xen_driver = {
+    .ids = xen_drv_ids,
+    .probe = xen_drv_probe,
+    .remove = xen_drv_remove,
+    .otherend_changed = backend_on_changed,

Quoted 4 lines

Quoted 1 line

What does "_on_" stand for?

Well, it is somewhat like a hint that this is called "on" event, e.g. event when the other end state has changed, backend in this case. It could be something like "backend_on_state_changed"

If you look at other xenbus_drivers none of the uses this so I think we should stick to conventional naming. (and the same applies to other backend_on_* routines).

-boris

drivers/gpu/drm/xen/xen_drm_front.c line 52

Quoted 5 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front.c new file +5 -0

+static struct xenbus_driver xen_driver = {
+	.ids = xen_drv_ids,
+	.probe = xen_drv_probe,
+	.remove = xen_drv_remove,
+	.otherend_changed = backend_on_changed,

What does "_on_" stand for?

-boris

From: Oleksandr Andrushchenko <[email protected]>

Initial handling for Xen bus states: implement
Xen bus state machine for the frontend driver according to the state diagram and recovery flow from display para-virtualized
protocol: xen/interface/io/displif.h.

Signed-off-by Oleksandr Andrushchenko <[email protected]>

---
 drivers/gpu/drm/xen/xen_drm_front.c | 124 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/xen/xen_drm_front.h |  26 ++++++++
 2 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/xen/xen_drm_front.h

drivers/gpu/drm/xen/xen_drm_front.c +82 -0

index fd372fb464a1..d0306f9d660d 100644
@@ -24,19 +24,141 @@
 
 #include <xen/interface/io/displif.h>
 
+#include "xen_drm_front.h"
+
+static void xen_drv_remove_internal(struct xen_drm_front_info *front_info)
+{
+}
+
+static int backend_on_initwait(struct xen_drm_front_info *front_info)
+{
+	return 0;
+}
+
+static int backend_on_connected(struct xen_drm_front_info *front_info)
+{
+	return 0;
+}
+
+static void backend_on_disconnected(struct xen_drm_front_info *front_info)
+{
+	xenbus_switch_state(front_info->xb_dev, XenbusStateInitialising);
+}
+
 static void backend_on_changed(struct xenbus_device *xb_dev,
 		enum xenbus_state backend_state)
 {
+	struct xen_drm_front_info *front_info = dev_get_drvdata(&xb_dev->dev);
+	int ret;
+
+	DRM_DEBUG("Backend state is %s, front is %s\n",
+			xenbus_strstate(backend_state),
+			xenbus_strstate(xb_dev->state));
+
+	switch (backend_state) {
+	case XenbusStateReconfiguring:
+		/* fall through */
+	case XenbusStateReconfigured:
+		/* fall through */
+	case XenbusStateInitialised:
+		break;
+
+	case XenbusStateInitialising:
+		/* recovering after backend unexpected closure */
+		backend_on_disconnected(front_info);
+		break;
+
+	case XenbusStateInitWait:
+		/* recovering after backend unexpected closure */
+		backend_on_disconnected(front_info);
+		if (xb_dev->state != XenbusStateInitialising)
+			break;
+
+		ret = backend_on_initwait(front_info);
+		if (ret < 0)
+			xenbus_dev_fatal(xb_dev, ret, "initializing frontend");
+		else
+			xenbus_switch_state(xb_dev, XenbusStateInitialised);
+		break;
+
+	case XenbusStateConnected:
+		if (xb_dev->state != XenbusStateInitialised)
+			break;
+
+		ret = backend_on_connected(front_info);
+		if (ret < 0)
+			xenbus_dev_fatal(xb_dev, ret, "initializing DRM driver");
+		else
+			xenbus_switch_state(xb_dev, XenbusStateConnected);
+		break;
+
+	case XenbusStateClosing:
+		/*
+		 * in this state backend starts freeing resources,
+		 * so let it go into closed state, so we can also
+		 * remove ours
+		 */
+		break;
+
+	case XenbusStateUnknown:
+		/* fall through */
+	case XenbusStateClosed:
+		if (xb_dev->state == XenbusStateClosed)
+			break;
+
+		backend_on_disconnected(front_info);
+		break;
+	}
 }
 

Quoted 109 lines · Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <[email protected]>

Initial handling for Xen bus states: implement
Xen bus state machine for the frontend driver according to the state diagram and recovery flow from display para-virtualized
protocol: xen/interface/io/displif.h.

Signed-off-by Oleksandr Andrushchenko <[email protected]>

---
  drivers/gpu/drm/xen/xen_drm_front.c | 124 +++++++++++++++++++++++++++++++++++-
  drivers/gpu/drm/xen/xen_drm_front.h |  26 ++++++++
  2 files changed, 149 insertions(+), 1 deletion(-)

create mode 100644 drivers/gpu/drm/xen/xen_drm_front.h

drivers/gpu/drm/xen/xen_drm_front.c +82 -0

index fd372fb464a1..d0306f9d660d 100644
@@ -24,19 +24,141 @@
 
  #include <xen/interface/io/displif.h>
 
+#include "xen_drm_front.h"
+
+static void xen_drv_remove_internal(struct xen_drm_front_info *front_info)
+{
+}
+
+static int backend_on_initwait(struct xen_drm_front_info *front_info)
+{
+	return 0;
+}
+
+static int backend_on_connected(struct xen_drm_front_info *front_info)
+{
+	return 0;
+}
+
+static void backend_on_disconnected(struct xen_drm_front_info *front_info)
+{
+	xenbus_switch_state(front_info->xb_dev, XenbusStateInitialising);
+}
+
  static void backend_on_changed(struct xenbus_device *xb_dev,
  		enum xenbus_state backend_state)
  {
+	struct xen_drm_front_info *front_info = dev_get_drvdata(&xb_dev->dev);
+	int ret;
+
+	DRM_DEBUG("Backend state is %s, front is %s\n",
+			xenbus_strstate(backend_state),
+			xenbus_strstate(xb_dev->state));
+
+	switch (backend_state) {
+	case XenbusStateReconfiguring:
+		/* fall through */
+	case XenbusStateReconfigured:
+		/* fall through */
+	case XenbusStateInitialised:
+		break;
+
+	case XenbusStateInitialising:
+		/* recovering after backend unexpected closure */
+		backend_on_disconnected(front_info);
+		break;
+
+	case XenbusStateInitWait:
+		/* recovering after backend unexpected closure */
+		backend_on_disconnected(front_info);
+		if (xb_dev->state != XenbusStateInitialising)
+			break;
+
+		ret = backend_on_initwait(front_info);
+		if (ret < 0)
+			xenbus_dev_fatal(xb_dev, ret, "initializing frontend");
+		else
+			xenbus_switch_state(xb_dev, XenbusStateInitialised);
+		break;
+
+	case XenbusStateConnected:
+		if (xb_dev->state != XenbusStateInitialised)
+			break;
+
+		ret = backend_on_connected(front_info);
+		if (ret < 0)
+			xenbus_dev_fatal(xb_dev, ret, "initializing DRM driver");
+		else
+			xenbus_switch_state(xb_dev, XenbusStateConnected);
+		break;
+
+	case XenbusStateClosing:
+		/*
+		 * in this state backend starts freeing resources,
+		 * so let it go into closed state, so we can also
+		 * remove ours
+		 */
+		break;
+
+	case XenbusStateUnknown:
+		/* fall through */
+	case XenbusStateClosed:
+		if (xb_dev->state == XenbusStateClosed)
+			break;
+
+		backend_on_disconnected(front_info);
+		break;
+	}
  }
 

drivers/gpu/drm/xen/xen_drm_front.c line 122

Quoted 10 lines

drivers/gpu/drm/xen/xen_drm_front.c +6 -1

  static int xen_drv_probe(struct xenbus_device *xb_dev,
  		const struct xenbus_device_id *id)
  {
-	return 0;
+	struct xen_drm_front_info *front_info;
+
+	front_info = devm_kzalloc(&xb_dev->dev,
+			sizeof(*front_info), GFP_KERNEL);
+	if (!front_info) {
+		xenbus_dev_fatal(xb_dev, -ENOMEM, "allocating device memory");

No need for message in case of allocation failure: this is handled in memory allocation already.

Juergen

drivers/gpu/drm/xen/xen_drm_front_cfg.c new file +9 -0

+
+static int cfg_connector(struct xen_drm_front_info *front_info,
+		struct xen_drm_front_cfg_connector *connector,
+		const char *path, int index)
+{
+	char *connector_path;
+
+	connector_path = devm_kasprintf(&front_info->xb_dev->dev,
+			GFP_KERNEL, "%s/%d", path, index);

Quoted 13 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front_cfg.c new file +13 -0

+
+static int cfg_connector(struct xen_drm_front_info *front_info,
+		struct xen_drm_front_cfg_connector *connector,
+		const char *path, int index)
+{
+	char *connector_path;
+
+	connector_path = devm_kasprintf(&front_info->xb_dev->dev,
+			GFP_KERNEL, "%s/%d", path, index);
+	if (!connector_path)
+		return -ENOMEM;
+
+	connector->xenstore_path = connector_path;

drivers/gpu/drm/xen/xen_drm_front_cfg.c line 46

Quoted 6 lines

drivers/gpu/drm/xen/xen_drm_front_cfg.c new file +6 -0

+	if (xenbus_scanf(XBT_NIL, connector_path, XENDISPL_FIELD_RESOLUTION,
+			"%d" XENDISPL_RESOLUTION_SEPARATOR "%d",
+			&connector->width, &connector->height) < 0) {
+		/* either no entry configured or wrong resolution set */
+		connector->width = 0;
+		connector->height = 0;

Do you also need to set connector->xenstore_path to NULL? Or maybe just set it after xenbus_scanf() call.

-boris

Quoted 13 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front_cfg.c new file +13 -0

+
+static int cfg_connector(struct xen_drm_front_info *front_info,
+		struct xen_drm_front_cfg_connector *connector,
+		const char *path, int index)
+{
+	char *connector_path;
+
+	connector_path = devm_kasprintf(&front_info->xb_dev->dev,
+			GFP_KERNEL, "%s/%d", path, index);
+	if (!connector_path)
+		return -ENOMEM;
+
+	connector->xenstore_path = connector_path;

drivers/gpu/drm/xen/xen_drm_front_cfg.c line 46

Quoted 6 lines

drivers/gpu/drm/xen/xen_drm_front_cfg.c new file +6 -0

+	if (xenbus_scanf(XBT_NIL, connector_path, XENDISPL_FIELD_RESOLUTION,
+			"%d" XENDISPL_RESOLUTION_SEPARATOR "%d",
+			&connector->width, &connector->height) < 0) {
+		/* either no entry configured or wrong resolution set */
+		connector->width = 0;
+		connector->height = 0;

Do you also need to set connector->xenstore_path to NULL? Or maybe just set it after xenbus_scanf() call.

-boris

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +27 -0

+
+static void evtchnl_free(struct xen_drm_front_info *front_info,
+		struct xen_drm_front_evtchnl *evtchnl)
+{
+	unsigned long page = 0;
+
+	if (evtchnl->type == EVTCHNL_TYPE_REQ)
+		page = (unsigned long)evtchnl->u.req.ring.sring;
+	else if (evtchnl->type == EVTCHNL_TYPE_EVT)
+		page = (unsigned long)evtchnl->u.evt.page;
+	if (!page)
+		return;
+
+	evtchnl->state = EVTCHNL_STATE_DISCONNECTED;
+
+	if (evtchnl->type == EVTCHNL_TYPE_REQ) {
+		/* release all who still waits for response if any */
+		evtchnl->u.req.resp_status = -EIO;
+		complete_all(&evtchnl->u.req.completion);
+	}
+
+	if (evtchnl->irq)
+		unbind_from_irqhandler(evtchnl->irq, evtchnl);
+
+	if (evtchnl->port)
+		xenbus_free_evtchn(front_info->xb_dev, evtchnl->port);
+

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c line 42

Quoted 13 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +13 -0

+
+static irqreturn_t evtchnl_interrupt_ctrl(int irq, void *dev_id)
+{
+	struct xen_drm_front_evtchnl *evtchnl = dev_id;
+	struct xen_drm_front_info *front_info = evtchnl->front_info;
+	struct xendispl_resp *resp;
+	RING_IDX i, rp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&front_info->io_lock, flags);
+
+	if (unlikely(evtchnl->state != EVTCHNL_STATE_CONNECTED))
+		goto out;

Do you need to check the state under lock? (in other routines too).

...

Quoted 31 lines

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +31 -0

+
+static void evtchnl_free(struct xen_drm_front_info *front_info,
+		struct xen_drm_front_evtchnl *evtchnl)
+{
+	unsigned long page = 0;
+
+	if (evtchnl->type == EVTCHNL_TYPE_REQ)
+		page = (unsigned long)evtchnl->u.req.ring.sring;
+	else if (evtchnl->type == EVTCHNL_TYPE_EVT)
+		page = (unsigned long)evtchnl->u.evt.page;
+	if (!page)
+		return;
+
+	evtchnl->state = EVTCHNL_STATE_DISCONNECTED;
+
+	if (evtchnl->type == EVTCHNL_TYPE_REQ) {
+		/* release all who still waits for response if any */
+		evtchnl->u.req.resp_status = -EIO;
+		complete_all(&evtchnl->u.req.completion);
+	}
+
+	if (evtchnl->irq)
+		unbind_from_irqhandler(evtchnl->irq, evtchnl);
+
+	if (evtchnl->port)
+		xenbus_free_evtchn(front_info->xb_dev, evtchnl->port);
+
+	/* end access and free the page */
+	if (evtchnl->gref != GRANT_INVALID_REF)
+		gnttab_end_foreign_access(evtchnl->gref, 0, page);
+

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c line 166

Quoted 6 lines

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +6 -0

+	if (evtchnl->type == EVTCHNL_TYPE_REQ)
+		evtchnl->u.req.ring.sring = NULL;
+	else
+		evtchnl->u.evt.page = NULL;
+
+	memset(evtchnl, 0, sizeof(*evtchnl));

Since you are zeroing out the structure you don't need to set fields to zero.

I also think you need to free the page.

-boris

Quoted 15 lines · Boris Ostrovsky

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c line 42

Quoted 13 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +13 -0

+
+static irqreturn_t evtchnl_interrupt_ctrl(int irq, void *dev_id)
+{
+    struct xen_drm_front_evtchnl *evtchnl = dev_id;
+    struct xen_drm_front_info *front_info = evtchnl->front_info;
+    struct xendispl_resp *resp;
+    RING_IDX i, rp;
+    unsigned long flags;
+
+    spin_lock_irqsave(&front_info->io_lock, flags);
+
+    if (unlikely(evtchnl->state != EVTCHNL_STATE_CONNECTED))
+        goto out;

Do you need to check the state under lock? (in other routines too).

not really, will move out of the lock in interrupt handlers other places (I assume you refer to be_stream_do_io)

it is set under lock as a part of atomic operation, e.g. we get a new request pointer from the ring and reset completion So, those places still seem to be ok

Quoted 16 lines · Oleksandr Andrushchenko

Quoted 15 lines · Boris Ostrovsky

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c line 42

Quoted 13 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +13 -0

+
+static irqreturn_t evtchnl_interrupt_ctrl(int irq, void *dev_id)
+{
+    struct xen_drm_front_evtchnl *evtchnl = dev_id;
+    struct xen_drm_front_info *front_info = evtchnl->front_info;
+    struct xendispl_resp *resp;
+    RING_IDX i, rp;
+    unsigned long flags;
+
+    spin_lock_irqsave(&front_info->io_lock, flags);
+
+    if (unlikely(evtchnl->state != EVTCHNL_STATE_CONNECTED))
+        goto out;

Do you need to check the state under lock? (in other routines too).

Quoted 2 lines

not really, will move out of the lock in interrupt handlers other places (I assume you refer to be_stream_do_io)

I was mostly referring to evtchnl_interrupt_evt().

-boris

Quoted 3 lines

it is set under lock as a part of atomic operation, e.g. we get a new request pointer from the ring and reset completion So, those places still seem to be ok

Quoted 16 lines · Oleksandr Andrushchenko

Quoted 15 lines · Boris Ostrovsky

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c line 42

Quoted 13 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +13 -0

+
+static irqreturn_t evtchnl_interrupt_ctrl(int irq, void *dev_id)
+{
+    struct xen_drm_front_evtchnl *evtchnl = dev_id;
+    struct xen_drm_front_info *front_info = evtchnl->front_info;
+    struct xendispl_resp *resp;
+    RING_IDX i, rp;
+    unsigned long flags;
+
+    spin_lock_irqsave(&front_info->io_lock, flags);
+
+    if (unlikely(evtchnl->state != EVTCHNL_STATE_CONNECTED))
+        goto out;

Do you need to check the state under lock? (in other routines too).

Quoted 2 lines

not really, will move out of the lock in interrupt handlers other places (I assume you refer to be_stream_do_io)

I was mostly referring to evtchnl_interrupt_evt().

-boris

Quoted 3 lines

it is set under lock as a part of atomic operation, e.g. we get a new request pointer from the ring and reset completion So, those places still seem to be ok

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c line 42

Quoted 13 lines · Oleksandr Andrushchenko

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +13 -0

+
+static irqreturn_t evtchnl_interrupt_ctrl(int irq, void *dev_id)
+{
+	struct xen_drm_front_evtchnl *evtchnl = dev_id;
+	struct xen_drm_front_info *front_info = evtchnl->front_info;
+	struct xendispl_resp *resp;
+	RING_IDX i, rp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&front_info->io_lock, flags);
+
+	if (unlikely(evtchnl->state != EVTCHNL_STATE_CONNECTED))
+		goto out;

Do you need to check the state under lock? (in other routines too).

...

Quoted 31 lines

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +31 -0

+
+static void evtchnl_free(struct xen_drm_front_info *front_info,
+		struct xen_drm_front_evtchnl *evtchnl)
+{
+	unsigned long page = 0;
+
+	if (evtchnl->type == EVTCHNL_TYPE_REQ)
+		page = (unsigned long)evtchnl->u.req.ring.sring;
+	else if (evtchnl->type == EVTCHNL_TYPE_EVT)
+		page = (unsigned long)evtchnl->u.evt.page;
+	if (!page)
+		return;
+
+	evtchnl->state = EVTCHNL_STATE_DISCONNECTED;
+
+	if (evtchnl->type == EVTCHNL_TYPE_REQ) {
+		/* release all who still waits for response if any */
+		evtchnl->u.req.resp_status = -EIO;
+		complete_all(&evtchnl->u.req.completion);
+	}
+
+	if (evtchnl->irq)
+		unbind_from_irqhandler(evtchnl->irq, evtchnl);
+
+	if (evtchnl->port)
+		xenbus_free_evtchn(front_info->xb_dev, evtchnl->port);
+
+	/* end access and free the page */
+	if (evtchnl->gref != GRANT_INVALID_REF)
+		gnttab_end_foreign_access(evtchnl->gref, 0, page);
+

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c line 166

Quoted 6 lines

drivers/gpu/drm/xen/xen_drm_front_evtchnl.c new file +6 -0

+	if (evtchnl->type == EVTCHNL_TYPE_REQ)
+		evtchnl->u.req.ring.sring = NULL;
+	else
+		evtchnl->u.evt.page = NULL;
+
+	memset(evtchnl, 0, sizeof(*evtchnl));

Since you are zeroing out the structure you don't need to set fields to zero.

I also think you need to free the page.

-boris

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +11 -0

+
+static int alloc_storage(struct xen_drm_front_shbuf *buf)
+{
+	if (buf->sgt) {
+		buf->pages = kvmalloc_array(buf->num_pages,
+				sizeof(struct page *), GFP_KERNEL);
+		if (!buf->pages)
+			return -ENOMEM;
+
+		if (drm_prime_sg_to_page_addr_arrays(buf->sgt, buf->pages,
+				NULL, buf->num_pages) < 0)

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +3 -0

+
+	return 0;
+}

drivers/gpu/drm/xen/xen_drm_front.c static struct xenbus_driver xen_driver = { · line 204

Quoted 5 lines · Oleksandr Andrushchenko

  
  static int __init xen_drv_init(void)
  {

drivers/gpu/drm/xen/xen_drm_front.c +2 -0

+	/* At the moment we only support case with XEN_PAGE_SIZE == PAGE_SIZE */
+	BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);

Why BUILD_BUG_ON? This should simply not load if page sizes are different.

drivers/gpu/drm/xen/xen_drm_front_shbuf.c line 187

Quoted 5 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +2 -0

+	ret = gnttab_map_refs(map_ops, NULL, buf->pages, buf->num_pages);
+	BUG_ON(ret);

We should try not to BUG*(). There are a few in this patch (and possibly others) that I think can be avoided.

Quoted 17 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +14 -0

+
+static int alloc_storage(struct xen_drm_front_shbuf *buf)
+{
+	if (buf->sgt) {
+		buf->pages = kvmalloc_array(buf->num_pages,
+				sizeof(struct page *), GFP_KERNEL);
+		if (!buf->pages)
+			return -ENOMEM;
+
+		if (drm_prime_sg_to_page_addr_arrays(buf->sgt, buf->pages,
+				NULL, buf->num_pages) < 0)
+			return -EINVAL;
+	}
+

drivers/gpu/drm/xen/xen_drm_front_shbuf.c line 367

Quoted 7 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +7 -0

+	buf->grefs = kcalloc(buf->num_grefs, sizeof(*buf->grefs), GFP_KERNEL);
+	if (!buf->grefs)
+		return -ENOMEM;
+
+	buf->directory = kcalloc(get_num_pages_dir(buf), PAGE_SIZE, GFP_KERNEL);
+	if (!buf->directory)
+		return -ENOMEM;

You need to clean up on errors.

-boris

drivers/gpu/drm/xen/xen_drm_front_shbuf.c line 70

Quoted 3 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +3 -0

+
+	return 0;
+}

Quoted 11 lines · Boris Ostrovsky

Quoted 5 lines · Oleksandr Andrushchenko

static int __init xen_drv_init(void)
{
+ /* At the moment we only support case with XEN_PAGE_SIZE ==
PAGE_SIZE */
+ BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);

Why BUILD_BUG_ON? This should simply not load if page sizes are different.

This is a compile time check, so if kernel/Xen is configured to use page size combination which is not supported by the driver it will fail during compilation. This seems correct to me, because you shouldn't even try to load the driver which cannot handle different page sizes to not make any harm.

Quoted 6 lines

Quoted 5 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +2 -0

+    ret = gnttab_map_refs(map_ops, NULL, buf->pages, buf->num_pages);
+    BUG_ON(ret);

Quoted 3 lines

We should try not to BUG*(). There are a few in this patch (and possibly others) that I think can be avoided.

I will rework BUG_* for map/unmap code to handle errors, but will still leave

    /* either pages or sgt, not both */
    BUG_ON(cfg->pages && cfg->sgt);

which is a real driver bug and must not happen

Quoted 11 lines · Oleksandr Andrushchenko

Quoted 10 lines · Boris Ostrovsky

Quoted 5 lines · Oleksandr Andrushchenko

static int __init xen_drv_init(void)
{
+ /* At the moment we only support case with XEN_PAGE_SIZE ==
PAGE_SIZE */
+ BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);

Why BUILD_BUG_ON? This should simply not load if page sizes are different.

Quoted 5 lines

This is a compile time check, so if kernel/Xen is configured to use page size combination which is not supported by the driver it will fail during compilation. This seems correct to me, because you shouldn't even try to load the driver which cannot handle different page sizes to not make any harm.

This will prevent whole kernel from building. So, for example, randconfig builds will fail.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +2 -0

+    ret = gnttab_map_refs(map_ops, NULL, buf->pages, buf->num_pages);
+    BUG_ON(ret);

Quoted 8 lines

Quoted 3 lines

We should try not to BUG*(). There are a few in this patch (and possibly others) that I think can be avoided.

I will rework BUG_* for map/unmap code to handle errors, but will still leave

     /* either pages or sgt, not both */
     BUG_ON(cfg->pages && cfg->sgt);

which is a real driver bug and must not happen

Why not return an error?

In fact, AFAICS you only call it in patch 9 where both of these can be tested, in which case something like -EINVAL would look reasonable.

-boris

Quoted 11 lines · Oleksandr Andrushchenko

Quoted 10 lines · Boris Ostrovsky

Quoted 5 lines · Oleksandr Andrushchenko

static int __init xen_drv_init(void)
{
+ /* At the moment we only support case with XEN_PAGE_SIZE ==
PAGE_SIZE */
+ BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);

Why BUILD_BUG_ON? This should simply not load if page sizes are different.

Quoted 5 lines

This is a compile time check, so if kernel/Xen is configured to use page size combination which is not supported by the driver it will fail during compilation. This seems correct to me, because you shouldn't even try to load the driver which cannot handle different page sizes to not make any harm.

This will prevent whole kernel from building. So, for example, randconfig builds will fail.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +2 -0

+    ret = gnttab_map_refs(map_ops, NULL, buf->pages, buf->num_pages);
+    BUG_ON(ret);

Quoted 8 lines

Quoted 3 lines

We should try not to BUG*(). There are a few in this patch (and possibly others) that I think can be avoided.

I will rework BUG_* for map/unmap code to handle errors, but will still leave

     /* either pages or sgt, not both */
     BUG_ON(cfg->pages && cfg->sgt);

which is a real driver bug and must not happen

Why not return an error?

In fact, AFAICS you only call it in patch 9 where both of these can be tested, in which case something like -EINVAL would look reasonable.

-boris

drivers/gpu/drm/xen/xen_drm_front.c static struct xenbus_driver xen_driver = { · line 204

Quoted 5 lines · Oleksandr Andrushchenko

  
  static int __init xen_drv_init(void)
  {

drivers/gpu/drm/xen/xen_drm_front.c +2 -0

+	/* At the moment we only support case with XEN_PAGE_SIZE == PAGE_SIZE */
+	BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);

Why BUILD_BUG_ON? This should simply not load if page sizes are different.

drivers/gpu/drm/xen/xen_drm_front_shbuf.c line 187

Quoted 5 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +2 -0

+	ret = gnttab_map_refs(map_ops, NULL, buf->pages, buf->num_pages);
+	BUG_ON(ret);

We should try not to BUG*(). There are a few in this patch (and possibly others) that I think can be avoided.

Quoted 17 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +14 -0

+
+static int alloc_storage(struct xen_drm_front_shbuf *buf)
+{
+	if (buf->sgt) {
+		buf->pages = kvmalloc_array(buf->num_pages,
+				sizeof(struct page *), GFP_KERNEL);
+		if (!buf->pages)
+			return -ENOMEM;
+
+		if (drm_prime_sg_to_page_addr_arrays(buf->sgt, buf->pages,
+				NULL, buf->num_pages) < 0)
+			return -EINVAL;
+	}
+

drivers/gpu/drm/xen/xen_drm_front_shbuf.c line 367

Quoted 7 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +7 -0

+	buf->grefs = kcalloc(buf->num_grefs, sizeof(*buf->grefs), GFP_KERNEL);
+	if (!buf->grefs)
+		return -ENOMEM;
+
+	buf->directory = kcalloc(get_num_pages_dir(buf), PAGE_SIZE, GFP_KERNEL);
+	if (!buf->directory)
+		return -ENOMEM;

You need to clean up on errors.

-boris

drivers/gpu/drm/xen/xen_drm_front_shbuf.c line 70

Quoted 3 lines

drivers/gpu/drm/xen/xen_drm_front_shbuf.c new file +3 -0

+
+	return 0;
+}