linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer
@ 2022-04-14 19:19 Oleksandr Tyshchenko
  2022-04-14 19:19 ` [RFC PATCH 1/6] xen/grants: support allocating consecutive grants Oleksandr Tyshchenko
                   ` (7 more replies)
  0 siblings, 8 replies; 40+ messages in thread
From: Oleksandr Tyshchenko @ 2022-04-14 19:19 UTC (permalink / raw)
  To: xen-devel, linux-kernel, linux-arm-kernel, virtualization
  Cc: Oleksandr Tyshchenko, Michael S. Tsirkin, Stefano Stabellini,
	Boris Ostrovsky, Juergen Gross, Julien Grall, Bertrand Marquis,
	Wei Chen, Henry Wang, Kaly Xin, Jiamei Xie, Alex Bennée

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Hello all.

The purpose of this RFC patch series is to add support for restricting memory access under Xen using specific
grant table based DMA ops layer. Patch series is based on Juergen Gross’ initial work [1] which implies using
grant references instead of raw guest physical addresses (GPA) for the virtio communications (some kind of
the software IOMMU).

The high level idea is to create new Xen’s grant table based DMA ops layer for the guest Linux whose main
purpose is to provide a special 64-bit DMA address which is formed by using the grant reference (for a page
to be shared with the backend) with offset and setting the highest address bit (this is for the backend to
be able to distinguish grant ref based DMA address from normal GPA). For this to work we need the ability
to allocate contiguous (consecutive) grant references for multi-page allocations. And the backend then needs
to offer VIRTIO_F_ACCESS_PLATFORM and VIRTIO_F_VERSION_1 feature bits (it must support virtio-mmio modern
transport for 64-bit addresses in the virtqueue).

Xen's grant mapping mechanism is the secure and safe solution to share pages between domains which proven
to work and works for years (in the context of traditional Xen PV drivers for example). So far, the foreign
mapping is used for the virtio backend to map and access guest memory. With the foreign mapping, the backend
is able to map arbitrary pages from the guest memory (or even from Dom0 memory). And as the result, the malicious
backend which runs in a non-trusted domain can take advantage of this. Instead, with the grant mapping
the backend is only allowed to map pages which were explicitly granted by the guest before and nothing else. 
According to the discussions in various mainline threads this solution would likely be welcome because it
perfectly fits in the security model Xen provides. 

What is more, the grant table based solution requires zero changes to the Xen hypervisor itself at least
with virtio-mmio and DT (in comparison, for example, with "foreign mapping + virtio-iommu" solution which would
require the whole new complex emulator in hypervisor in addition to new functionality/hypercall to pass IOVA
from the virtio backend running elsewhere to the hypervisor and translate it to the GPA before mapping into
P2M or denying the foreign mapping request if no corresponding IOVA-GPA mapping present in the IOMMU page table
for that particular device). We only need to update toolstack to insert a new "xen,dev-domid" property to
the virtio-mmio device node when creating a guest device-tree (this is an indicator for the guest to use grants
and the ID of Xen domain where the corresponding backend resides, it is used as an argument to the grant mapping
APIs). It worth mentioning that toolstack patch is based on non  upstreamed yet “Virtio support for toolstack
on Arm” series which is on review now [2].

Please note the following:
- Patch series only covers Arm and virtio-mmio (device-tree) for now. To enable the restricted memory access
  feature on Arm the following options should be set:
  CONFIG_XEN_VIRTIO = y
  CONFIG_XEN_HVM_VIRTIO_GRANT = y
- Some callbacks in xen-virtio DMA ops layer (map_sg/unmap_sg, etc) are not implemented yet as they are not
  needed/used in the first prototype

Patch series is rebased on Linux 5.18-rc2 tag and tested on Renesas Salvator-X board + H3 ES3.0 SoC (Arm64)
with standalone userspace (non-Qemu) virtio-mmio based virtio-disk backend running in Driver domain and Linux
guest running on existing virtio-blk driver (frontend). No issues were observed. Guest domain 'reboot/destroy'
use-cases work properly. I have also tested other use-cases such as assigning several virtio block devices
or a mix of virtio and Xen PV block devices to the guest. 

1. Xen changes located at (last patch):
https://github.com/otyshchenko1/xen/commits/libxl_virtio_next
2. Linux changes located at:
https://github.com/otyshchenko1/linux/commits/virtio_grant5
3. virtio-disk changes located at:
https://github.com/otyshchenko1/virtio-disk/commits/virtio_grant

Any feedback/help would be highly appreciated.

[1] https://www.youtube.com/watch?v=IrlEdaIUDPk
[2] https://lore.kernel.org/xen-devel/1649442065-8332-1-git-send-email-olekstysh@gmail.com/

Juergen Gross (2):
  xen/grants: support allocating consecutive grants
  virtio: add option to restrict memory access under Xen

Oleksandr Tyshchenko (4):
  dt-bindings: xen: Add xen,dev-domid property description for
    xen-virtio layer
  virtio: Various updates to xen-virtio DMA ops layer
  arm/xen: Introduce xen_setup_dma_ops()
  arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests

 .../devicetree/bindings/virtio/xen,dev-domid.yaml  |  39 +++
 arch/arm/include/asm/xen/xen-ops.h                 |   1 +
 arch/arm/mm/dma-mapping.c                          |   5 +-
 arch/arm/xen/enlighten.c                           |  11 +
 arch/arm64/include/asm/xen/xen-ops.h               |   1 +
 arch/arm64/mm/dma-mapping.c                        |   5 +-
 arch/x86/mm/init.c                                 |  15 +
 arch/x86/mm/mem_encrypt.c                          |   5 -
 arch/x86/xen/Kconfig                               |   9 +
 drivers/xen/Kconfig                                |  20 ++
 drivers/xen/Makefile                               |   1 +
 drivers/xen/grant-table.c                          | 238 +++++++++++++--
 drivers/xen/xen-virtio.c                           | 335 +++++++++++++++++++++
 include/xen/arm/xen-ops.h                          |  20 ++
 include/xen/grant_table.h                          |   4 +
 include/xen/xen-ops.h                              |  13 +
 16 files changed, 679 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
 create mode 100644 arch/arm/include/asm/xen/xen-ops.h
 create mode 100644 arch/arm64/include/asm/xen/xen-ops.h
 create mode 100644 drivers/xen/xen-virtio.c
 create mode 100644 include/xen/arm/xen-ops.h

-- 
2.7.4


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC PATCH 1/6] xen/grants: support allocating consecutive grants
  2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
@ 2022-04-14 19:19 ` Oleksandr Tyshchenko
  2022-04-14 19:19 ` [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen Oleksandr Tyshchenko
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 40+ messages in thread
From: Oleksandr Tyshchenko @ 2022-04-14 19:19 UTC (permalink / raw)
  To: xen-devel, linux-kernel
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, Julien Grall,
	Oleksandr Tyshchenko

From: Juergen Gross <jgross@suse.com>

For support of virtio via grant mappings in rare cases larger mappings
using consecutive grants are needed. Support those by adding a bitmap
of free grants.

As consecutive grants will be needed only in very rare cases (e.g. when
configuring a virtio device with a multi-page ring), optimize for the
normal case of non-consecutive allocations.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/grant-table.c | 238 +++++++++++++++++++++++++++++++++++++++-------
 include/xen/grant_table.h |   4 +
 2 files changed, 210 insertions(+), 32 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 8ccccac..1b458c0 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -33,6 +33,7 @@
 
 #define pr_fmt(fmt) "xen:" KBUILD_MODNAME ": " fmt
 
+#include <linux/bitmap.h>
 #include <linux/memblock.h>
 #include <linux/sched.h>
 #include <linux/mm.h>
@@ -72,9 +73,32 @@
 
 static grant_ref_t **gnttab_list;
 static unsigned int nr_grant_frames;
+
+/*
+ * Handling of free grants:
+ *
+ * Free grants are in a simple list anchored in gnttab_free_head. They are
+ * linked by grant ref, the last element contains GNTTAB_LIST_END. The number
+ * of free entries is stored in gnttab_free_count.
+ * Additionally there is a bitmap of free entries anchored in
+ * gnttab_free_bitmap. This is being used for simplifying allocation of
+ * multiple consecutive grants, which is needed e.g. for support of virtio.
+ * gnttab_last_free is used to add free entries of new frames at the end of
+ * the free list.
+ * gnttab_free_tail_ptr specifies the variable which references the start
+ * of consecutive free grants ending with gnttab_last_free. This pointer is
+ * updated in a rather defensive way, in order to avoid performance hits in
+ * hot paths.
+ * All those variables are protected by gnttab_list_lock.
+ */
 static int gnttab_free_count;
-static grant_ref_t gnttab_free_head;
+static unsigned int gnttab_size;
+static grant_ref_t gnttab_free_head = GNTTAB_LIST_END;
+static grant_ref_t gnttab_last_free = GNTTAB_LIST_END;
+static grant_ref_t *gnttab_free_tail_ptr;
+static unsigned long *gnttab_free_bitmap;
 static DEFINE_SPINLOCK(gnttab_list_lock);
+
 struct grant_frames xen_auto_xlat_grant_frames;
 static unsigned int xen_gnttab_version;
 module_param_named(version, xen_gnttab_version, uint, 0);
@@ -170,16 +194,111 @@ static int get_free_entries(unsigned count)
 
 	ref = head = gnttab_free_head;
 	gnttab_free_count -= count;
-	while (count-- > 1)
-		head = gnttab_entry(head);
+	while (count--) {
+		bitmap_clear(gnttab_free_bitmap, head, 1);
+		if (gnttab_free_tail_ptr == __gnttab_entry(head))
+			gnttab_free_tail_ptr = &gnttab_free_head;
+		if (count)
+			head = gnttab_entry(head);
+	}
 	gnttab_free_head = gnttab_entry(head);
 	gnttab_entry(head) = GNTTAB_LIST_END;
 
+	if (!gnttab_free_count) {
+		gnttab_last_free = GNTTAB_LIST_END;
+		gnttab_free_tail_ptr = NULL;
+	}
+
 	spin_unlock_irqrestore(&gnttab_list_lock, flags);
 
 	return ref;
 }
 
+static int get_seq_entry_count(void)
+{
+	if (gnttab_last_free == GNTTAB_LIST_END || !gnttab_free_tail_ptr ||
+	    *gnttab_free_tail_ptr == GNTTAB_LIST_END)
+		return 0;
+
+	return gnttab_last_free - *gnttab_free_tail_ptr + 1;
+}
+
+/* Rebuilds the free grant list and tries to find count consecutive entries. */
+static int get_free_seq(unsigned int count)
+{
+	int ret = -ENOSPC;
+	unsigned int from, to;
+	grant_ref_t *last;
+
+	gnttab_free_tail_ptr = &gnttab_free_head;
+	last = &gnttab_free_head;
+
+	for (from = find_first_bit(gnttab_free_bitmap, gnttab_size);
+	     from < gnttab_size;
+	     from = find_next_bit(gnttab_free_bitmap, gnttab_size, to + 1)) {
+		to = find_next_zero_bit(gnttab_free_bitmap, gnttab_size,
+					from + 1);
+		if (ret < 0 && to - from >= count) {
+			ret = from;
+			bitmap_clear(gnttab_free_bitmap, ret, count);
+			from += count;
+			gnttab_free_count -= count;
+			if (from == to)
+				continue;
+		}
+
+		while (from < to) {
+			*last = from;
+			last = __gnttab_entry(from);
+			gnttab_last_free = from;
+			from++;
+		}
+		if (to < gnttab_size)
+			gnttab_free_tail_ptr = __gnttab_entry(to - 1);
+	}
+
+	*last = GNTTAB_LIST_END;
+	if (gnttab_last_free != gnttab_size - 1)
+		gnttab_free_tail_ptr = NULL;
+
+	return ret;
+}
+
+static int get_free_entries_seq(unsigned int count)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&gnttab_list_lock, flags);
+
+	if (gnttab_free_count < count) {
+		ret = gnttab_expand(count - gnttab_free_count);
+		if (ret < 0)
+			goto out;
+	}
+
+	if (get_seq_entry_count() < count) {
+		ret = get_free_seq(count);
+		if (ret >= 0)
+			goto out;
+		ret = gnttab_expand(count - get_seq_entry_count());
+		if (ret < 0)
+			goto out;
+	}
+
+	ret = *gnttab_free_tail_ptr;
+	*gnttab_free_tail_ptr = gnttab_entry(ret + count - 1);
+	gnttab_free_count -= count;
+	if (!gnttab_free_count)
+		gnttab_free_tail_ptr = NULL;
+	bitmap_clear(gnttab_free_bitmap, ret, count);
+
+ out:
+	spin_unlock_irqrestore(&gnttab_list_lock, flags);
+
+	return ret;
+}
+
 static void do_free_callbacks(void)
 {
 	struct gnttab_free_callback *callback, *next;
@@ -206,17 +325,48 @@ static inline void check_free_callbacks(void)
 		do_free_callbacks();
 }
 
-static void put_free_entry(grant_ref_t ref)
+static void put_free_entry_locked(grant_ref_t ref)
 {
-	unsigned long flags;
-	spin_lock_irqsave(&gnttab_list_lock, flags);
 	gnttab_entry(ref) = gnttab_free_head;
 	gnttab_free_head = ref;
+	if (!gnttab_free_count)
+		gnttab_last_free = ref;
+	if (gnttab_free_tail_ptr == &gnttab_free_head)
+		gnttab_free_tail_ptr = __gnttab_entry(ref);
 	gnttab_free_count++;
+	bitmap_set(gnttab_free_bitmap, ref, 1);
+}
+
+static void put_free_entry(grant_ref_t ref)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&gnttab_list_lock, flags);
+	put_free_entry_locked(ref);
 	check_free_callbacks();
 	spin_unlock_irqrestore(&gnttab_list_lock, flags);
 }
 
+static void gnttab_set_free(unsigned int start, unsigned int n)
+{
+	unsigned int i;
+
+	for (i = start; i < start + n - 1; i++)
+		gnttab_entry(i) = i + 1;
+
+	gnttab_entry(i) = GNTTAB_LIST_END;
+	if (!gnttab_free_count) {
+		gnttab_free_head = start;
+		gnttab_free_tail_ptr = &gnttab_free_head;
+	} else {
+		gnttab_entry(gnttab_last_free) = start;
+	}
+	gnttab_free_count += n;
+	gnttab_last_free = i;
+
+	bitmap_set(gnttab_free_bitmap, start, n);
+}
+
 /*
  * Following applies to gnttab_update_entry_v1 and gnttab_update_entry_v2.
  * Introducing a valid entry into the grant table:
@@ -448,23 +598,31 @@ void gnttab_free_grant_references(grant_ref_t head)
 {
 	grant_ref_t ref;
 	unsigned long flags;
-	int count = 1;
-	if (head == GNTTAB_LIST_END)
-		return;
+
 	spin_lock_irqsave(&gnttab_list_lock, flags);
-	ref = head;
-	while (gnttab_entry(ref) != GNTTAB_LIST_END) {
-		ref = gnttab_entry(ref);
-		count++;
+	while (head != GNTTAB_LIST_END) {
+		ref = gnttab_entry(head);
+		put_free_entry_locked(head);
+		head = ref;
 	}
-	gnttab_entry(ref) = gnttab_free_head;
-	gnttab_free_head = head;
-	gnttab_free_count += count;
 	check_free_callbacks();
 	spin_unlock_irqrestore(&gnttab_list_lock, flags);
 }
 EXPORT_SYMBOL_GPL(gnttab_free_grant_references);
 
+void gnttab_free_grant_reference_seq(grant_ref_t head, unsigned int count)
+{
+	unsigned long flags;
+	unsigned int i;
+
+	spin_lock_irqsave(&gnttab_list_lock, flags);
+	for (i = count; i > 0; i--)
+		put_free_entry_locked(head + i - 1);
+	check_free_callbacks();
+	spin_unlock_irqrestore(&gnttab_list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(gnttab_free_grant_reference_seq);
+
 int gnttab_alloc_grant_references(u16 count, grant_ref_t *head)
 {
 	int h = get_free_entries(count);
@@ -478,6 +636,24 @@ int gnttab_alloc_grant_references(u16 count, grant_ref_t *head)
 }
 EXPORT_SYMBOL_GPL(gnttab_alloc_grant_references);
 
+int gnttab_alloc_grant_reference_seq(unsigned int count, grant_ref_t *first)
+{
+	int h;
+
+	if (count == 1)
+		h = get_free_entries(1);
+	else
+		h = get_free_entries_seq(count);
+
+	if (h < 0)
+		return -ENOSPC;
+
+	*first = h;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gnttab_alloc_grant_reference_seq);
+
 int gnttab_empty_grant_references(const grant_ref_t *private_head)
 {
 	return (*private_head == GNTTAB_LIST_END);
@@ -570,16 +746,13 @@ static int grow_gnttab_list(unsigned int more_frames)
 			goto grow_nomem;
 	}
 
+	gnttab_set_free(gnttab_size, extra_entries);
 
-	for (i = grefs_per_frame * nr_grant_frames;
-	     i < grefs_per_frame * new_nr_grant_frames - 1; i++)
-		gnttab_entry(i) = i + 1;
-
-	gnttab_entry(i) = gnttab_free_head;
-	gnttab_free_head = grefs_per_frame * nr_grant_frames;
-	gnttab_free_count += extra_entries;
+	if (!gnttab_free_tail_ptr)
+		gnttab_free_tail_ptr = __gnttab_entry(gnttab_size);
 
 	nr_grant_frames = new_nr_grant_frames;
+	gnttab_size += extra_entries;
 
 	check_free_callbacks();
 
@@ -1424,7 +1597,6 @@ int gnttab_init(void)
 	int i;
 	unsigned long max_nr_grant_frames;
 	unsigned int max_nr_glist_frames, nr_glist_frames;
-	unsigned int nr_init_grefs;
 	int ret;
 
 	gnttab_request_version();
@@ -1452,6 +1624,13 @@ int gnttab_init(void)
 		}
 	}
 
+	i = gnttab_interface->grefs_per_grant_frame * max_nr_grant_frames;
+	gnttab_free_bitmap = bitmap_zalloc(i, GFP_KERNEL);
+	if (!gnttab_free_bitmap) {
+		ret = -ENOMEM;
+		goto ini_nomem;
+	}
+
 	ret = arch_gnttab_init(max_nr_grant_frames,
 			       nr_status_frames(max_nr_grant_frames));
 	if (ret < 0)
@@ -1462,15 +1641,9 @@ int gnttab_init(void)
 		goto ini_nomem;
 	}
 
-	nr_init_grefs = nr_grant_frames *
-			gnttab_interface->grefs_per_grant_frame;
-
-	for (i = NR_RESERVED_ENTRIES; i < nr_init_grefs - 1; i++)
-		gnttab_entry(i) = i + 1;
+	gnttab_size = nr_grant_frames * gnttab_interface->grefs_per_grant_frame;
 
-	gnttab_entry(nr_init_grefs - 1) = GNTTAB_LIST_END;
-	gnttab_free_count = nr_init_grefs - NR_RESERVED_ENTRIES;
-	gnttab_free_head  = NR_RESERVED_ENTRIES;
+	gnttab_set_free(NR_RESERVED_ENTRIES, gnttab_size - NR_RESERVED_ENTRIES);
 
 	printk("Grant table initialized\n");
 	return 0;
@@ -1479,6 +1652,7 @@ int gnttab_init(void)
 	for (i--; i >= 0; i--)
 		free_page((unsigned long)gnttab_list[i]);
 	kfree(gnttab_list);
+	bitmap_free(gnttab_free_bitmap);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(gnttab_init);
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index dfd5bf3..d815e1d 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -129,10 +129,14 @@ int gnttab_try_end_foreign_access(grant_ref_t ref);
  */
 int gnttab_alloc_grant_references(u16 count, grant_ref_t *pprivate_head);
 
+int gnttab_alloc_grant_reference_seq(unsigned int count, grant_ref_t *first);
+
 void gnttab_free_grant_reference(grant_ref_t ref);
 
 void gnttab_free_grant_references(grant_ref_t head);
 
+void gnttab_free_grant_reference_seq(grant_ref_t head, unsigned int count);
+
 int gnttab_empty_grant_references(const grant_ref_t *pprivate_head);
 
 int gnttab_claim_grant_reference(grant_ref_t *pprivate_head);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen
  2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
  2022-04-14 19:19 ` [RFC PATCH 1/6] xen/grants: support allocating consecutive grants Oleksandr Tyshchenko
@ 2022-04-14 19:19 ` Oleksandr Tyshchenko
  2022-04-14 19:43   ` H. Peter Anvin
  2022-04-15 22:01   ` Stefano Stabellini
  2022-04-14 19:19 ` [RFC PATCH 3/6] dt-bindings: xen: Add xen,dev-domid property description for xen-virtio layer Oleksandr Tyshchenko
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 40+ messages in thread
From: Oleksandr Tyshchenko @ 2022-04-14 19:19 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Boris Ostrovsky, Stefano Stabellini, Julien Grall,
	Oleksandr Tyshchenko

From: Juergen Gross <jgross@suse.com>

In order to support virtio in Xen guests add a config option enabling
the user to specify whether in all Xen guests virtio should be able to
access memory via Xen grant mappings only on the host side.

This applies to fully virtualized guests only, as for paravirtualized
guests this is mandatory.

This requires to switch arch_has_restricted_virtio_memory_access()
from a pure stub to a real function on x86 systems (Arm systems are
not covered by now).

Add the needed functionality by providing a special set of DMA ops
handling the needed grant operations for the I/O pages.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/mm/init.c        |  15 ++++
 arch/x86/mm/mem_encrypt.c |   5 --
 arch/x86/xen/Kconfig      |   9 +++
 drivers/xen/Kconfig       |  20 ++++++
 drivers/xen/Makefile      |   1 +
 drivers/xen/xen-virtio.c  | 177 ++++++++++++++++++++++++++++++++++++++++++++++
 include/xen/xen-ops.h     |   8 +++
 7 files changed, 230 insertions(+), 5 deletions(-)
 create mode 100644 drivers/xen/xen-virtio.c

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index d8cfce2..526a3b2 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -8,6 +8,8 @@
 #include <linux/kmemleak.h>
 #include <linux/sched/task.h>
 
+#include <xen/xen.h>
+
 #include <asm/set_memory.h>
 #include <asm/e820/api.h>
 #include <asm/init.h>
@@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
 	return pages;
 }
 #endif
+
+#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
+int arch_has_restricted_virtio_memory_access(void)
+{
+	if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
+		return 1;
+	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
+		return 1;
+
+	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
+}
+EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
+#endif
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 50d2099..dda020f 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
 	print_mem_encrypt_feature_info();
 }
 
-int arch_has_restricted_virtio_memory_access(void)
-{
-	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
-}
-EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
index 85246dd..dffdffd 100644
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -92,3 +92,12 @@ config XEN_DOM0
 	select X86_X2APIC if XEN_PVH && X86_64
 	help
 	  Support running as a Xen Dom0 guest.
+
+config XEN_PV_VIRTIO
+	bool "Xen virtio support for PV guests"
+	depends on XEN_VIRTIO && XEN_PV
+	default y
+	help
+	  Support virtio for running as a paravirtualized guest. This will
+	  need support on the backend side (qemu or kernel, depending on the
+	  virtio device types used).
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 120d32f..fc61f7a 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
 	  having to balloon out RAM regions in order to obtain physical memory
 	  space to create such mappings.
 
+config XEN_VIRTIO
+	bool "Xen virtio support"
+	default n
+	depends on VIRTIO && DMA_OPS
+	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
+	help
+	  Enable virtio support for running as Xen guest. Depending on the
+	  guest type this will require special support on the backend side
+	  (qemu or kernel, depending on the virtio device types used).
+
+config XEN_HVM_VIRTIO_GRANT
+	bool "Require virtio for fully virtualized guests to use grant mappings"
+	depends on XEN_VIRTIO && X86_64
+	default y
+	help
+	  Require virtio for fully virtualized guests to use grant mappings.
+	  This will avoid the need to give the backend the right to map all
+	  of the guest memory. This will need support on the backend side
+	  (qemu or kernel, depending on the virtio device types used).
+
 endmenu
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 5aae66e..767009c 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -39,3 +39,4 @@ xen-gntalloc-y				:= gntalloc.o
 xen-privcmd-y				:= privcmd.o privcmd-buf.o
 obj-$(CONFIG_XEN_FRONT_PGDIR_SHBUF)	+= xen-front-pgdir-shbuf.o
 obj-$(CONFIG_XEN_UNPOPULATED_ALLOC)	+= unpopulated-alloc.o
+obj-$(CONFIG_XEN_VIRTIO)		+= xen-virtio.o
diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
new file mode 100644
index 00000000..cfd5eda
--- /dev/null
+++ b/drivers/xen/xen-virtio.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/******************************************************************************
+ * Xen virtio driver - enables using virtio devices in Xen guests.
+ *
+ * Copyright (c) 2021, Juergen Gross <jgross@suse.com>
+ */
+
+#include <linux/module.h>
+#include <linux/dma-map-ops.h>
+#include <linux/pci.h>
+#include <linux/pfn.h>
+#include <linux/virtio_config.h>
+#include <xen/xen.h>
+#include <xen/grant_table.h>
+
+#define XEN_GRANT_ADDR_OFF	0x8000000000000000ULL
+
+static inline dma_addr_t grant_to_dma(grant_ref_t grant)
+{
+	return XEN_GRANT_ADDR_OFF | ((dma_addr_t)grant << PAGE_SHIFT);
+}
+
+static inline grant_ref_t dma_to_grant(dma_addr_t dma)
+{
+	return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
+}
+
+/*
+ * DMA ops for Xen virtio frontends.
+ *
+ * Used to act as a kind of software IOMMU for Xen guests by using grants as
+ * DMA addresses.
+ * Such a DMA address is formed by using the grant reference as a frame
+ * number and setting the highest address bit (this bit is for the backend
+ * to be able to distinguish it from e.g. a mmio address).
+ *
+ * Note that for now we hard wire dom0 to be the backend domain. In order to
+ * support any domain as backend we'd need to add a way to communicate the
+ * domid of this backend, e.g. via Xenstore or via the PCI-device's config
+ * space.
+ */
+static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
+				  dma_addr_t *dma_handle, gfp_t gfp,
+				  unsigned long attrs)
+{
+	unsigned int n_pages = PFN_UP(size);
+	unsigned int i;
+	unsigned long pfn;
+	grant_ref_t grant;
+	void *ret;
+
+	ret = (void *)__get_free_pages(gfp, get_order(size));
+	if (!ret)
+		return NULL;
+
+	pfn = virt_to_pfn(ret);
+
+	if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
+		free_pages((unsigned long)ret, get_order(size));
+		return NULL;
+	}
+
+	for (i = 0; i < n_pages; i++) {
+		gnttab_grant_foreign_access_ref(grant + i, 0,
+						pfn_to_gfn(pfn + i), 0);
+	}
+
+	*dma_handle = grant_to_dma(grant);
+
+	return ret;
+}
+
+static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
+				dma_addr_t dma_handle, unsigned long attrs)
+{
+	unsigned int n_pages = PFN_UP(size);
+	unsigned int i;
+	grant_ref_t grant;
+
+	grant = dma_to_grant(dma_handle);
+
+	for (i = 0; i < n_pages; i++)
+		gnttab_end_foreign_access_ref(grant + i);
+
+	gnttab_free_grant_reference_seq(grant, n_pages);
+
+	free_pages((unsigned long)vaddr, get_order(size));
+}
+
+static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
+					       dma_addr_t *dma_handle,
+					       enum dma_data_direction dir,
+					       gfp_t gfp)
+{
+	WARN_ONCE(1, "xen_virtio_dma_alloc_pages size %ld\n", size);
+	return NULL;
+}
+
+static void xen_virtio_dma_free_pages(struct device *dev, size_t size,
+				      struct page *vaddr, dma_addr_t dma_handle,
+				      enum dma_data_direction dir)
+{
+	WARN_ONCE(1, "xen_virtio_dma_free_pages size %ld\n", size);
+}
+
+static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
+					  unsigned long offset, size_t size,
+					  enum dma_data_direction dir,
+					  unsigned long attrs)
+{
+	grant_ref_t grant;
+
+	if (gnttab_alloc_grant_references(1, &grant))
+		return 0;
+
+	gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
+					dir == DMA_TO_DEVICE);
+
+	return grant_to_dma(grant) + offset;
+}
+
+static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
+				      size_t size, enum dma_data_direction dir,
+				      unsigned long attrs)
+{
+	grant_ref_t grant;
+
+	grant = dma_to_grant(dma_handle);
+
+	gnttab_end_foreign_access_ref(grant);
+
+	gnttab_free_grant_reference(grant);
+}
+
+static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
+				 int nents, enum dma_data_direction dir,
+				 unsigned long attrs)
+{
+	WARN_ONCE(1, "xen_virtio_dma_map_sg nents %d\n", nents);
+	return -EINVAL;
+}
+
+static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
+				    int nents, enum dma_data_direction dir,
+				    unsigned long attrs)
+{
+	WARN_ONCE(1, "xen_virtio_dma_unmap_sg nents %d\n", nents);
+}
+
+static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
+{
+	return 1;
+}
+
+static const struct dma_map_ops xen_virtio_dma_ops = {
+	.alloc = xen_virtio_dma_alloc,
+	.free = xen_virtio_dma_free,
+	.alloc_pages = xen_virtio_dma_alloc_pages,
+	.free_pages = xen_virtio_dma_free_pages,
+	.mmap = dma_common_mmap,
+	.get_sgtable = dma_common_get_sgtable,
+	.map_page = xen_virtio_dma_map_page,
+	.unmap_page = xen_virtio_dma_unmap_page,
+	.map_sg = xen_virtio_dma_map_sg,
+	.unmap_sg = xen_virtio_dma_unmap_sg,
+	.dma_supported = xen_virtio_dma_dma_supported,
+};
+
+void xen_virtio_setup_dma_ops(struct device *dev)
+{
+	dev->dma_ops = &xen_virtio_dma_ops;
+}
+EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
+
+MODULE_DESCRIPTION("Xen virtio support driver");
+MODULE_AUTHOR("Juergen Gross <jgross@suse.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index a3584a3..ae3c1bc 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -221,4 +221,12 @@ static inline void xen_preemptible_hcall_end(void) { }
 
 #endif /* CONFIG_XEN_PV && !CONFIG_PREEMPTION */
 
+#ifdef CONFIG_XEN_VIRTIO
+void xen_virtio_setup_dma_ops(struct device *dev);
+#else
+static inline void xen_virtio_setup_dma_ops(struct device *dev)
+{
+}
+#endif /* CONFIG_XEN_VIRTIO */
+
 #endif /* INCLUDE_XEN_OPS_H */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [RFC PATCH 3/6] dt-bindings: xen: Add xen,dev-domid property description for xen-virtio layer
  2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
  2022-04-14 19:19 ` [RFC PATCH 1/6] xen/grants: support allocating consecutive grants Oleksandr Tyshchenko
  2022-04-14 19:19 ` [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen Oleksandr Tyshchenko
@ 2022-04-14 19:19 ` Oleksandr Tyshchenko
  2022-04-15 22:01   ` Stefano Stabellini
  2022-04-14 19:19 ` [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer Oleksandr Tyshchenko
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 40+ messages in thread
From: Oleksandr Tyshchenko @ 2022-04-14 19:19 UTC (permalink / raw)
  To: xen-devel, virtualization, devicetree, linux-kernel
  Cc: Oleksandr Tyshchenko, Michael S. Tsirkin, Jason Wang,
	Rob Herring, Krzysztof Kozlowski, Julien Grall, Juergen Gross,
	Stefano Stabellini

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Introduce Xen specific binding for the virtio-mmio device to be used
by Xen virtio support driver in a subsequent commit.

This binding specifies the ID of Xen domain where the corresponding
device (backend) resides. This is needed for the option to restrict
memory access using Xen grant mappings to work.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 .../devicetree/bindings/virtio/xen,dev-domid.yaml  | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml

diff --git a/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml b/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
new file mode 100644
index 00000000..78be993
--- /dev/null
+++ b/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/virtio/xen,dev-domid.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xen specific binding for the virtio device
+
+maintainers:
+  - Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
+
+select: true
+
+description:
+  This binding specifies the ID of Xen domain where the corresponding device
+  (backend) resides. This is needed for the option to restrict memory access
+  using Xen grant mappings to work.
+
+  Note that current and generic "iommus" bindings are mutually exclusive, since
+  the restricted memory access model on Xen behaves as a kind of software IOMMU.
+
+properties:
+  xen,dev-domid:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Should contain the ID of device's domain.
+
+additionalProperties: true
+
+examples:
+  - |
+    virtio_block@3000 {
+            compatible = "virtio,mmio";
+            reg = <0x3000 0x100>;
+            interrupts = <41>;
+
+            /* The device is located in Xen domain with ID 1 */
+            xen,dev-domid = <1>;
+    };
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer
  2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
                   ` (2 preceding siblings ...)
  2022-04-14 19:19 ` [RFC PATCH 3/6] dt-bindings: xen: Add xen,dev-domid property description for xen-virtio layer Oleksandr Tyshchenko
@ 2022-04-14 19:19 ` Oleksandr Tyshchenko
  2022-04-15 22:02   ` Stefano Stabellini
  2022-04-16  6:05   ` Christoph Hellwig
  2022-04-14 19:19 ` [RFC PATCH 5/6] arm/xen: Introduce xen_setup_dma_ops() Oleksandr Tyshchenko
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 40+ messages in thread
From: Oleksandr Tyshchenko @ 2022-04-14 19:19 UTC (permalink / raw)
  To: xen-devel, linux-kernel, linux-arm-kernel
  Cc: Oleksandr Tyshchenko, Stefano Stabellini, Russell King,
	Boris Ostrovsky, Juergen Gross, Julien Grall

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

In the context of current patch do the following:
1. Update code to support virtio-mmio devices
2. Introduce struct xen_virtio_data and account passed virtio devices
   (using list) as we need to store some per-device data
3. Add multi-page support for xen_virtio_dma_map(unmap)_page callbacks
4. Harden code against malicious backend
5. Change to use alloc_pages_exact() instead of __get_free_pages()
6. Introduce locking scheme to protect mappings (I am not 100% sure
   whether per-device lock is really needed)
7. Handle virtio device's DMA mask
8. Retrieve the ID of backend domain from DT for virtio-mmio device
   instead of hardcoding it.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 arch/arm/xen/enlighten.c |  11 +++
 drivers/xen/Kconfig      |   2 +-
 drivers/xen/xen-virtio.c | 200 ++++++++++++++++++++++++++++++++++++++++++-----
 include/xen/xen-ops.h    |   5 ++
 4 files changed, 196 insertions(+), 22 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index ec5b082..870d92f 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -409,6 +409,17 @@ int __init arch_xen_unpopulated_init(struct resource **res)
 }
 #endif
 
+#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
+int arch_has_restricted_virtio_memory_access(void)
+{
+	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
+		return 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
+#endif
+
 static void __init xen_dt_guest_init(void)
 {
 	struct device_node *xen_node;
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index fc61f7a..56afe6a 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -347,7 +347,7 @@ config XEN_VIRTIO
 
 config XEN_HVM_VIRTIO_GRANT
 	bool "Require virtio for fully virtualized guests to use grant mappings"
-	depends on XEN_VIRTIO && X86_64
+	depends on XEN_VIRTIO && (X86_64 || ARM || ARM64)
 	default y
 	help
 	  Require virtio for fully virtualized guests to use grant mappings.
diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
index cfd5eda..c5b2ec9 100644
--- a/drivers/xen/xen-virtio.c
+++ b/drivers/xen/xen-virtio.c
@@ -7,12 +7,26 @@
 
 #include <linux/module.h>
 #include <linux/dma-map-ops.h>
+#include <linux/of.h>
 #include <linux/pci.h>
 #include <linux/pfn.h>
 #include <linux/virtio_config.h>
 #include <xen/xen.h>
 #include <xen/grant_table.h>
 
+struct xen_virtio_data {
+	/* The ID of backend domain */
+	domid_t dev_domid;
+	struct device *dev;
+	struct list_head list;
+	spinlock_t lock;
+	/* Is device behaving sane? */
+	bool broken;
+};
+
+static LIST_HEAD(xen_virtio_devices);
+static DEFINE_SPINLOCK(xen_virtio_lock);
+
 #define XEN_GRANT_ADDR_OFF	0x8000000000000000ULL
 
 static inline dma_addr_t grant_to_dma(grant_ref_t grant)
@@ -25,6 +39,25 @@ static inline grant_ref_t dma_to_grant(dma_addr_t dma)
 	return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
 }
 
+static struct xen_virtio_data *find_xen_virtio_data(struct device *dev)
+{
+	struct xen_virtio_data *data = NULL;
+	bool found = false;
+
+	spin_lock(&xen_virtio_lock);
+
+	list_for_each_entry( data, &xen_virtio_devices, list) {
+		if (data->dev == dev) {
+			found = true;
+			break;
+		}
+	}
+
+	spin_unlock(&xen_virtio_lock);
+
+	return found ? data : NULL;
+}
+
 /*
  * DMA ops for Xen virtio frontends.
  *
@@ -43,48 +76,78 @@ static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
 				  dma_addr_t *dma_handle, gfp_t gfp,
 				  unsigned long attrs)
 {
-	unsigned int n_pages = PFN_UP(size);
-	unsigned int i;
+	struct xen_virtio_data *data;
+	unsigned int i, n_pages = PFN_UP(size);
 	unsigned long pfn;
 	grant_ref_t grant;
-	void *ret;
+	void *ret = NULL;
 
-	ret = (void *)__get_free_pages(gfp, get_order(size));
-	if (!ret)
+	data = find_xen_virtio_data(dev);
+	if (!data)
 		return NULL;
 
+	spin_lock(&data->lock);
+
+	if (unlikely(data->broken))
+		goto out;
+
+	ret = alloc_pages_exact(n_pages * PAGE_SIZE, gfp);
+	if (!ret)
+		goto out;
+
 	pfn = virt_to_pfn(ret);
 
 	if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
-		free_pages((unsigned long)ret, get_order(size));
-		return NULL;
+		free_pages_exact(ret, n_pages * PAGE_SIZE);
+		ret = NULL;
+		goto out;
 	}
 
 	for (i = 0; i < n_pages; i++) {
-		gnttab_grant_foreign_access_ref(grant + i, 0,
+		gnttab_grant_foreign_access_ref(grant + i, data->dev_domid,
 						pfn_to_gfn(pfn + i), 0);
 	}
 
 	*dma_handle = grant_to_dma(grant);
 
+out:
+	spin_unlock(&data->lock);
+
 	return ret;
 }
 
 static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
 				dma_addr_t dma_handle, unsigned long attrs)
 {
-	unsigned int n_pages = PFN_UP(size);
-	unsigned int i;
+	struct xen_virtio_data *data;
+	unsigned int i, n_pages = PFN_UP(size);
 	grant_ref_t grant;
 
+	data = find_xen_virtio_data(dev);
+	if (!data)
+		return;
+
+	spin_lock(&data->lock);
+
+	if (unlikely(data->broken))
+		goto out;
+
 	grant = dma_to_grant(dma_handle);
 
-	for (i = 0; i < n_pages; i++)
-		gnttab_end_foreign_access_ref(grant + i);
+	for (i = 0; i < n_pages; i++) {
+		if (unlikely(!gnttab_end_foreign_access_ref(grant + i))) {
+			dev_alert(dev, "Grant still in use by backend domain, disabled for further use\n");
+			data->broken = true;
+			goto out;
+		}
+	}
 
 	gnttab_free_grant_reference_seq(grant, n_pages);
 
-	free_pages((unsigned long)vaddr, get_order(size));
+	free_pages_exact(vaddr, n_pages * PAGE_SIZE);
+
+out:
+	spin_unlock(&data->lock);
 }
 
 static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
@@ -108,28 +171,71 @@ static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
 					  enum dma_data_direction dir,
 					  unsigned long attrs)
 {
+	struct xen_virtio_data *data;
+	unsigned int i, n_pages = PFN_UP(size);
 	grant_ref_t grant;
+	dma_addr_t dma_handle = DMA_MAPPING_ERROR;
+
+	BUG_ON(dir == DMA_NONE);
+
+	data = find_xen_virtio_data(dev);
+	if (!data)
+		return DMA_MAPPING_ERROR;
+
+	spin_lock(&data->lock);
 
-	if (gnttab_alloc_grant_references(1, &grant))
-		return 0;
+	if (unlikely(data->broken))
+		goto out;
 
-	gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
-					dir == DMA_TO_DEVICE);
+	if (gnttab_alloc_grant_reference_seq(n_pages, &grant))
+		goto out;
 
-	return grant_to_dma(grant) + offset;
+	for (i = 0; i < n_pages; i++) {
+		gnttab_grant_foreign_access_ref(grant + i, data->dev_domid,
+				xen_page_to_gfn(page) + i, dir == DMA_TO_DEVICE);
+	}
+
+	dma_handle = grant_to_dma(grant) + offset;
+
+out:
+	spin_unlock(&data->lock);
+
+	return dma_handle;
 }
 
 static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
 				      size_t size, enum dma_data_direction dir,
 				      unsigned long attrs)
 {
+	struct xen_virtio_data *data;
+	unsigned int i, n_pages = PFN_UP(size);
 	grant_ref_t grant;
 
+	BUG_ON(dir == DMA_NONE);
+
+	data = find_xen_virtio_data(dev);
+	if (!data)
+		return;
+
+	spin_lock(&data->lock);
+
+	if (unlikely(data->broken))
+		goto out;
+
 	grant = dma_to_grant(dma_handle);
 
-	gnttab_end_foreign_access_ref(grant);
+	for (i = 0; i < n_pages; i++) {
+		if (unlikely(!gnttab_end_foreign_access_ref(grant + i))) {
+			dev_alert(dev, "Grant still in use by backend domain, disabled for further use\n");
+			data->broken = true;
+			goto out;
+		}
+	}
+
+	gnttab_free_grant_reference_seq(grant, n_pages);
 
-	gnttab_free_grant_reference(grant);
+out:
+	spin_unlock(&data->lock);
 }
 
 static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
@@ -149,7 +255,7 @@ static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
 
 static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
 {
-	return 1;
+	return mask == DMA_BIT_MASK(64);
 }
 
 static const struct dma_map_ops xen_virtio_dma_ops = {
@@ -166,9 +272,61 @@ static const struct dma_map_ops xen_virtio_dma_ops = {
 	.dma_supported = xen_virtio_dma_dma_supported,
 };
 
+bool xen_is_virtio_device(struct device *dev)
+{
+	/* XXX Handle only DT devices for now */
+	if (!dev->of_node)
+		return false;
+
+	if (!of_device_is_compatible(dev->of_node, "virtio,mmio"))
+		return false;
+
+	return of_property_read_bool(dev->of_node, "xen,dev-domid");
+}
+EXPORT_SYMBOL_GPL(xen_is_virtio_device);
+
 void xen_virtio_setup_dma_ops(struct device *dev)
 {
+	struct xen_virtio_data *data;
+	uint32_t dev_domid;
+
+	data = find_xen_virtio_data(dev);
+	if (data) {
+		dev_err(dev, "xen_virtio data is already created\n");
+		return;
+	}
+
+	if (dev_is_pci(dev)) {
+		/* XXX Leave it hard wired to dom0 for now */
+		dev_domid = 0;
+	} else if (dev->of_node) {
+		if (of_property_read_u32(dev->of_node, "xen,dev-domid", &dev_domid)) {
+			dev_err(dev, "xen,dev-domid property is not present\n");
+			goto err;
+		}
+	} else
+		/* The ACPI case is not supported */
+		goto err;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		dev_err(dev, "Сannot allocate xen_virtio data\n");
+		goto err;
+	}
+	data->dev_domid = dev_domid;
+	data->dev = dev;
+	spin_lock_init(&data->lock);
+
+	spin_lock(&xen_virtio_lock);
+	list_add(&data->list, &xen_virtio_devices);
+	spin_unlock(&xen_virtio_lock);
+
 	dev->dma_ops = &xen_virtio_dma_ops;
+
+	return;
+
+err:
+	dev_err(dev, "Сannot set up xen_virtio DMA ops, retain platform DMA ops\n");
 }
 EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
 
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index ae3c1bc..fdbcb99 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -223,10 +223,15 @@ static inline void xen_preemptible_hcall_end(void) { }
 
 #ifdef CONFIG_XEN_VIRTIO
 void xen_virtio_setup_dma_ops(struct device *dev);
+bool xen_is_virtio_device(struct device *dev);
 #else
 static inline void xen_virtio_setup_dma_ops(struct device *dev)
 {
 }
+static inline bool xen_is_virtio_device(struct device *dev)
+{
+	return false;
+}
 #endif /* CONFIG_XEN_VIRTIO */
 
 #endif /* INCLUDE_XEN_OPS_H */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [RFC PATCH 5/6] arm/xen: Introduce xen_setup_dma_ops()
  2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
                   ` (3 preceding siblings ...)
  2022-04-14 19:19 ` [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer Oleksandr Tyshchenko
@ 2022-04-14 19:19 ` Oleksandr Tyshchenko
  2022-04-15 22:02   ` Stefano Stabellini
  2022-04-14 19:19 ` [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests Oleksandr Tyshchenko
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 40+ messages in thread
From: Oleksandr Tyshchenko @ 2022-04-14 19:19 UTC (permalink / raw)
  To: xen-devel, linux-kernel, linux-arm-kernel
  Cc: Oleksandr Tyshchenko, Stefano Stabellini, Russell King,
	Catalin Marinas, Will Deacon, Boris Ostrovsky, Juergen Gross,
	Logan Gunthorpe, David Hildenbrand, Martin Oliveira, Kees Cook,
	Jean-Philippe Brucker, Julien Grall

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This patch introduces new helper and places it in new header.
The helper's purpose is to assign any Xen specific DMA ops in
a single place. For now, we deal with xen-swiotlb DMA ops only.
The subsequent patch will add xen-virtio DMA ops case.

Also re-use the xen_swiotlb_detect() check on Arm32.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 arch/arm/include/asm/xen/xen-ops.h   |  1 +
 arch/arm/mm/dma-mapping.c            |  5 ++---
 arch/arm64/include/asm/xen/xen-ops.h |  1 +
 arch/arm64/mm/dma-mapping.c          |  5 ++---
 include/xen/arm/xen-ops.h            | 13 +++++++++++++
 5 files changed, 19 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm/include/asm/xen/xen-ops.h
 create mode 100644 arch/arm64/include/asm/xen/xen-ops.h
 create mode 100644 include/xen/arm/xen-ops.h

diff --git a/arch/arm/include/asm/xen/xen-ops.h b/arch/arm/include/asm/xen/xen-ops.h
new file mode 100644
index 00000000..8d2fa24
--- /dev/null
+++ b/arch/arm/include/asm/xen/xen-ops.h
@@ -0,0 +1 @@
+#include <xen/arm/xen-ops.h>
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 82ffac6..a1bf9dd 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -33,7 +33,7 @@
 #include <asm/dma-iommu.h>
 #include <asm/mach/map.h>
 #include <asm/system_info.h>
-#include <xen/swiotlb-xen.h>
+#include <asm/xen/xen-ops.h>
 
 #include "dma.h"
 #include "mm.h"
@@ -2288,8 +2288,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	set_dma_ops(dev, dma_ops);
 
 #ifdef CONFIG_XEN
-	if (xen_initial_domain())
-		dev->dma_ops = &xen_swiotlb_dma_ops;
+	xen_setup_dma_ops(dev);
 #endif
 	dev->archdata.dma_ops_setup = true;
 }
diff --git a/arch/arm64/include/asm/xen/xen-ops.h b/arch/arm64/include/asm/xen/xen-ops.h
new file mode 100644
index 00000000..8d2fa24
--- /dev/null
+++ b/arch/arm64/include/asm/xen/xen-ops.h
@@ -0,0 +1 @@
+#include <xen/arm/xen-ops.h>
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 6719f9e..831e673 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -9,9 +9,9 @@
 #include <linux/dma-map-ops.h>
 #include <linux/dma-iommu.h>
 #include <xen/xen.h>
-#include <xen/swiotlb-xen.h>
 
 #include <asm/cacheflush.h>
+#include <asm/xen/xen-ops.h>
 
 void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
 		enum dma_data_direction dir)
@@ -53,7 +53,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		iommu_setup_dma_ops(dev, dma_base, dma_base + size - 1);
 
 #ifdef CONFIG_XEN
-	if (xen_swiotlb_detect())
-		dev->dma_ops = &xen_swiotlb_dma_ops;
+	xen_setup_dma_ops(dev);
 #endif
 }
diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h
new file mode 100644
index 00000000..621da05
--- /dev/null
+++ b/include/xen/arm/xen-ops.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM_XEN_OPS_H
+#define _ASM_ARM_XEN_OPS_H
+
+#include <xen/swiotlb-xen.h>
+
+static inline void xen_setup_dma_ops(struct device *dev)
+{
+	if (xen_swiotlb_detect())
+		dev->dma_ops = &xen_swiotlb_dma_ops;
+}
+
+#endif /* _ASM_ARM_XEN_OPS_H */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
                   ` (4 preceding siblings ...)
  2022-04-14 19:19 ` [RFC PATCH 5/6] arm/xen: Introduce xen_setup_dma_ops() Oleksandr Tyshchenko
@ 2022-04-14 19:19 ` Oleksandr Tyshchenko
  2022-04-15 22:02   ` Stefano Stabellini
  2022-04-15  7:41 ` [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Christoph Hellwig
  2022-04-15  8:44 ` Michael S. Tsirkin
  7 siblings, 1 reply; 40+ messages in thread
From: Oleksandr Tyshchenko @ 2022-04-14 19:19 UTC (permalink / raw)
  To: xen-devel, linux-kernel, linux-arm-kernel
  Cc: Oleksandr Tyshchenko, Boris Ostrovsky, Juergen Gross,
	Stefano Stabellini, Julien Grall

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Call xen_virtio_setup_dma_ops() only for Xen-aware virtio devices
in Xen guests if restricted access to the guest memory is enabled.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 include/xen/arm/xen-ops.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h
index 621da05..28b2ad3 100644
--- a/include/xen/arm/xen-ops.h
+++ b/include/xen/arm/xen-ops.h
@@ -2,12 +2,19 @@
 #ifndef _ASM_ARM_XEN_OPS_H
 #define _ASM_ARM_XEN_OPS_H
 
+#include <linux/virtio_config.h>
 #include <xen/swiotlb-xen.h>
+#include <xen/xen-ops.h>
 
 static inline void xen_setup_dma_ops(struct device *dev)
 {
 	if (xen_swiotlb_detect())
 		dev->dma_ops = &xen_swiotlb_dma_ops;
+
+#ifdef CONFIG_XEN_VIRTIO
+	if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev))
+		xen_virtio_setup_dma_ops(dev);
+#endif
 }
 
 #endif /* _ASM_ARM_XEN_OPS_H */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen
  2022-04-14 19:19 ` [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen Oleksandr Tyshchenko
@ 2022-04-14 19:43   ` H. Peter Anvin
  2022-04-15 15:20     ` Oleksandr
  2022-04-15 22:01   ` Stefano Stabellini
  1 sibling, 1 reply; 40+ messages in thread
From: H. Peter Anvin @ 2022-04-14 19:43 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Boris Ostrovsky,
	Stefano Stabellini, Julien Grall, Oleksandr Tyshchenko

On April 14, 2022 12:19:29 PM PDT, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote:
>From: Juergen Gross <jgross@suse.com>
>
>In order to support virtio in Xen guests add a config option enabling
>the user to specify whether in all Xen guests virtio should be able to
>access memory via Xen grant mappings only on the host side.
>
>This applies to fully virtualized guests only, as for paravirtualized
>guests this is mandatory.
>
>This requires to switch arch_has_restricted_virtio_memory_access()
>from a pure stub to a real function on x86 systems (Arm systems are
>not covered by now).
>
>Add the needed functionality by providing a special set of DMA ops
>handling the needed grant operations for the I/O pages.
>
>Signed-off-by: Juergen Gross <jgross@suse.com>
>---
> arch/x86/mm/init.c        |  15 ++++
> arch/x86/mm/mem_encrypt.c |   5 --
> arch/x86/xen/Kconfig      |   9 +++
> drivers/xen/Kconfig       |  20 ++++++
> drivers/xen/Makefile      |   1 +
> drivers/xen/xen-virtio.c  | 177 ++++++++++++++++++++++++++++++++++++++++++++++
> include/xen/xen-ops.h     |   8 +++
> 7 files changed, 230 insertions(+), 5 deletions(-)
> create mode 100644 drivers/xen/xen-virtio.c
>
>diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>index d8cfce2..526a3b2 100644
>--- a/arch/x86/mm/init.c
>+++ b/arch/x86/mm/init.c
>@@ -8,6 +8,8 @@
> #include <linux/kmemleak.h>
> #include <linux/sched/task.h>
> 
>+#include <xen/xen.h>
>+
> #include <asm/set_memory.h>
> #include <asm/e820/api.h>
> #include <asm/init.h>
>@@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
> 	return pages;
> }
> #endif
>+
>+#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>+int arch_has_restricted_virtio_memory_access(void)
>+{
>+	if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
>+		return 1;
>+	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>+		return 1;
>+
>+	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>+}
>+EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>+#endif
>diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>index 50d2099..dda020f 100644
>--- a/arch/x86/mm/mem_encrypt.c
>+++ b/arch/x86/mm/mem_encrypt.c
>@@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
> 	print_mem_encrypt_feature_info();
> }
> 
>-int arch_has_restricted_virtio_memory_access(void)
>-{
>-	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>-}
>-EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
>index 85246dd..dffdffd 100644
>--- a/arch/x86/xen/Kconfig
>+++ b/arch/x86/xen/Kconfig
>@@ -92,3 +92,12 @@ config XEN_DOM0
> 	select X86_X2APIC if XEN_PVH && X86_64
> 	help
> 	  Support running as a Xen Dom0 guest.
>+
>+config XEN_PV_VIRTIO
>+	bool "Xen virtio support for PV guests"
>+	depends on XEN_VIRTIO && XEN_PV
>+	default y
>+	help
>+	  Support virtio for running as a paravirtualized guest. This will
>+	  need support on the backend side (qemu or kernel, depending on the
>+	  virtio device types used).
>diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>index 120d32f..fc61f7a 100644
>--- a/drivers/xen/Kconfig
>+++ b/drivers/xen/Kconfig
>@@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
> 	  having to balloon out RAM regions in order to obtain physical memory
> 	  space to create such mappings.
> 
>+config XEN_VIRTIO
>+	bool "Xen virtio support"
>+	default n
>+	depends on VIRTIO && DMA_OPS
>+	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>+	help
>+	  Enable virtio support for running as Xen guest. Depending on the
>+	  guest type this will require special support on the backend side
>+	  (qemu or kernel, depending on the virtio device types used).
>+
>+config XEN_HVM_VIRTIO_GRANT
>+	bool "Require virtio for fully virtualized guests to use grant mappings"
>+	depends on XEN_VIRTIO && X86_64
>+	default y
>+	help
>+	  Require virtio for fully virtualized guests to use grant mappings.
>+	  This will avoid the need to give the backend the right to map all
>+	  of the guest memory. This will need support on the backend side
>+	  (qemu or kernel, depending on the virtio device types used).
>+
> endmenu
>diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
>index 5aae66e..767009c 100644
>--- a/drivers/xen/Makefile
>+++ b/drivers/xen/Makefile
>@@ -39,3 +39,4 @@ xen-gntalloc-y				:= gntalloc.o
> xen-privcmd-y				:= privcmd.o privcmd-buf.o
> obj-$(CONFIG_XEN_FRONT_PGDIR_SHBUF)	+= xen-front-pgdir-shbuf.o
> obj-$(CONFIG_XEN_UNPOPULATED_ALLOC)	+= unpopulated-alloc.o
>+obj-$(CONFIG_XEN_VIRTIO)		+= xen-virtio.o
>diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
>new file mode 100644
>index 00000000..cfd5eda
>--- /dev/null
>+++ b/drivers/xen/xen-virtio.c
>@@ -0,0 +1,177 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/******************************************************************************
>+ * Xen virtio driver - enables using virtio devices in Xen guests.
>+ *
>+ * Copyright (c) 2021, Juergen Gross <jgross@suse.com>
>+ */
>+
>+#include <linux/module.h>
>+#include <linux/dma-map-ops.h>
>+#include <linux/pci.h>
>+#include <linux/pfn.h>
>+#include <linux/virtio_config.h>
>+#include <xen/xen.h>
>+#include <xen/grant_table.h>
>+
>+#define XEN_GRANT_ADDR_OFF	0x8000000000000000ULL
>+
>+static inline dma_addr_t grant_to_dma(grant_ref_t grant)
>+{
>+	return XEN_GRANT_ADDR_OFF | ((dma_addr_t)grant << PAGE_SHIFT);
>+}
>+
>+static inline grant_ref_t dma_to_grant(dma_addr_t dma)
>+{
>+	return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
>+}
>+
>+/*
>+ * DMA ops for Xen virtio frontends.
>+ *
>+ * Used to act as a kind of software IOMMU for Xen guests by using grants as
>+ * DMA addresses.
>+ * Such a DMA address is formed by using the grant reference as a frame
>+ * number and setting the highest address bit (this bit is for the backend
>+ * to be able to distinguish it from e.g. a mmio address).
>+ *
>+ * Note that for now we hard wire dom0 to be the backend domain. In order to
>+ * support any domain as backend we'd need to add a way to communicate the
>+ * domid of this backend, e.g. via Xenstore or via the PCI-device's config
>+ * space.
>+ */
>+static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
>+				  dma_addr_t *dma_handle, gfp_t gfp,
>+				  unsigned long attrs)
>+{
>+	unsigned int n_pages = PFN_UP(size);
>+	unsigned int i;
>+	unsigned long pfn;
>+	grant_ref_t grant;
>+	void *ret;
>+
>+	ret = (void *)__get_free_pages(gfp, get_order(size));
>+	if (!ret)
>+		return NULL;
>+
>+	pfn = virt_to_pfn(ret);
>+
>+	if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
>+		free_pages((unsigned long)ret, get_order(size));
>+		return NULL;
>+	}
>+
>+	for (i = 0; i < n_pages; i++) {
>+		gnttab_grant_foreign_access_ref(grant + i, 0,
>+						pfn_to_gfn(pfn + i), 0);
>+	}
>+
>+	*dma_handle = grant_to_dma(grant);
>+
>+	return ret;
>+}
>+
>+static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
>+				dma_addr_t dma_handle, unsigned long attrs)
>+{
>+	unsigned int n_pages = PFN_UP(size);
>+	unsigned int i;
>+	grant_ref_t grant;
>+
>+	grant = dma_to_grant(dma_handle);
>+
>+	for (i = 0; i < n_pages; i++)
>+		gnttab_end_foreign_access_ref(grant + i);
>+
>+	gnttab_free_grant_reference_seq(grant, n_pages);
>+
>+	free_pages((unsigned long)vaddr, get_order(size));
>+}
>+
>+static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
>+					       dma_addr_t *dma_handle,
>+					       enum dma_data_direction dir,
>+					       gfp_t gfp)
>+{
>+	WARN_ONCE(1, "xen_virtio_dma_alloc_pages size %ld\n", size);
>+	return NULL;
>+}
>+
>+static void xen_virtio_dma_free_pages(struct device *dev, size_t size,
>+				      struct page *vaddr, dma_addr_t dma_handle,
>+				      enum dma_data_direction dir)
>+{
>+	WARN_ONCE(1, "xen_virtio_dma_free_pages size %ld\n", size);
>+}
>+
>+static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
>+					  unsigned long offset, size_t size,
>+					  enum dma_data_direction dir,
>+					  unsigned long attrs)
>+{
>+	grant_ref_t grant;
>+
>+	if (gnttab_alloc_grant_references(1, &grant))
>+		return 0;
>+
>+	gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
>+					dir == DMA_TO_DEVICE);
>+
>+	return grant_to_dma(grant) + offset;
>+}
>+
>+static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>+				      size_t size, enum dma_data_direction dir,
>+				      unsigned long attrs)
>+{
>+	grant_ref_t grant;
>+
>+	grant = dma_to_grant(dma_handle);
>+
>+	gnttab_end_foreign_access_ref(grant);
>+
>+	gnttab_free_grant_reference(grant);
>+}
>+
>+static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
>+				 int nents, enum dma_data_direction dir,
>+				 unsigned long attrs)
>+{
>+	WARN_ONCE(1, "xen_virtio_dma_map_sg nents %d\n", nents);
>+	return -EINVAL;
>+}
>+
>+static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>+				    int nents, enum dma_data_direction dir,
>+				    unsigned long attrs)
>+{
>+	WARN_ONCE(1, "xen_virtio_dma_unmap_sg nents %d\n", nents);
>+}
>+
>+static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
>+{
>+	return 1;
>+}
>+
>+static const struct dma_map_ops xen_virtio_dma_ops = {
>+	.alloc = xen_virtio_dma_alloc,
>+	.free = xen_virtio_dma_free,
>+	.alloc_pages = xen_virtio_dma_alloc_pages,
>+	.free_pages = xen_virtio_dma_free_pages,
>+	.mmap = dma_common_mmap,
>+	.get_sgtable = dma_common_get_sgtable,
>+	.map_page = xen_virtio_dma_map_page,
>+	.unmap_page = xen_virtio_dma_unmap_page,
>+	.map_sg = xen_virtio_dma_map_sg,
>+	.unmap_sg = xen_virtio_dma_unmap_sg,
>+	.dma_supported = xen_virtio_dma_dma_supported,
>+};
>+
>+void xen_virtio_setup_dma_ops(struct device *dev)
>+{
>+	dev->dma_ops = &xen_virtio_dma_ops;
>+}
>+EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
>+
>+MODULE_DESCRIPTION("Xen virtio support driver");
>+MODULE_AUTHOR("Juergen Gross <jgross@suse.com>");
>+MODULE_LICENSE("GPL");
>diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
>index a3584a3..ae3c1bc 100644
>--- a/include/xen/xen-ops.h
>+++ b/include/xen/xen-ops.h
>@@ -221,4 +221,12 @@ static inline void xen_preemptible_hcall_end(void) { }
> 
> #endif /* CONFIG_XEN_PV && !CONFIG_PREEMPTION */
> 
>+#ifdef CONFIG_XEN_VIRTIO
>+void xen_virtio_setup_dma_ops(struct device *dev);
>+#else
>+static inline void xen_virtio_setup_dma_ops(struct device *dev)
>+{
>+}
>+#endif /* CONFIG_XEN_VIRTIO */
>+
> #endif /* INCLUDE_XEN_OPS_H */

Can you please encapsulate the Xen part of the test in some Xen-specific file?

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer
  2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
                   ` (5 preceding siblings ...)
  2022-04-14 19:19 ` [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests Oleksandr Tyshchenko
@ 2022-04-15  7:41 ` Christoph Hellwig
  2022-04-15 10:04   ` Oleksandr
  2022-04-15  8:44 ` Michael S. Tsirkin
  7 siblings, 1 reply; 40+ messages in thread
From: Christoph Hellwig @ 2022-04-15  7:41 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: xen-devel, linux-kernel, linux-arm-kernel, virtualization,
	Oleksandr Tyshchenko, Michael S. Tsirkin, Stefano Stabellini,
	Boris Ostrovsky, Juergen Gross, Julien Grall, Bertrand Marquis,
	Wei Chen, Henry Wang, Kaly Xin, Jiamei Xie, Alex Bennée

I can only see three out of 6 patches on the linux-arm-kernel list,
which makes reviewing this impossible.  Also please Cc me directly
on any series doing crazy things with dma ops.  Thanks!

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer
  2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
                   ` (6 preceding siblings ...)
  2022-04-15  7:41 ` [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Christoph Hellwig
@ 2022-04-15  8:44 ` Michael S. Tsirkin
  2022-04-15 15:29   ` Oleksandr
  7 siblings, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2022-04-15  8:44 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: xen-devel, linux-kernel, linux-arm-kernel, virtualization,
	Oleksandr Tyshchenko, Stefano Stabellini, Boris Ostrovsky,
	Juergen Gross, Julien Grall, Bertrand Marquis, Wei Chen,
	Henry Wang, Kaly Xin, Jiamei Xie, Alex Bennée

On Thu, Apr 14, 2022 at 10:19:27PM +0300, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Hello all.
> 
> The purpose of this RFC patch series is to add support for restricting memory access under Xen using specific
> grant table based DMA ops layer. Patch series is based on Juergen Gross’ initial work [1] which implies using
> grant references instead of raw guest physical addresses (GPA) for the virtio communications (some kind of
> the software IOMMU).
> 
> The high level idea is to create new Xen’s grant table based DMA ops layer for the guest Linux whose main
> purpose is to provide a special 64-bit DMA address which is formed by using the grant reference (for a page
> to be shared with the backend) with offset and setting the highest address bit (this is for the backend to
> be able to distinguish grant ref based DMA address from normal GPA). For this to work we need the ability
> to allocate contiguous (consecutive) grant references for multi-page allocations. And the backend then needs
> to offer VIRTIO_F_ACCESS_PLATFORM and VIRTIO_F_VERSION_1 feature bits (it must support virtio-mmio modern
> transport for 64-bit addresses in the virtqueue).

I'm not enough of a xen expert to review this, and I didn't get
all patches, but I'm very happy to see that approach being
taken. VIRTIO_F_ACCESS_PLATFORM and VIRTIO_F_VERSION_1 are
exactly the way to declare not all of memory is accessible.
Thanks!

> Xen's grant mapping mechanism is the secure and safe solution to share pages between domains which proven
> to work and works for years (in the context of traditional Xen PV drivers for example). So far, the foreign
> mapping is used for the virtio backend to map and access guest memory. With the foreign mapping, the backend
> is able to map arbitrary pages from the guest memory (or even from Dom0 memory). And as the result, the malicious
> backend which runs in a non-trusted domain can take advantage of this. Instead, with the grant mapping
> the backend is only allowed to map pages which were explicitly granted by the guest before and nothing else. 
> According to the discussions in various mainline threads this solution would likely be welcome because it
> perfectly fits in the security model Xen provides. 
> 
> What is more, the grant table based solution requires zero changes to the Xen hypervisor itself at least
> with virtio-mmio and DT (in comparison, for example, with "foreign mapping + virtio-iommu" solution which would
> require the whole new complex emulator in hypervisor in addition to new functionality/hypercall to pass IOVA
> from the virtio backend running elsewhere to the hypervisor and translate it to the GPA before mapping into
> P2M or denying the foreign mapping request if no corresponding IOVA-GPA mapping present in the IOMMU page table
> for that particular device). We only need to update toolstack to insert a new "xen,dev-domid" property to
> the virtio-mmio device node when creating a guest device-tree (this is an indicator for the guest to use grants
> and the ID of Xen domain where the corresponding backend resides, it is used as an argument to the grant mapping
> APIs). It worth mentioning that toolstack patch is based on non  upstreamed yet “Virtio support for toolstack
> on Arm” series which is on review now [2].
> 
> Please note the following:
> - Patch series only covers Arm and virtio-mmio (device-tree) for now. To enable the restricted memory access
>   feature on Arm the following options should be set:
>   CONFIG_XEN_VIRTIO = y
>   CONFIG_XEN_HVM_VIRTIO_GRANT = y
> - Some callbacks in xen-virtio DMA ops layer (map_sg/unmap_sg, etc) are not implemented yet as they are not
>   needed/used in the first prototype
> 
> Patch series is rebased on Linux 5.18-rc2 tag and tested on Renesas Salvator-X board + H3 ES3.0 SoC (Arm64)
> with standalone userspace (non-Qemu) virtio-mmio based virtio-disk backend running in Driver domain and Linux
> guest running on existing virtio-blk driver (frontend). No issues were observed. Guest domain 'reboot/destroy'
> use-cases work properly. I have also tested other use-cases such as assigning several virtio block devices
> or a mix of virtio and Xen PV block devices to the guest. 
> 
> 1. Xen changes located at (last patch):
> https://github.com/otyshchenko1/xen/commits/libxl_virtio_next
> 2. Linux changes located at:
> https://github.com/otyshchenko1/linux/commits/virtio_grant5
> 3. virtio-disk changes located at:
> https://github.com/otyshchenko1/virtio-disk/commits/virtio_grant
> 
> Any feedback/help would be highly appreciated.
> 
> [1] https://www.youtube.com/watch?v=IrlEdaIUDPk
> [2] https://lore.kernel.org/xen-devel/1649442065-8332-1-git-send-email-olekstysh@gmail.com/
> 
> Juergen Gross (2):
>   xen/grants: support allocating consecutive grants
>   virtio: add option to restrict memory access under Xen
> 
> Oleksandr Tyshchenko (4):
>   dt-bindings: xen: Add xen,dev-domid property description for
>     xen-virtio layer
>   virtio: Various updates to xen-virtio DMA ops layer
>   arm/xen: Introduce xen_setup_dma_ops()
>   arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
> 
>  .../devicetree/bindings/virtio/xen,dev-domid.yaml  |  39 +++
>  arch/arm/include/asm/xen/xen-ops.h                 |   1 +
>  arch/arm/mm/dma-mapping.c                          |   5 +-
>  arch/arm/xen/enlighten.c                           |  11 +
>  arch/arm64/include/asm/xen/xen-ops.h               |   1 +
>  arch/arm64/mm/dma-mapping.c                        |   5 +-
>  arch/x86/mm/init.c                                 |  15 +
>  arch/x86/mm/mem_encrypt.c                          |   5 -
>  arch/x86/xen/Kconfig                               |   9 +
>  drivers/xen/Kconfig                                |  20 ++
>  drivers/xen/Makefile                               |   1 +
>  drivers/xen/grant-table.c                          | 238 +++++++++++++--
>  drivers/xen/xen-virtio.c                           | 335 +++++++++++++++++++++
>  include/xen/arm/xen-ops.h                          |  20 ++
>  include/xen/grant_table.h                          |   4 +
>  include/xen/xen-ops.h                              |  13 +
>  16 files changed, 679 insertions(+), 43 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
>  create mode 100644 arch/arm/include/asm/xen/xen-ops.h
>  create mode 100644 arch/arm64/include/asm/xen/xen-ops.h
>  create mode 100644 drivers/xen/xen-virtio.c
>  create mode 100644 include/xen/arm/xen-ops.h
> 
> -- 
> 2.7.4


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer
  2022-04-15  7:41 ` [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Christoph Hellwig
@ 2022-04-15 10:04   ` Oleksandr
  0 siblings, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-15 10:04 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: xen-devel, linux-kernel, linux-arm-kernel, virtualization,
	Oleksandr Tyshchenko, Michael S. Tsirkin, Stefano Stabellini,
	Boris Ostrovsky, Juergen Gross, Julien Grall, Bertrand Marquis,
	Wei Chen, Henry Wang, Kaly Xin, Jiamei Xie, Alex Bennée


On 15.04.22 10:41, Christoph Hellwig wrote:

Hello Christoph

> I can only see three out of 6 patches on the linux-arm-kernel list,
> which makes reviewing this impossible.


Oops, I will add linux-arm-kernel. I blindly followed what 
get_maintainer.pl suggested for each patch plus added manually some Xen 
folks,
but, indeed, the first three patches add the base of this enabling work.


> Also please Cc me directly
> on any series doing crazy things with dma ops.  Thanks!

yes, sure.


-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen
  2022-04-14 19:43   ` H. Peter Anvin
@ 2022-04-15 15:20     ` Oleksandr
  0 siblings, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-15 15:20 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: xen-devel, x86, linux-kernel, Juergen Gross, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Boris Ostrovsky, Stefano Stabellini,
	Julien Grall, Oleksandr Tyshchenko, Christoph Hellwig,
	Michael S. Tsirkin, linux-arm-kernel


On 14.04.22 22:43, H. Peter Anvin wrote:

Hello Peter


> On April 14, 2022 12:19:29 PM PDT, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote:
>> From: Juergen Gross <jgross@suse.com>
>>
>> In order to support virtio in Xen guests add a config option enabling
>> the user to specify whether in all Xen guests virtio should be able to
>> access memory via Xen grant mappings only on the host side.
>>
>> This applies to fully virtualized guests only, as for paravirtualized
>> guests this is mandatory.
>>
>> This requires to switch arch_has_restricted_virtio_memory_access()
> >from a pure stub to a real function on x86 systems (Arm systems are
>> not covered by now).
>>
>> Add the needed functionality by providing a special set of DMA ops
>> handling the needed grant operations for the I/O pages.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> arch/x86/mm/init.c        |  15 ++++
>> arch/x86/mm/mem_encrypt.c |   5 --
>> arch/x86/xen/Kconfig      |   9 +++
>> drivers/xen/Kconfig       |  20 ++++++
>> drivers/xen/Makefile      |   1 +
>> drivers/xen/xen-virtio.c  | 177 ++++++++++++++++++++++++++++++++++++++++++++++
>> include/xen/xen-ops.h     |   8 +++
>> 7 files changed, 230 insertions(+), 5 deletions(-)
>> create mode 100644 drivers/xen/xen-virtio.c
>>
>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>> index d8cfce2..526a3b2 100644
>> --- a/arch/x86/mm/init.c
>> +++ b/arch/x86/mm/init.c
>> @@ -8,6 +8,8 @@
>> #include <linux/kmemleak.h>
>> #include <linux/sched/task.h>
>>
>> +#include <xen/xen.h>
>> +
>> #include <asm/set_memory.h>
>> #include <asm/e820/api.h>
>> #include <asm/init.h>
>> @@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
>> 	return pages;
>> }
>> #endif
>> +
>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>> +int arch_has_restricted_virtio_memory_access(void)
>> +{
>> +	if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
>> +		return 1;
>> +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>> +		return 1;
>> +
>> +	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>> +}
>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>> +#endif
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 50d2099..dda020f 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
>> 	print_mem_encrypt_feature_info();
>> }
>>
>> -int arch_has_restricted_virtio_memory_access(void)
>> -{
>> -	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>> -}
>> -EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
>> index 85246dd..dffdffd 100644
>> --- a/arch/x86/xen/Kconfig
>> +++ b/arch/x86/xen/Kconfig
>> @@ -92,3 +92,12 @@ config XEN_DOM0
>> 	select X86_X2APIC if XEN_PVH && X86_64
>> 	help
>> 	  Support running as a Xen Dom0 guest.
>> +
>> +config XEN_PV_VIRTIO
>> +	bool "Xen virtio support for PV guests"
>> +	depends on XEN_VIRTIO && XEN_PV
>> +	default y
>> +	help
>> +	  Support virtio for running as a paravirtualized guest. This will
>> +	  need support on the backend side (qemu or kernel, depending on the
>> +	  virtio device types used).
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index 120d32f..fc61f7a 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
>> 	  having to balloon out RAM regions in order to obtain physical memory
>> 	  space to create such mappings.
>>
>> +config XEN_VIRTIO
>> +	bool "Xen virtio support"
>> +	default n
>> +	depends on VIRTIO && DMA_OPS
>> +	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>> +	help
>> +	  Enable virtio support for running as Xen guest. Depending on the
>> +	  guest type this will require special support on the backend side
>> +	  (qemu or kernel, depending on the virtio device types used).
>> +
>> +config XEN_HVM_VIRTIO_GRANT
>> +	bool "Require virtio for fully virtualized guests to use grant mappings"
>> +	depends on XEN_VIRTIO && X86_64
>> +	default y
>> +	help
>> +	  Require virtio for fully virtualized guests to use grant mappings.
>> +	  This will avoid the need to give the backend the right to map all
>> +	  of the guest memory. This will need support on the backend side
>> +	  (qemu or kernel, depending on the virtio device types used).
>> +
>> endmenu
>> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
>> index 5aae66e..767009c 100644
>> --- a/drivers/xen/Makefile
>> +++ b/drivers/xen/Makefile
>> @@ -39,3 +39,4 @@ xen-gntalloc-y				:= gntalloc.o
>> xen-privcmd-y				:= privcmd.o privcmd-buf.o
>> obj-$(CONFIG_XEN_FRONT_PGDIR_SHBUF)	+= xen-front-pgdir-shbuf.o
>> obj-$(CONFIG_XEN_UNPOPULATED_ALLOC)	+= unpopulated-alloc.o
>> +obj-$(CONFIG_XEN_VIRTIO)		+= xen-virtio.o
>> diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
>> new file mode 100644
>> index 00000000..cfd5eda
>> --- /dev/null
>> +++ b/drivers/xen/xen-virtio.c
>> @@ -0,0 +1,177 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/******************************************************************************
>> + * Xen virtio driver - enables using virtio devices in Xen guests.
>> + *
>> + * Copyright (c) 2021, Juergen Gross <jgross@suse.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/dma-map-ops.h>
>> +#include <linux/pci.h>
>> +#include <linux/pfn.h>
>> +#include <linux/virtio_config.h>
>> +#include <xen/xen.h>
>> +#include <xen/grant_table.h>
>> +
>> +#define XEN_GRANT_ADDR_OFF	0x8000000000000000ULL
>> +
>> +static inline dma_addr_t grant_to_dma(grant_ref_t grant)
>> +{
>> +	return XEN_GRANT_ADDR_OFF | ((dma_addr_t)grant << PAGE_SHIFT);
>> +}
>> +
>> +static inline grant_ref_t dma_to_grant(dma_addr_t dma)
>> +{
>> +	return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
>> +}
>> +
>> +/*
>> + * DMA ops for Xen virtio frontends.
>> + *
>> + * Used to act as a kind of software IOMMU for Xen guests by using grants as
>> + * DMA addresses.
>> + * Such a DMA address is formed by using the grant reference as a frame
>> + * number and setting the highest address bit (this bit is for the backend
>> + * to be able to distinguish it from e.g. a mmio address).
>> + *
>> + * Note that for now we hard wire dom0 to be the backend domain. In order to
>> + * support any domain as backend we'd need to add a way to communicate the
>> + * domid of this backend, e.g. via Xenstore or via the PCI-device's config
>> + * space.
>> + */
>> +static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
>> +				  dma_addr_t *dma_handle, gfp_t gfp,
>> +				  unsigned long attrs)
>> +{
>> +	unsigned int n_pages = PFN_UP(size);
>> +	unsigned int i;
>> +	unsigned long pfn;
>> +	grant_ref_t grant;
>> +	void *ret;
>> +
>> +	ret = (void *)__get_free_pages(gfp, get_order(size));
>> +	if (!ret)
>> +		return NULL;
>> +
>> +	pfn = virt_to_pfn(ret);
>> +
>> +	if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
>> +		free_pages((unsigned long)ret, get_order(size));
>> +		return NULL;
>> +	}
>> +
>> +	for (i = 0; i < n_pages; i++) {
>> +		gnttab_grant_foreign_access_ref(grant + i, 0,
>> +						pfn_to_gfn(pfn + i), 0);
>> +	}
>> +
>> +	*dma_handle = grant_to_dma(grant);
>> +
>> +	return ret;
>> +}
>> +
>> +static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
>> +				dma_addr_t dma_handle, unsigned long attrs)
>> +{
>> +	unsigned int n_pages = PFN_UP(size);
>> +	unsigned int i;
>> +	grant_ref_t grant;
>> +
>> +	grant = dma_to_grant(dma_handle);
>> +
>> +	for (i = 0; i < n_pages; i++)
>> +		gnttab_end_foreign_access_ref(grant + i);
>> +
>> +	gnttab_free_grant_reference_seq(grant, n_pages);
>> +
>> +	free_pages((unsigned long)vaddr, get_order(size));
>> +}
>> +
>> +static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
>> +					       dma_addr_t *dma_handle,
>> +					       enum dma_data_direction dir,
>> +					       gfp_t gfp)
>> +{
>> +	WARN_ONCE(1, "xen_virtio_dma_alloc_pages size %ld\n", size);
>> +	return NULL;
>> +}
>> +
>> +static void xen_virtio_dma_free_pages(struct device *dev, size_t size,
>> +				      struct page *vaddr, dma_addr_t dma_handle,
>> +				      enum dma_data_direction dir)
>> +{
>> +	WARN_ONCE(1, "xen_virtio_dma_free_pages size %ld\n", size);
>> +}
>> +
>> +static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
>> +					  unsigned long offset, size_t size,
>> +					  enum dma_data_direction dir,
>> +					  unsigned long attrs)
>> +{
>> +	grant_ref_t grant;
>> +
>> +	if (gnttab_alloc_grant_references(1, &grant))
>> +		return 0;
>> +
>> +	gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
>> +					dir == DMA_TO_DEVICE);
>> +
>> +	return grant_to_dma(grant) + offset;
>> +}
>> +
>> +static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>> +				      size_t size, enum dma_data_direction dir,
>> +				      unsigned long attrs)
>> +{
>> +	grant_ref_t grant;
>> +
>> +	grant = dma_to_grant(dma_handle);
>> +
>> +	gnttab_end_foreign_access_ref(grant);
>> +
>> +	gnttab_free_grant_reference(grant);
>> +}
>> +
>> +static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
>> +				 int nents, enum dma_data_direction dir,
>> +				 unsigned long attrs)
>> +{
>> +	WARN_ONCE(1, "xen_virtio_dma_map_sg nents %d\n", nents);
>> +	return -EINVAL;
>> +}
>> +
>> +static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>> +				    int nents, enum dma_data_direction dir,
>> +				    unsigned long attrs)
>> +{
>> +	WARN_ONCE(1, "xen_virtio_dma_unmap_sg nents %d\n", nents);
>> +}
>> +
>> +static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
>> +{
>> +	return 1;
>> +}
>> +
>> +static const struct dma_map_ops xen_virtio_dma_ops = {
>> +	.alloc = xen_virtio_dma_alloc,
>> +	.free = xen_virtio_dma_free,
>> +	.alloc_pages = xen_virtio_dma_alloc_pages,
>> +	.free_pages = xen_virtio_dma_free_pages,
>> +	.mmap = dma_common_mmap,
>> +	.get_sgtable = dma_common_get_sgtable,
>> +	.map_page = xen_virtio_dma_map_page,
>> +	.unmap_page = xen_virtio_dma_unmap_page,
>> +	.map_sg = xen_virtio_dma_map_sg,
>> +	.unmap_sg = xen_virtio_dma_unmap_sg,
>> +	.dma_supported = xen_virtio_dma_dma_supported,
>> +};
>> +
>> +void xen_virtio_setup_dma_ops(struct device *dev)
>> +{
>> +	dev->dma_ops = &xen_virtio_dma_ops;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
>> +
>> +MODULE_DESCRIPTION("Xen virtio support driver");
>> +MODULE_AUTHOR("Juergen Gross <jgross@suse.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
>> index a3584a3..ae3c1bc 100644
>> --- a/include/xen/xen-ops.h
>> +++ b/include/xen/xen-ops.h
>> @@ -221,4 +221,12 @@ static inline void xen_preemptible_hcall_end(void) { }
>>
>> #endif /* CONFIG_XEN_PV && !CONFIG_PREEMPTION */
>>
>> +#ifdef CONFIG_XEN_VIRTIO
>> +void xen_virtio_setup_dma_ops(struct device *dev);
>> +#else
>> +static inline void xen_virtio_setup_dma_ops(struct device *dev)
>> +{
>> +}
>> +#endif /* CONFIG_XEN_VIRTIO */
>> +
>> #endif /* INCLUDE_XEN_OPS_H */
> Can you please encapsulate the Xen part of the test in some Xen-specific file?

I assume your question is about changes to common arch/x86/mm/init.c?


#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
int arch_has_restricted_virtio_memory_access(void)
{
     if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
         return 1;
     if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
         return 1;

     return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
}
EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);


If we are speaking about the whole function, then I am not sure, 
unfortunately. I think, if this was a purely Xen specific function 
(which was used by Xen guests only) I would move it somewhere to 
arch/x86/xen/...   (probably to arch/x86/xen/enlighten.c).

As I understand (please bear in mind I am not too familiar with x86 
code), so far this function was only used by SEV guests, but with 
current changes it is going to be used by both Xen and SEV guests, so it 
should be available even if Xen support is compiled out. Could you 
please suggest a better place to keep this stuff?


If we are speaking about only Xen specific bits in that function, then 
definitely yes, for example, in this way:

1. arch/x86/mm/init.c or other common (non-Xen specific) location:

#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
int arch_has_restricted_virtio_memory_access(void)
{
     return (xen_has_restricted_virtio_memory_access() ||
             cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
}
EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
#endif

2.  include/xen/xen.h or other Xen specific location:

static inline int xen_has_restricted_virtio_memory_access(void)
{
     if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
         return 1;
     if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
         return 1;

     return 0;
}

Or I misunderstood your question?

-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer
  2022-04-15  8:44 ` Michael S. Tsirkin
@ 2022-04-15 15:29   ` Oleksandr
  0 siblings, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-15 15:29 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: xen-devel, linux-kernel, linux-arm-kernel, virtualization,
	Oleksandr Tyshchenko, Stefano Stabellini, Boris Ostrovsky,
	Juergen Gross, Julien Grall, Bertrand Marquis, Wei Chen,
	Henry Wang, Kaly Xin, Jiamei Xie, Alex Bennée,
	Christoph Hellwig


On 15.04.22 11:44, Michael S. Tsirkin wrote:


Hello Michael



> On Thu, Apr 14, 2022 at 10:19:27PM +0300, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Hello all.
>>
>> The purpose of this RFC patch series is to add support for restricting memory access under Xen using specific
>> grant table based DMA ops layer. Patch series is based on Juergen Gross’ initial work [1] which implies using
>> grant references instead of raw guest physical addresses (GPA) for the virtio communications (some kind of
>> the software IOMMU).
>>
>> The high level idea is to create new Xen’s grant table based DMA ops layer for the guest Linux whose main
>> purpose is to provide a special 64-bit DMA address which is formed by using the grant reference (for a page
>> to be shared with the backend) with offset and setting the highest address bit (this is for the backend to
>> be able to distinguish grant ref based DMA address from normal GPA). For this to work we need the ability
>> to allocate contiguous (consecutive) grant references for multi-page allocations. And the backend then needs
>> to offer VIRTIO_F_ACCESS_PLATFORM and VIRTIO_F_VERSION_1 feature bits (it must support virtio-mmio modern
>> transport for 64-bit addresses in the virtqueue).
> I'm not enough of a xen expert to review this, and I didn't get
> all patches, but I'm very happy to see that approach being
> taken. VIRTIO_F_ACCESS_PLATFORM and VIRTIO_F_VERSION_1 are
> exactly the way to declare not all of memory is accessible.
> Thanks!

I am happy to hear that! Thank you.


Regarding the "all patches" I have already redirect missing ones, I hope 
you and Christoph will get them.

Sorry for the inconvenience.


>
>> Xen's grant mapping mechanism is the secure and safe solution to share pages between domains which proven
>> to work and works for years (in the context of traditional Xen PV drivers for example). So far, the foreign
>> mapping is used for the virtio backend to map and access guest memory. With the foreign mapping, the backend
>> is able to map arbitrary pages from the guest memory (or even from Dom0 memory). And as the result, the malicious
>> backend which runs in a non-trusted domain can take advantage of this. Instead, with the grant mapping
>> the backend is only allowed to map pages which were explicitly granted by the guest before and nothing else.
>> According to the discussions in various mainline threads this solution would likely be welcome because it
>> perfectly fits in the security model Xen provides.
>>
>> What is more, the grant table based solution requires zero changes to the Xen hypervisor itself at least
>> with virtio-mmio and DT (in comparison, for example, with "foreign mapping + virtio-iommu" solution which would
>> require the whole new complex emulator in hypervisor in addition to new functionality/hypercall to pass IOVA
>> from the virtio backend running elsewhere to the hypervisor and translate it to the GPA before mapping into
>> P2M or denying the foreign mapping request if no corresponding IOVA-GPA mapping present in the IOMMU page table
>> for that particular device). We only need to update toolstack to insert a new "xen,dev-domid" property to
>> the virtio-mmio device node when creating a guest device-tree (this is an indicator for the guest to use grants
>> and the ID of Xen domain where the corresponding backend resides, it is used as an argument to the grant mapping
>> APIs). It worth mentioning that toolstack patch is based on non  upstreamed yet “Virtio support for toolstack
>> on Arm” series which is on review now [2].
>>
>> Please note the following:
>> - Patch series only covers Arm and virtio-mmio (device-tree) for now. To enable the restricted memory access
>>    feature on Arm the following options should be set:
>>    CONFIG_XEN_VIRTIO = y
>>    CONFIG_XEN_HVM_VIRTIO_GRANT = y
>> - Some callbacks in xen-virtio DMA ops layer (map_sg/unmap_sg, etc) are not implemented yet as they are not
>>    needed/used in the first prototype
>>
>> Patch series is rebased on Linux 5.18-rc2 tag and tested on Renesas Salvator-X board + H3 ES3.0 SoC (Arm64)
>> with standalone userspace (non-Qemu) virtio-mmio based virtio-disk backend running in Driver domain and Linux
>> guest running on existing virtio-blk driver (frontend). No issues were observed. Guest domain 'reboot/destroy'
>> use-cases work properly. I have also tested other use-cases such as assigning several virtio block devices
>> or a mix of virtio and Xen PV block devices to the guest.
>>
>> 1. Xen changes located at (last patch):
>> https://github.com/otyshchenko1/xen/commits/libxl_virtio_next
>> 2. Linux changes located at:
>> https://github.com/otyshchenko1/linux/commits/virtio_grant5
>> 3. virtio-disk changes located at:
>> https://github.com/otyshchenko1/virtio-disk/commits/virtio_grant
>>
>> Any feedback/help would be highly appreciated.
>>
>> [1] https://www.youtube.com/watch?v=IrlEdaIUDPk
>> [2] https://lore.kernel.org/xen-devel/1649442065-8332-1-git-send-email-olekstysh@gmail.com/
>>
>> Juergen Gross (2):
>>    xen/grants: support allocating consecutive grants
>>    virtio: add option to restrict memory access under Xen
>>
>> Oleksandr Tyshchenko (4):
>>    dt-bindings: xen: Add xen,dev-domid property description for
>>      xen-virtio layer
>>    virtio: Various updates to xen-virtio DMA ops layer
>>    arm/xen: Introduce xen_setup_dma_ops()
>>    arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
>>
>>   .../devicetree/bindings/virtio/xen,dev-domid.yaml  |  39 +++
>>   arch/arm/include/asm/xen/xen-ops.h                 |   1 +
>>   arch/arm/mm/dma-mapping.c                          |   5 +-
>>   arch/arm/xen/enlighten.c                           |  11 +
>>   arch/arm64/include/asm/xen/xen-ops.h               |   1 +
>>   arch/arm64/mm/dma-mapping.c                        |   5 +-
>>   arch/x86/mm/init.c                                 |  15 +
>>   arch/x86/mm/mem_encrypt.c                          |   5 -
>>   arch/x86/xen/Kconfig                               |   9 +
>>   drivers/xen/Kconfig                                |  20 ++
>>   drivers/xen/Makefile                               |   1 +
>>   drivers/xen/grant-table.c                          | 238 +++++++++++++--
>>   drivers/xen/xen-virtio.c                           | 335 +++++++++++++++++++++
>>   include/xen/arm/xen-ops.h                          |  20 ++
>>   include/xen/grant_table.h                          |   4 +
>>   include/xen/xen-ops.h                              |  13 +
>>   16 files changed, 679 insertions(+), 43 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
>>   create mode 100644 arch/arm/include/asm/xen/xen-ops.h
>>   create mode 100644 arch/arm64/include/asm/xen/xen-ops.h
>>   create mode 100644 drivers/xen/xen-virtio.c
>>   create mode 100644 include/xen/arm/xen-ops.h
>>
>> -- 
>> 2.7.4

-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen
  2022-04-14 19:19 ` [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen Oleksandr Tyshchenko
  2022-04-14 19:43   ` H. Peter Anvin
@ 2022-04-15 22:01   ` Stefano Stabellini
  2022-04-17 17:02     ` Oleksandr
  1 sibling, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-15 22:01 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: xen-devel, x86, linux-kernel, Juergen Gross, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, Boris Ostrovsky,
	Stefano Stabellini, Julien Grall, Oleksandr Tyshchenko

On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Juergen Gross <jgross@suse.com>
> 
> In order to support virtio in Xen guests add a config option enabling
> the user to specify whether in all Xen guests virtio should be able to
> access memory via Xen grant mappings only on the host side.
> 
> This applies to fully virtualized guests only, as for paravirtualized
> guests this is mandatory.
> 
> This requires to switch arch_has_restricted_virtio_memory_access()
> from a pure stub to a real function on x86 systems (Arm systems are
> not covered by now).
> 
> Add the needed functionality by providing a special set of DMA ops
> handling the needed grant operations for the I/O pages.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/mm/init.c        |  15 ++++
>  arch/x86/mm/mem_encrypt.c |   5 --
>  arch/x86/xen/Kconfig      |   9 +++
>  drivers/xen/Kconfig       |  20 ++++++
>  drivers/xen/Makefile      |   1 +
>  drivers/xen/xen-virtio.c  | 177 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/xen/xen-ops.h     |   8 +++
>  7 files changed, 230 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/xen/xen-virtio.c
> 
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index d8cfce2..526a3b2 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -8,6 +8,8 @@
>  #include <linux/kmemleak.h>
>  #include <linux/sched/task.h>
>  
> +#include <xen/xen.h>
> +
>  #include <asm/set_memory.h>
>  #include <asm/e820/api.h>
>  #include <asm/init.h>
> @@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
>  	return pages;
>  }
>  #endif
> +
> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> +int arch_has_restricted_virtio_memory_access(void)
> +{
> +	if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
> +		return 1;
> +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
> +		return 1;

I think these two checks could be moved to a separate function in a Xen
header, e.g. xen_restricted_virtio_memory_access, and here you could
just

if (xen_restricted_virtio_memory_access())
    return 1;



> +	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
> +}
> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
> +#endif
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 50d2099..dda020f 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
>  	print_mem_encrypt_feature_info();
>  }
>  
> -int arch_has_restricted_virtio_memory_access(void)
> -{
> -	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
> -}
> -EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> index 85246dd..dffdffd 100644
> --- a/arch/x86/xen/Kconfig
> +++ b/arch/x86/xen/Kconfig
> @@ -92,3 +92,12 @@ config XEN_DOM0
>  	select X86_X2APIC if XEN_PVH && X86_64
>  	help
>  	  Support running as a Xen Dom0 guest.
> +
> +config XEN_PV_VIRTIO
> +	bool "Xen virtio support for PV guests"
> +	depends on XEN_VIRTIO && XEN_PV
> +	default y
> +	help
> +	  Support virtio for running as a paravirtualized guest. This will
> +	  need support on the backend side (qemu or kernel, depending on the
> +	  virtio device types used).
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 120d32f..fc61f7a 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
>  	  having to balloon out RAM regions in order to obtain physical memory
>  	  space to create such mappings.
>  
> +config XEN_VIRTIO
> +	bool "Xen virtio support"
> +	default n
> +	depends on VIRTIO && DMA_OPS
> +	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> +	help
> +	  Enable virtio support for running as Xen guest. Depending on the
> +	  guest type this will require special support on the backend side
> +	  (qemu or kernel, depending on the virtio device types used).
> +
> +config XEN_HVM_VIRTIO_GRANT
> +	bool "Require virtio for fully virtualized guests to use grant mappings"
> +	depends on XEN_VIRTIO && X86_64
> +	default y
> +	help
> +	  Require virtio for fully virtualized guests to use grant mappings.
> +	  This will avoid the need to give the backend the right to map all
> +	  of the guest memory. This will need support on the backend side
> +	  (qemu or kernel, depending on the virtio device types used).

I don't think we need 3 visible kconfig options for this.

In fact, I would only add one: XEN_VIRTIO. We can have any X86 (or ARM)
specific dependencies in the "depends" line under XEN_VIRTIO. And I
don't think we need XEN_HVM_VIRTIO_GRANT as a kconfig option
necessarely. It doesn't seem like some we want as build time option. At
most, it could be a runtime option (like a command line) or a debug
option (like an #define at the top of the source file.)


>  endmenu
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 5aae66e..767009c 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -39,3 +39,4 @@ xen-gntalloc-y				:= gntalloc.o
>  xen-privcmd-y				:= privcmd.o privcmd-buf.o
>  obj-$(CONFIG_XEN_FRONT_PGDIR_SHBUF)	+= xen-front-pgdir-shbuf.o
>  obj-$(CONFIG_XEN_UNPOPULATED_ALLOC)	+= unpopulated-alloc.o
> +obj-$(CONFIG_XEN_VIRTIO)		+= xen-virtio.o
> diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
> new file mode 100644
> index 00000000..cfd5eda
> --- /dev/null
> +++ b/drivers/xen/xen-virtio.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/******************************************************************************
> + * Xen virtio driver - enables using virtio devices in Xen guests.
> + *
> + * Copyright (c) 2021, Juergen Gross <jgross@suse.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/dma-map-ops.h>
> +#include <linux/pci.h>
> +#include <linux/pfn.h>
> +#include <linux/virtio_config.h>
> +#include <xen/xen.h>
> +#include <xen/grant_table.h>
> +
> +#define XEN_GRANT_ADDR_OFF	0x8000000000000000ULL

NIT: (1ULL << 31)


> +static inline dma_addr_t grant_to_dma(grant_ref_t grant)
> +{
> +	return XEN_GRANT_ADDR_OFF | ((dma_addr_t)grant << PAGE_SHIFT);
> +}
> +
> +static inline grant_ref_t dma_to_grant(dma_addr_t dma)
> +{
> +	return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
> +}
> +
> +/*
> + * DMA ops for Xen virtio frontends.
> + *
> + * Used to act as a kind of software IOMMU for Xen guests by using grants as
> + * DMA addresses.
> + * Such a DMA address is formed by using the grant reference as a frame
> + * number and setting the highest address bit (this bit is for the backend
> + * to be able to distinguish it from e.g. a mmio address).
> + *
> + * Note that for now we hard wire dom0 to be the backend domain. In order to
> + * support any domain as backend we'd need to add a way to communicate the
> + * domid of this backend, e.g. via Xenstore or via the PCI-device's config
> + * space.

I would add device tree as possible way of domid communication


> + */
> +static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
> +				  dma_addr_t *dma_handle, gfp_t gfp,
> +				  unsigned long attrs)
> +{
> +	unsigned int n_pages = PFN_UP(size);
> +	unsigned int i;
> +	unsigned long pfn;
> +	grant_ref_t grant;
> +	void *ret;
> +
> +	ret = (void *)__get_free_pages(gfp, get_order(size));
> +	if (!ret)
> +		return NULL;
> +
> +	pfn = virt_to_pfn(ret);
> +
> +	if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
> +		free_pages((unsigned long)ret, get_order(size));
> +		return NULL;
> +	}
> +
> +	for (i = 0; i < n_pages; i++) {
> +		gnttab_grant_foreign_access_ref(grant + i, 0,
> +						pfn_to_gfn(pfn + i), 0);
> +	}
> +
> +	*dma_handle = grant_to_dma(grant);
> +
> +	return ret;
> +}
> +
> +static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
> +				dma_addr_t dma_handle, unsigned long attrs)
> +{
> +	unsigned int n_pages = PFN_UP(size);
> +	unsigned int i;
> +	grant_ref_t grant;
> +
> +	grant = dma_to_grant(dma_handle);
> +
> +	for (i = 0; i < n_pages; i++)
> +		gnttab_end_foreign_access_ref(grant + i);
> +
> +	gnttab_free_grant_reference_seq(grant, n_pages);
> +
> +	free_pages((unsigned long)vaddr, get_order(size));
> +}
> +
> +static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
> +					       dma_addr_t *dma_handle,
> +					       enum dma_data_direction dir,
> +					       gfp_t gfp)
> +{
> +	WARN_ONCE(1, "xen_virtio_dma_alloc_pages size %ld\n", size);
> +	return NULL;
> +}
> +
> +static void xen_virtio_dma_free_pages(struct device *dev, size_t size,
> +				      struct page *vaddr, dma_addr_t dma_handle,
> +				      enum dma_data_direction dir)
> +{
> +	WARN_ONCE(1, "xen_virtio_dma_free_pages size %ld\n", size);
> +}
> +
> +static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
> +					  unsigned long offset, size_t size,
> +					  enum dma_data_direction dir,
> +					  unsigned long attrs)
> +{
> +	grant_ref_t grant;
> +
> +	if (gnttab_alloc_grant_references(1, &grant))
> +		return 0;
> +
> +	gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
> +					dir == DMA_TO_DEVICE);
> +	return grant_to_dma(grant) + offset;
> +}
> +
> +static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> +				      size_t size, enum dma_data_direction dir,
> +				      unsigned long attrs)
> +{
> +	grant_ref_t grant;
> +
> +	grant = dma_to_grant(dma_handle);
> +
> +	gnttab_end_foreign_access_ref(grant);
> +
> +	gnttab_free_grant_reference(grant);
> +}
> +
> +static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
> +				 int nents, enum dma_data_direction dir,
> +				 unsigned long attrs)
> +{
> +	WARN_ONCE(1, "xen_virtio_dma_map_sg nents %d\n", nents);
> +	return -EINVAL;
> +}
> +
> +static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> +				    int nents, enum dma_data_direction dir,
> +				    unsigned long attrs)
> +{
> +	WARN_ONCE(1, "xen_virtio_dma_unmap_sg nents %d\n", nents);
> +}

You can implement xen_virtio_dma_map_sg and xen_virtio_dma_unmap_sg
based on xen_virtio_dma_map_page and xen_virtio_dma_unmap_page, like we
do in drivers/xen/swiotlb-xen.c.


> +static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
> +{
> +	return 1;
> +}
> +
> +static const struct dma_map_ops xen_virtio_dma_ops = {
> +	.alloc = xen_virtio_dma_alloc,
> +	.free = xen_virtio_dma_free,
> +	.alloc_pages = xen_virtio_dma_alloc_pages,
> +	.free_pages = xen_virtio_dma_free_pages,
> +	.mmap = dma_common_mmap,
> +	.get_sgtable = dma_common_get_sgtable,
> +	.map_page = xen_virtio_dma_map_page,
> +	.unmap_page = xen_virtio_dma_unmap_page,
> +	.map_sg = xen_virtio_dma_map_sg,
> +	.unmap_sg = xen_virtio_dma_unmap_sg,
> +	.dma_supported = xen_virtio_dma_dma_supported,
> +};
> +
> +void xen_virtio_setup_dma_ops(struct device *dev)
> +{
> +	dev->dma_ops = &xen_virtio_dma_ops;
> +}
> +EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
> +
> +MODULE_DESCRIPTION("Xen virtio support driver");
> +MODULE_AUTHOR("Juergen Gross <jgross@suse.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index a3584a3..ae3c1bc 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -221,4 +221,12 @@ static inline void xen_preemptible_hcall_end(void) { }
>  
>  #endif /* CONFIG_XEN_PV && !CONFIG_PREEMPTION */
>  
> +#ifdef CONFIG_XEN_VIRTIO
> +void xen_virtio_setup_dma_ops(struct device *dev);
> +#else
> +static inline void xen_virtio_setup_dma_ops(struct device *dev)
> +{
> +}
> +#endif /* CONFIG_XEN_VIRTIO */
> +
>  #endif /* INCLUDE_XEN_OPS_H */
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/6] dt-bindings: xen: Add xen,dev-domid property description for xen-virtio layer
  2022-04-14 19:19 ` [RFC PATCH 3/6] dt-bindings: xen: Add xen,dev-domid property description for xen-virtio layer Oleksandr Tyshchenko
@ 2022-04-15 22:01   ` Stefano Stabellini
  2022-04-17 17:24     ` Oleksandr
  0 siblings, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-15 22:01 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: xen-devel, virtualization, devicetree, linux-kernel,
	Oleksandr Tyshchenko, Michael S. Tsirkin, Jason Wang,
	Rob Herring, Krzysztof Kozlowski, Julien Grall, Juergen Gross,
	Stefano Stabellini

On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Introduce Xen specific binding for the virtio-mmio device to be used
> by Xen virtio support driver in a subsequent commit.
> 
> This binding specifies the ID of Xen domain where the corresponding
> device (backend) resides. This is needed for the option to restrict
> memory access using Xen grant mappings to work.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  .../devicetree/bindings/virtio/xen,dev-domid.yaml  | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
> 
> diff --git a/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml b/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
> new file mode 100644
> index 00000000..78be993
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/virtio/xen,dev-domid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xen specific binding for the virtio device
> +
> +maintainers:
> +  - Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> +
> +select: true
> +
> +description:
> +  This binding specifies the ID of Xen domain where the corresponding device
> +  (backend) resides. This is needed for the option to restrict memory access
> +  using Xen grant mappings to work.
> +
> +  Note that current and generic "iommus" bindings are mutually exclusive, since
> +  the restricted memory access model on Xen behaves as a kind of software IOMMU.

I don't think that this last statement is necessary or fully accurate, so
I would remove it. Other than that, this looks good to me.


> +properties:
> +  xen,dev-domid:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Should contain the ID of device's domain.

Maybe better as:
"The domid (domain ID) of the domain where the device (backend) is running"



> +additionalProperties: true
> +
> +examples:
> +  - |
> +    virtio_block@3000 {
> +            compatible = "virtio,mmio";
> +            reg = <0x3000 0x100>;
> +            interrupts = <41>;
> +
> +            /* The device is located in Xen domain with ID 1 */
> +            xen,dev-domid = <1>;
> +    };
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer
  2022-04-14 19:19 ` [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer Oleksandr Tyshchenko
@ 2022-04-15 22:02   ` Stefano Stabellini
  2022-04-17 18:21     ` Oleksandr
  2022-04-16  6:05   ` Christoph Hellwig
  1 sibling, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-15 22:02 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Stefano Stabellini, Russell King, Boris Ostrovsky, Juergen Gross,
	Julien Grall

[-- Attachment #1: Type: text/plain, Size: 11759 bytes --]

On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> In the context of current patch do the following:
> 1. Update code to support virtio-mmio devices
> 2. Introduce struct xen_virtio_data and account passed virtio devices
>    (using list) as we need to store some per-device data
> 3. Add multi-page support for xen_virtio_dma_map(unmap)_page callbacks
> 4. Harden code against malicious backend
> 5. Change to use alloc_pages_exact() instead of __get_free_pages()
> 6. Introduce locking scheme to protect mappings (I am not 100% sure
>    whether per-device lock is really needed)
> 7. Handle virtio device's DMA mask
> 8. Retrieve the ID of backend domain from DT for virtio-mmio device
>    instead of hardcoding it.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  arch/arm/xen/enlighten.c |  11 +++
>  drivers/xen/Kconfig      |   2 +-
>  drivers/xen/xen-virtio.c | 200 ++++++++++++++++++++++++++++++++++++++++++-----
>  include/xen/xen-ops.h    |   5 ++
>  4 files changed, 196 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index ec5b082..870d92f 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -409,6 +409,17 @@ int __init arch_xen_unpopulated_init(struct resource **res)
>  }
>  #endif
>  
> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> +int arch_has_restricted_virtio_memory_access(void)
> +{
> +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
> +		return 1;

Instead of xen_hvm_domain(), you can just use xen_domain(). Also there
is no need for the #ifdef
CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, given that:

CONFIG_XEN_HVM_VIRTIO_GRANT depends on XEN_VIRTIO which selects
ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS


> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
> +#endif
> +
>  static void __init xen_dt_guest_init(void)
>  {
>  	struct device_node *xen_node;
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index fc61f7a..56afe6a 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -347,7 +347,7 @@ config XEN_VIRTIO
>  
>  config XEN_HVM_VIRTIO_GRANT
>  	bool "Require virtio for fully virtualized guests to use grant mappings"
> -	depends on XEN_VIRTIO && X86_64
> +	depends on XEN_VIRTIO && (X86_64 || ARM || ARM64)

you can remove the architectural dependencies


>  	default y
>  	help
>  	  Require virtio for fully virtualized guests to use grant mappings.
> diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
> index cfd5eda..c5b2ec9 100644
> --- a/drivers/xen/xen-virtio.c
> +++ b/drivers/xen/xen-virtio.c
> @@ -7,12 +7,26 @@
>  
>  #include <linux/module.h>
>  #include <linux/dma-map-ops.h>
> +#include <linux/of.h>
>  #include <linux/pci.h>
>  #include <linux/pfn.h>
>  #include <linux/virtio_config.h>
>  #include <xen/xen.h>
>  #include <xen/grant_table.h>
>  
> +struct xen_virtio_data {
> +	/* The ID of backend domain */
> +	domid_t dev_domid;
> +	struct device *dev;
> +	struct list_head list;
> +	spinlock_t lock;
> +	/* Is device behaving sane? */
> +	bool broken;

If you moved "broken" after "dev_domid" we would save a few bytes for
every allocation due to padding.

Is data->lock only there to protect accesses to "broken"? If so, we
might not need it, but I am not sure.


> +};
> +
> +static LIST_HEAD(xen_virtio_devices);
> +static DEFINE_SPINLOCK(xen_virtio_lock);
> +
>  #define XEN_GRANT_ADDR_OFF	0x8000000000000000ULL
>  
>  static inline dma_addr_t grant_to_dma(grant_ref_t grant)
> @@ -25,6 +39,25 @@ static inline grant_ref_t dma_to_grant(dma_addr_t dma)
>  	return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
>  }
>  
> +static struct xen_virtio_data *find_xen_virtio_data(struct device *dev)
> +{
> +	struct xen_virtio_data *data = NULL;
> +	bool found = false;
> +
> +	spin_lock(&xen_virtio_lock);
> +
> +	list_for_each_entry( data, &xen_virtio_devices, list) {
> +		if (data->dev == dev) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	spin_unlock(&xen_virtio_lock);
> +
> +	return found ? data : NULL;
> +}
> +
>  /*
>   * DMA ops for Xen virtio frontends.
>   *
> @@ -43,48 +76,78 @@ static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
>  				  dma_addr_t *dma_handle, gfp_t gfp,
>  				  unsigned long attrs)
>  {
> -	unsigned int n_pages = PFN_UP(size);
> -	unsigned int i;
> +	struct xen_virtio_data *data;
> +	unsigned int i, n_pages = PFN_UP(size);
>  	unsigned long pfn;
>  	grant_ref_t grant;
> -	void *ret;
> +	void *ret = NULL;
>  
> -	ret = (void *)__get_free_pages(gfp, get_order(size));
> -	if (!ret)
> +	data = find_xen_virtio_data(dev);
> +	if (!data)
>  		return NULL;
>  
> +	spin_lock(&data->lock);
> +
> +	if (unlikely(data->broken))
> +		goto out;
> +
> +	ret = alloc_pages_exact(n_pages * PAGE_SIZE, gfp);
> +	if (!ret)
> +		goto out;
> +
>  	pfn = virt_to_pfn(ret);
>  
>  	if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
> -		free_pages((unsigned long)ret, get_order(size));
> -		return NULL;
> +		free_pages_exact(ret, n_pages * PAGE_SIZE);
> +		ret = NULL;
> +		goto out;
>  	}
>  
>  	for (i = 0; i < n_pages; i++) {
> -		gnttab_grant_foreign_access_ref(grant + i, 0,
> +		gnttab_grant_foreign_access_ref(grant + i, data->dev_domid,
>  						pfn_to_gfn(pfn + i), 0);
>  	}
>  
>  	*dma_handle = grant_to_dma(grant);
>  
> +out:
> +	spin_unlock(&data->lock);
> +
>  	return ret;
>  }
>  
>  static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
>  				dma_addr_t dma_handle, unsigned long attrs)
>  {
> -	unsigned int n_pages = PFN_UP(size);
> -	unsigned int i;
> +	struct xen_virtio_data *data;
> +	unsigned int i, n_pages = PFN_UP(size);
>  	grant_ref_t grant;
>  
> +	data = find_xen_virtio_data(dev);
> +	if (!data)
> +		return;
> +
> +	spin_lock(&data->lock);
> +
> +	if (unlikely(data->broken))
> +		goto out;
> +
>  	grant = dma_to_grant(dma_handle);
>  
> -	for (i = 0; i < n_pages; i++)
> -		gnttab_end_foreign_access_ref(grant + i);
> +	for (i = 0; i < n_pages; i++) {
> +		if (unlikely(!gnttab_end_foreign_access_ref(grant + i))) {
> +			dev_alert(dev, "Grant still in use by backend domain, disabled for further use\n");
> +			data->broken = true;
> +			goto out;
> +		}
> +	}
>  
>  	gnttab_free_grant_reference_seq(grant, n_pages);
>  
> -	free_pages((unsigned long)vaddr, get_order(size));
> +	free_pages_exact(vaddr, n_pages * PAGE_SIZE);
> +
> +out:
> +	spin_unlock(&data->lock);
>  }
>  
>  static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
> @@ -108,28 +171,71 @@ static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
>  					  enum dma_data_direction dir,
>  					  unsigned long attrs)
>  {
> +	struct xen_virtio_data *data;
> +	unsigned int i, n_pages = PFN_UP(size);
>  	grant_ref_t grant;
> +	dma_addr_t dma_handle = DMA_MAPPING_ERROR;
> +
> +	BUG_ON(dir == DMA_NONE);
> +
> +	data = find_xen_virtio_data(dev);
> +	if (!data)
> +		return DMA_MAPPING_ERROR;
> +
> +	spin_lock(&data->lock);
>  
> -	if (gnttab_alloc_grant_references(1, &grant))
> -		return 0;
> +	if (unlikely(data->broken))
> +		goto out;
>  
> -	gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
> -					dir == DMA_TO_DEVICE);
> +	if (gnttab_alloc_grant_reference_seq(n_pages, &grant))
> +		goto out;
>  
> -	return grant_to_dma(grant) + offset;
> +	for (i = 0; i < n_pages; i++) {
> +		gnttab_grant_foreign_access_ref(grant + i, data->dev_domid,
> +				xen_page_to_gfn(page) + i, dir == DMA_TO_DEVICE);
> +	}
> +
> +	dma_handle = grant_to_dma(grant) + offset;
> +
> +out:
> +	spin_unlock(&data->lock);
> +
> +	return dma_handle;
>  }
>  
>  static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>  				      size_t size, enum dma_data_direction dir,
>  				      unsigned long attrs)
>  {
> +	struct xen_virtio_data *data;
> +	unsigned int i, n_pages = PFN_UP(size);
>  	grant_ref_t grant;
>  
> +	BUG_ON(dir == DMA_NONE);
> +
> +	data = find_xen_virtio_data(dev);
> +	if (!data)
> +		return;
> +
> +	spin_lock(&data->lock);
> +
> +	if (unlikely(data->broken))
> +		goto out;
> +
>  	grant = dma_to_grant(dma_handle);
>  
> -	gnttab_end_foreign_access_ref(grant);
> +	for (i = 0; i < n_pages; i++) {
> +		if (unlikely(!gnttab_end_foreign_access_ref(grant + i))) {
> +			dev_alert(dev, "Grant still in use by backend domain, disabled for further use\n");
> +			data->broken = true;
> +			goto out;
> +		}
> +	}
> +
> +	gnttab_free_grant_reference_seq(grant, n_pages);
>  
> -	gnttab_free_grant_reference(grant);
> +out:
> +	spin_unlock(&data->lock);
>  }
>  
>  static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
> @@ -149,7 +255,7 @@ static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>  
>  static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
>  {
> -	return 1;
> +	return mask == DMA_BIT_MASK(64);
>  }
>  
>  static const struct dma_map_ops xen_virtio_dma_ops = {
> @@ -166,9 +272,61 @@ static const struct dma_map_ops xen_virtio_dma_ops = {
>  	.dma_supported = xen_virtio_dma_dma_supported,
>  };
>  
> +bool xen_is_virtio_device(struct device *dev)
> +{
> +	/* XXX Handle only DT devices for now */
> +	if (!dev->of_node)
> +		return false;
> +
> +	if (!of_device_is_compatible(dev->of_node, "virtio,mmio"))
> +		return false;
> +
> +	return of_property_read_bool(dev->of_node, "xen,dev-domid");
> +}
> +EXPORT_SYMBOL_GPL(xen_is_virtio_device);
> +
>  void xen_virtio_setup_dma_ops(struct device *dev)
>  {
> +	struct xen_virtio_data *data;
> +	uint32_t dev_domid;
> +
> +	data = find_xen_virtio_data(dev);
> +	if (data) {
> +		dev_err(dev, "xen_virtio data is already created\n");
> +		return;
> +	}
> +
> +	if (dev_is_pci(dev)) {
> +		/* XXX Leave it hard wired to dom0 for now */
> +		dev_domid = 0;
> +	} else if (dev->of_node) {
> +		if (of_property_read_u32(dev->of_node, "xen,dev-domid", &dev_domid)) {
> +			dev_err(dev, "xen,dev-domid property is not present\n");
> +			goto err;
> +		}
> +	} else
> +		/* The ACPI case is not supported */
> +		goto err;

If we get here, it means that xen_is_virtio_device returned true, so the
PCI case is actually impossible?

I would rewrite these checks like this:

/* XXX: ACPI and PCI unsupported for now */
if (dev_is_pci(dev) || !dev->of_node) {
	goto err;
}
if (of_property_read_u32(dev->of_node, "xen,dev-domid", &dev_domid)) {
	dev_err(dev, "xen,dev-domid property is not present\n");
	goto err;
}



> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data) {
> +		dev_err(dev, "Сannot allocate xen_virtio data\n");
> +		goto err;
> +	}
> +	data->dev_domid = dev_domid;
> +	data->dev = dev;
> +	spin_lock_init(&data->lock);
> +
> +	spin_lock(&xen_virtio_lock);
> +	list_add(&data->list, &xen_virtio_devices);
> +	spin_unlock(&xen_virtio_lock);
> +
>  	dev->dma_ops = &xen_virtio_dma_ops;
> +
> +	return;
> +
> +err:
> +	dev_err(dev, "Сannot set up xen_virtio DMA ops, retain platform DMA ops\n");
>  }
>  EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
>  
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index ae3c1bc..fdbcb99 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -223,10 +223,15 @@ static inline void xen_preemptible_hcall_end(void) { }
>  
>  #ifdef CONFIG_XEN_VIRTIO
>  void xen_virtio_setup_dma_ops(struct device *dev);
> +bool xen_is_virtio_device(struct device *dev);
>  #else
>  static inline void xen_virtio_setup_dma_ops(struct device *dev)
>  {
>  }
> +static inline bool xen_is_virtio_device(struct device *dev)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_XEN_VIRTIO */
>  
>  #endif /* INCLUDE_XEN_OPS_H */

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 5/6] arm/xen: Introduce xen_setup_dma_ops()
  2022-04-14 19:19 ` [RFC PATCH 5/6] arm/xen: Introduce xen_setup_dma_ops() Oleksandr Tyshchenko
@ 2022-04-15 22:02   ` Stefano Stabellini
  2022-04-17 18:43     ` Oleksandr
  0 siblings, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-15 22:02 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Stefano Stabellini, Russell King, Catalin Marinas, Will Deacon,
	Boris Ostrovsky, Juergen Gross, Logan Gunthorpe,
	David Hildenbrand, Martin Oliveira, Kees Cook,
	Jean-Philippe Brucker, Julien Grall

On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> This patch introduces new helper and places it in new header.
> The helper's purpose is to assign any Xen specific DMA ops in
> a single place. For now, we deal with xen-swiotlb DMA ops only.
> The subsequent patch will add xen-virtio DMA ops case.
> 
> Also re-use the xen_swiotlb_detect() check on Arm32.

Thanks for the patch, this is good to have in any case. I would move it
to the beginning of the series.


> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  arch/arm/include/asm/xen/xen-ops.h   |  1 +
>  arch/arm/mm/dma-mapping.c            |  5 ++---
>  arch/arm64/include/asm/xen/xen-ops.h |  1 +
>  arch/arm64/mm/dma-mapping.c          |  5 ++---
>  include/xen/arm/xen-ops.h            | 13 +++++++++++++
>  5 files changed, 19 insertions(+), 6 deletions(-)
>  create mode 100644 arch/arm/include/asm/xen/xen-ops.h
>  create mode 100644 arch/arm64/include/asm/xen/xen-ops.h
>  create mode 100644 include/xen/arm/xen-ops.h
> 
> diff --git a/arch/arm/include/asm/xen/xen-ops.h b/arch/arm/include/asm/xen/xen-ops.h
> new file mode 100644
> index 00000000..8d2fa24
> --- /dev/null
> +++ b/arch/arm/include/asm/xen/xen-ops.h
> @@ -0,0 +1 @@
> +#include <xen/arm/xen-ops.h>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 82ffac6..a1bf9dd 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -33,7 +33,7 @@
>  #include <asm/dma-iommu.h>
>  #include <asm/mach/map.h>
>  #include <asm/system_info.h>
> -#include <xen/swiotlb-xen.h>
> +#include <asm/xen/xen-ops.h>
>  
>  #include "dma.h"
>  #include "mm.h"
> @@ -2288,8 +2288,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  	set_dma_ops(dev, dma_ops);
>  
>  #ifdef CONFIG_XEN
> -	if (xen_initial_domain())
> -		dev->dma_ops = &xen_swiotlb_dma_ops;
> +	xen_setup_dma_ops(dev);
>  #endif

You can take this opportunity to also remove the #ifdef, by providing an
empty stub implemention of xen_setup_dma_ops for the !CONFIG_XEN case.


>  	dev->archdata.dma_ops_setup = true;
>  }
> diff --git a/arch/arm64/include/asm/xen/xen-ops.h b/arch/arm64/include/asm/xen/xen-ops.h
> new file mode 100644
> index 00000000..8d2fa24
> --- /dev/null
> +++ b/arch/arm64/include/asm/xen/xen-ops.h
> @@ -0,0 +1 @@
> +#include <xen/arm/xen-ops.h>
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 6719f9e..831e673 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -9,9 +9,9 @@
>  #include <linux/dma-map-ops.h>
>  #include <linux/dma-iommu.h>
>  #include <xen/xen.h>
> -#include <xen/swiotlb-xen.h>
>  
>  #include <asm/cacheflush.h>
> +#include <asm/xen/xen-ops.h>
>  
>  void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>  		enum dma_data_direction dir)
> @@ -53,7 +53,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  		iommu_setup_dma_ops(dev, dma_base, dma_base + size - 1);
>  
>  #ifdef CONFIG_XEN
> -	if (xen_swiotlb_detect())
> -		dev->dma_ops = &xen_swiotlb_dma_ops;
> +	xen_setup_dma_ops(dev);
>  #endif

same here


>  }
> diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h
> new file mode 100644
> index 00000000..621da05
> --- /dev/null
> +++ b/include/xen/arm/xen-ops.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_ARM_XEN_OPS_H
> +#define _ASM_ARM_XEN_OPS_H
> +
> +#include <xen/swiotlb-xen.h>
> +
> +static inline void xen_setup_dma_ops(struct device *dev)
> +{
> +	if (xen_swiotlb_detect())
> +		dev->dma_ops = &xen_swiotlb_dma_ops;
> +}
> +
> +#endif /* _ASM_ARM_XEN_OPS_H */
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-14 19:19 ` [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests Oleksandr Tyshchenko
@ 2022-04-15 22:02   ` Stefano Stabellini
  2022-04-16  6:07     ` Christoph Hellwig
  2022-04-17 19:20     ` Oleksandr
  0 siblings, 2 replies; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-15 22:02 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Boris Ostrovsky, Juergen Gross, Stefano Stabellini, Julien Grall

On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Call xen_virtio_setup_dma_ops() only for Xen-aware virtio devices
> in Xen guests if restricted access to the guest memory is enabled.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  include/xen/arm/xen-ops.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h
> index 621da05..28b2ad3 100644
> --- a/include/xen/arm/xen-ops.h
> +++ b/include/xen/arm/xen-ops.h
> @@ -2,12 +2,19 @@
>  #ifndef _ASM_ARM_XEN_OPS_H
>  #define _ASM_ARM_XEN_OPS_H
>  
> +#include <linux/virtio_config.h>
>  #include <xen/swiotlb-xen.h>
> +#include <xen/xen-ops.h>
>  
>  static inline void xen_setup_dma_ops(struct device *dev)
>  {
>  	if (xen_swiotlb_detect())
>  		dev->dma_ops = &xen_swiotlb_dma_ops;
> +
> +#ifdef CONFIG_XEN_VIRTIO
> +	if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev))
> +		xen_virtio_setup_dma_ops(dev);
> +#endif

This makes sense overall. Considering that the swiotlb-xen case and the
virtio case are mutually exclusive, I would write it like this:

	if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev))
		xen_virtio_setup_dma_ops(dev);
	else if (xen_swiotlb_detect())
		dev->dma_ops = &xen_swiotlb_dma_ops;

There is no need for #ifdef (also see other comments).

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer
  2022-04-14 19:19 ` [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer Oleksandr Tyshchenko
  2022-04-15 22:02   ` Stefano Stabellini
@ 2022-04-16  6:05   ` Christoph Hellwig
  2022-04-17 18:39     ` Oleksandr
  1 sibling, 1 reply; 40+ messages in thread
From: Christoph Hellwig @ 2022-04-16  6:05 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Stefano Stabellini, Russell King, Boris Ostrovsky, Juergen Gross,
	Julien Grall

On Thu, Apr 14, 2022 at 10:19:31PM +0300, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Various updates is a big indicator that the patch should be split
further.  Please do one change at a time, and fold updates to the
previous patches in the series into those patches instead of fixing
them up later.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-15 22:02   ` Stefano Stabellini
@ 2022-04-16  6:07     ` Christoph Hellwig
  2022-04-17 21:05       ` Oleksandr
  2022-04-17 19:20     ` Oleksandr
  1 sibling, 1 reply; 40+ messages in thread
From: Christoph Hellwig @ 2022-04-16  6:07 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Oleksandr Tyshchenko, xen-devel, linux-kernel, linux-arm-kernel,
	Oleksandr Tyshchenko, Boris Ostrovsky, Juergen Gross,
	Julien Grall

On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
> This makes sense overall. Considering that the swiotlb-xen case and the
> virtio case are mutually exclusive, I would write it like this:

Curious question:  Why can't the same grant scheme also be used for
non-virtio devices?  I really hate having virtio hooks in the arch
dma code.  Why can't Xen just say in DT/ACPI that grants can be used
for a given device?

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen
  2022-04-15 22:01   ` Stefano Stabellini
@ 2022-04-17 17:02     ` Oleksandr
  2022-04-18 19:11       ` Stefano Stabellini
  0 siblings, 1 reply; 40+ messages in thread
From: Oleksandr @ 2022-04-17 17:02 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, x86, linux-kernel, Juergen Gross, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, Boris Ostrovsky, Julien Grall,
	Oleksandr Tyshchenko, linux-arm-kernel, Christoph Hellwig,
	Michael S. Tsirkin


On 16.04.22 01:01, Stefano Stabellini wrote:


Hello Stefano


> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>> From: Juergen Gross <jgross@suse.com>
>>
>> In order to support virtio in Xen guests add a config option enabling
>> the user to specify whether in all Xen guests virtio should be able to
>> access memory via Xen grant mappings only on the host side.
>>
>> This applies to fully virtualized guests only, as for paravirtualized
>> guests this is mandatory.
>>
>> This requires to switch arch_has_restricted_virtio_memory_access()
>> from a pure stub to a real function on x86 systems (Arm systems are
>> not covered by now).
>>
>> Add the needed functionality by providing a special set of DMA ops
>> handling the needed grant operations for the I/O pages.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   arch/x86/mm/init.c        |  15 ++++
>>   arch/x86/mm/mem_encrypt.c |   5 --
>>   arch/x86/xen/Kconfig      |   9 +++
>>   drivers/xen/Kconfig       |  20 ++++++
>>   drivers/xen/Makefile      |   1 +
>>   drivers/xen/xen-virtio.c  | 177 ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/xen/xen-ops.h     |   8 +++
>>   7 files changed, 230 insertions(+), 5 deletions(-)
>>   create mode 100644 drivers/xen/xen-virtio.c
>>
>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>> index d8cfce2..526a3b2 100644
>> --- a/arch/x86/mm/init.c
>> +++ b/arch/x86/mm/init.c
>> @@ -8,6 +8,8 @@
>>   #include <linux/kmemleak.h>
>>   #include <linux/sched/task.h>
>>   
>> +#include <xen/xen.h>
>> +
>>   #include <asm/set_memory.h>
>>   #include <asm/e820/api.h>
>>   #include <asm/init.h>
>> @@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
>>   	return pages;
>>   }
>>   #endif
>> +
>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>> +int arch_has_restricted_virtio_memory_access(void)
>> +{
>> +	if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
>> +		return 1;
>> +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>> +		return 1;
> I think these two checks could be moved to a separate function in a Xen
> header, e.g. xen_restricted_virtio_memory_access, and here you could
> just
>
> if (xen_restricted_virtio_memory_access())
>      return 1;

Agree, will do


>
>
>
>> +	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>> +}
>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>> +#endif
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 50d2099..dda020f 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
>>   	print_mem_encrypt_feature_info();
>>   }
>>   
>> -int arch_has_restricted_virtio_memory_access(void)
>> -{
>> -	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>> -}
>> -EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
>> index 85246dd..dffdffd 100644
>> --- a/arch/x86/xen/Kconfig
>> +++ b/arch/x86/xen/Kconfig
>> @@ -92,3 +92,12 @@ config XEN_DOM0
>>   	select X86_X2APIC if XEN_PVH && X86_64
>>   	help
>>   	  Support running as a Xen Dom0 guest.
>> +
>> +config XEN_PV_VIRTIO
>> +	bool "Xen virtio support for PV guests"
>> +	depends on XEN_VIRTIO && XEN_PV
>> +	default y
>> +	help
>> +	  Support virtio for running as a paravirtualized guest. This will
>> +	  need support on the backend side (qemu or kernel, depending on the
>> +	  virtio device types used).
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index 120d32f..fc61f7a 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
>>   	  having to balloon out RAM regions in order to obtain physical memory
>>   	  space to create such mappings.
>>   
>> +config XEN_VIRTIO
>> +	bool "Xen virtio support"
>> +	default n
>> +	depends on VIRTIO && DMA_OPS
>> +	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>> +	help
>> +	  Enable virtio support for running as Xen guest. Depending on the
>> +	  guest type this will require special support on the backend side
>> +	  (qemu or kernel, depending on the virtio device types used).
>> +
>> +config XEN_HVM_VIRTIO_GRANT
>> +	bool "Require virtio for fully virtualized guests to use grant mappings"
>> +	depends on XEN_VIRTIO && X86_64
>> +	default y
>> +	help
>> +	  Require virtio for fully virtualized guests to use grant mappings.
>> +	  This will avoid the need to give the backend the right to map all
>> +	  of the guest memory. This will need support on the backend side
>> +	  (qemu or kernel, depending on the virtio device types used).
> I don't think we need 3 visible kconfig options for this.
>
> In fact, I would only add one: XEN_VIRTIO. We can have any X86 (or ARM)
> specific dependencies in the "depends" line under XEN_VIRTIO. And I
> don't think we need XEN_HVM_VIRTIO_GRANT as a kconfig option
> necessarely. It doesn't seem like some we want as build time option. At
> most, it could be a runtime option (like a command line) or a debug
> option (like an #define at the top of the source file.)


I don't know what was the initial idea of having and extra 
XEN_HVM_VIRTIO and XEN_PV_VIRTIO options, but taking into the account that
they are only used in arch_has_restricted_virtio_memory_access() 
currently, I share your opinion regarding a single XEN_VIRTIO option.

Looking ahead (including changes in the commit #4), we can imagine the 
resulting option:

config XEN_VIRTIO
     bool "Xen virtio support"
     default n
     depends on VIRTIO && DMA_OPS
     depends on (X86_64 || ARM || ARM64)
     select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
     help
       Enable virtio support for running as Xen guest. Depending on the
       guest type this will require special support on the backend side
       (qemu or kernel, depending on the virtio device types used).


and then arch_has_restricted_virtio_memory_access() per arch:


1. x86:

int arch_has_restricted_virtio_memory_access(void)
{
     return (xen_has_restricted_virtio_memory_access() ||
             cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
}


2. Arm:

int arch_has_restricted_virtio_memory_access(void)
{
     return xen_has_restricted_virtio_memory_access();
}


3. xen.h:

static inline int xen_has_restricted_virtio_memory_access(void)
{
     if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() || 
xen_hvm_domain()))
         return 1;

     return 0;
}


Actually, as domain type on Arm is always XEN_HVM_DOMAIN, we could 
probably have the following on Arm:

int arch_has_restricted_virtio_memory_access(void)
{
     return IS_ENABLED(CONFIG_XEN_VIRTIO);
}

but I would prefer not to diverge and use common 
xen_has_restricted_virtio_memory_access().

Any thoughts?



>
>
>>   endmenu
>> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
>> index 5aae66e..767009c 100644
>> --- a/drivers/xen/Makefile
>> +++ b/drivers/xen/Makefile
>> @@ -39,3 +39,4 @@ xen-gntalloc-y				:= gntalloc.o
>>   xen-privcmd-y				:= privcmd.o privcmd-buf.o
>>   obj-$(CONFIG_XEN_FRONT_PGDIR_SHBUF)	+= xen-front-pgdir-shbuf.o
>>   obj-$(CONFIG_XEN_UNPOPULATED_ALLOC)	+= unpopulated-alloc.o
>> +obj-$(CONFIG_XEN_VIRTIO)		+= xen-virtio.o
>> diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
>> new file mode 100644
>> index 00000000..cfd5eda
>> --- /dev/null
>> +++ b/drivers/xen/xen-virtio.c
>> @@ -0,0 +1,177 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/******************************************************************************
>> + * Xen virtio driver - enables using virtio devices in Xen guests.
>> + *
>> + * Copyright (c) 2021, Juergen Gross <jgross@suse.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/dma-map-ops.h>
>> +#include <linux/pci.h>
>> +#include <linux/pfn.h>
>> +#include <linux/virtio_config.h>
>> +#include <xen/xen.h>
>> +#include <xen/grant_table.h>
>> +
>> +#define XEN_GRANT_ADDR_OFF	0x8000000000000000ULL
> NIT: (1ULL << 31)

ok, I assume you meant (1ULL << 63)?


>
>
>> +static inline dma_addr_t grant_to_dma(grant_ref_t grant)
>> +{
>> +	return XEN_GRANT_ADDR_OFF | ((dma_addr_t)grant << PAGE_SHIFT);
>> +}
>> +
>> +static inline grant_ref_t dma_to_grant(dma_addr_t dma)
>> +{
>> +	return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
>> +}
>> +
>> +/*
>> + * DMA ops for Xen virtio frontends.
>> + *
>> + * Used to act as a kind of software IOMMU for Xen guests by using grants as
>> + * DMA addresses.
>> + * Such a DMA address is formed by using the grant reference as a frame
>> + * number and setting the highest address bit (this bit is for the backend
>> + * to be able to distinguish it from e.g. a mmio address).
>> + *
>> + * Note that for now we hard wire dom0 to be the backend domain. In order to
>> + * support any domain as backend we'd need to add a way to communicate the
>> + * domid of this backend, e.g. via Xenstore or via the PCI-device's config
>> + * space.
> I would add device tree as possible way of domid communication

I agree, but changes in the commit #4 (which add DT support and remove 
hardcoded domid 0) render this comment stale. For the next version I 
will squash changes and drop or rephrase this comment.


>
>
>> + */
>> +static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
>> +				  dma_addr_t *dma_handle, gfp_t gfp,
>> +				  unsigned long attrs)
>> +{
>> +	unsigned int n_pages = PFN_UP(size);
>> +	unsigned int i;
>> +	unsigned long pfn;
>> +	grant_ref_t grant;
>> +	void *ret;
>> +
>> +	ret = (void *)__get_free_pages(gfp, get_order(size));
>> +	if (!ret)
>> +		return NULL;
>> +
>> +	pfn = virt_to_pfn(ret);
>> +
>> +	if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
>> +		free_pages((unsigned long)ret, get_order(size));
>> +		return NULL;
>> +	}
>> +
>> +	for (i = 0; i < n_pages; i++) {
>> +		gnttab_grant_foreign_access_ref(grant + i, 0,
>> +						pfn_to_gfn(pfn + i), 0);
>> +	}
>> +
>> +	*dma_handle = grant_to_dma(grant);
>> +
>> +	return ret;
>> +}
>> +
>> +static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
>> +				dma_addr_t dma_handle, unsigned long attrs)
>> +{
>> +	unsigned int n_pages = PFN_UP(size);
>> +	unsigned int i;
>> +	grant_ref_t grant;
>> +
>> +	grant = dma_to_grant(dma_handle);
>> +
>> +	for (i = 0; i < n_pages; i++)
>> +		gnttab_end_foreign_access_ref(grant + i);
>> +
>> +	gnttab_free_grant_reference_seq(grant, n_pages);
>> +
>> +	free_pages((unsigned long)vaddr, get_order(size));
>> +}
>> +
>> +static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
>> +					       dma_addr_t *dma_handle,
>> +					       enum dma_data_direction dir,
>> +					       gfp_t gfp)
>> +{
>> +	WARN_ONCE(1, "xen_virtio_dma_alloc_pages size %ld\n", size);
>> +	return NULL;
>> +}
>> +
>> +static void xen_virtio_dma_free_pages(struct device *dev, size_t size,
>> +				      struct page *vaddr, dma_addr_t dma_handle,
>> +				      enum dma_data_direction dir)
>> +{
>> +	WARN_ONCE(1, "xen_virtio_dma_free_pages size %ld\n", size);
>> +}
>> +
>> +static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
>> +					  unsigned long offset, size_t size,
>> +					  enum dma_data_direction dir,
>> +					  unsigned long attrs)
>> +{
>> +	grant_ref_t grant;
>> +
>> +	if (gnttab_alloc_grant_references(1, &grant))
>> +		return 0;
>> +
>> +	gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
>> +					dir == DMA_TO_DEVICE);
>> +	return grant_to_dma(grant) + offset;
>> +}
>> +
>> +static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>> +				      size_t size, enum dma_data_direction dir,
>> +				      unsigned long attrs)
>> +{
>> +	grant_ref_t grant;
>> +
>> +	grant = dma_to_grant(dma_handle);
>> +
>> +	gnttab_end_foreign_access_ref(grant);
>> +
>> +	gnttab_free_grant_reference(grant);
>> +}
>> +
>> +static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
>> +				 int nents, enum dma_data_direction dir,
>> +				 unsigned long attrs)
>> +{
>> +	WARN_ONCE(1, "xen_virtio_dma_map_sg nents %d\n", nents);
>> +	return -EINVAL;
>> +}
>> +
>> +static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>> +				    int nents, enum dma_data_direction dir,
>> +				    unsigned long attrs)
>> +{
>> +	WARN_ONCE(1, "xen_virtio_dma_unmap_sg nents %d\n", nents);
>> +}
> You can implement xen_virtio_dma_map_sg and xen_virtio_dma_unmap_sg
> based on xen_virtio_dma_map_page and xen_virtio_dma_unmap_page, like we
> do in drivers/xen/swiotlb-xen.c.

Good point, thank you, will implement.


>
>
>> +static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
>> +{
>> +	return 1;
>> +}
>> +
>> +static const struct dma_map_ops xen_virtio_dma_ops = {
>> +	.alloc = xen_virtio_dma_alloc,
>> +	.free = xen_virtio_dma_free,
>> +	.alloc_pages = xen_virtio_dma_alloc_pages,
>> +	.free_pages = xen_virtio_dma_free_pages,
>> +	.mmap = dma_common_mmap,
>> +	.get_sgtable = dma_common_get_sgtable,
>> +	.map_page = xen_virtio_dma_map_page,
>> +	.unmap_page = xen_virtio_dma_unmap_page,
>> +	.map_sg = xen_virtio_dma_map_sg,
>> +	.unmap_sg = xen_virtio_dma_unmap_sg,
>> +	.dma_supported = xen_virtio_dma_dma_supported,
>> +};
>> +
>> +void xen_virtio_setup_dma_ops(struct device *dev)
>> +{
>> +	dev->dma_ops = &xen_virtio_dma_ops;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
>> +
>> +MODULE_DESCRIPTION("Xen virtio support driver");
>> +MODULE_AUTHOR("Juergen Gross <jgross@suse.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
>> index a3584a3..ae3c1bc 100644
>> --- a/include/xen/xen-ops.h
>> +++ b/include/xen/xen-ops.h
>> @@ -221,4 +221,12 @@ static inline void xen_preemptible_hcall_end(void) { }
>>   
>>   #endif /* CONFIG_XEN_PV && !CONFIG_PREEMPTION */
>>   
>> +#ifdef CONFIG_XEN_VIRTIO
>> +void xen_virtio_setup_dma_ops(struct device *dev);
>> +#else
>> +static inline void xen_virtio_setup_dma_ops(struct device *dev)
>> +{
>> +}
>> +#endif /* CONFIG_XEN_VIRTIO */
>> +
>>   #endif /* INCLUDE_XEN_OPS_H */
>> -- 
>> 2.7.4
>>
-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/6] dt-bindings: xen: Add xen,dev-domid property description for xen-virtio layer
  2022-04-15 22:01   ` Stefano Stabellini
@ 2022-04-17 17:24     ` Oleksandr
  0 siblings, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-17 17:24 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, virtualization, devicetree, linux-kernel,
	Oleksandr Tyshchenko, Michael S. Tsirkin, Jason Wang,
	Rob Herring, Krzysztof Kozlowski, Julien Grall, Juergen Gross,
	Christoph Hellwig, linux-arm-kernel


On 16.04.22 01:01, Stefano Stabellini wrote:

Hello Stefano


> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Introduce Xen specific binding for the virtio-mmio device to be used
>> by Xen virtio support driver in a subsequent commit.
>>
>> This binding specifies the ID of Xen domain where the corresponding
>> device (backend) resides. This is needed for the option to restrict
>> memory access using Xen grant mappings to work.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   .../devicetree/bindings/virtio/xen,dev-domid.yaml  | 39 ++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml b/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
>> new file mode 100644
>> index 00000000..78be993
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/virtio/xen,dev-domid.yaml
>> @@ -0,0 +1,39 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/virtio/xen,dev-domid.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Xen specific binding for the virtio device
>> +
>> +maintainers:
>> +  - Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> +
>> +select: true
>> +
>> +description:
>> +  This binding specifies the ID of Xen domain where the corresponding device
>> +  (backend) resides. This is needed for the option to restrict memory access
>> +  using Xen grant mappings to work.
>> +
>> +  Note that current and generic "iommus" bindings are mutually exclusive, since
>> +  the restricted memory access model on Xen behaves as a kind of software IOMMU.
> I don't think that this last statement is necessary or fully accurate, so
> I would remove it.


ok, will remove


> Other than that, this looks good to me.


thank you


>
>
>> +properties:
>> +  xen,dev-domid:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Should contain the ID of device's domain.
> Maybe better as:
> "The domid (domain ID) of the domain where the device (backend) is running"


ok, will change


>
>
>
>> +additionalProperties: true
>> +
>> +examples:
>> +  - |
>> +    virtio_block@3000 {
>> +            compatible = "virtio,mmio";
>> +            reg = <0x3000 0x100>;
>> +            interrupts = <41>;
>> +
>> +            /* The device is located in Xen domain with ID 1 */
>> +            xen,dev-domid = <1>;
>> +    };
>> -- 
>> 2.7.4
>>
-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer
  2022-04-15 22:02   ` Stefano Stabellini
@ 2022-04-17 18:21     ` Oleksandr
  2022-04-18 19:11       ` Stefano Stabellini
  0 siblings, 1 reply; 40+ messages in thread
From: Oleksandr @ 2022-04-17 18:21 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Russell King, Boris Ostrovsky, Juergen Gross, Julien Grall,
	Michael S. Tsirkin, Christoph Hellwig


On 16.04.22 01:02, Stefano Stabellini wrote:

Hello Stefano


> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> In the context of current patch do the following:
>> 1. Update code to support virtio-mmio devices
>> 2. Introduce struct xen_virtio_data and account passed virtio devices
>>     (using list) as we need to store some per-device data
>> 3. Add multi-page support for xen_virtio_dma_map(unmap)_page callbacks
>> 4. Harden code against malicious backend
>> 5. Change to use alloc_pages_exact() instead of __get_free_pages()
>> 6. Introduce locking scheme to protect mappings (I am not 100% sure
>>     whether per-device lock is really needed)
>> 7. Handle virtio device's DMA mask
>> 8. Retrieve the ID of backend domain from DT for virtio-mmio device
>>     instead of hardcoding it.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   arch/arm/xen/enlighten.c |  11 +++
>>   drivers/xen/Kconfig      |   2 +-
>>   drivers/xen/xen-virtio.c | 200 ++++++++++++++++++++++++++++++++++++++++++-----
>>   include/xen/xen-ops.h    |   5 ++
>>   4 files changed, 196 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> index ec5b082..870d92f 100644
>> --- a/arch/arm/xen/enlighten.c
>> +++ b/arch/arm/xen/enlighten.c
>> @@ -409,6 +409,17 @@ int __init arch_xen_unpopulated_init(struct resource **res)
>>   }
>>   #endif
>>   
>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>> +int arch_has_restricted_virtio_memory_access(void)
>> +{
>> +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>> +		return 1;
> Instead of xen_hvm_domain(), you can just use xen_domain(). Also there
> is no need for the #ifdef
> CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, given that:
>
> CONFIG_XEN_HVM_VIRTIO_GRANT depends on XEN_VIRTIO which selects
> ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS


Yes, but please see my comments in commit #2 regarding 
CONFIG_XEN_HVM_VIRTIO_GRANT option and 
arch_has_restricted_virtio_memory_access() on Arm.

I propose to have the following on Arm:

int arch_has_restricted_virtio_memory_access(void)
{
      return xen_has_restricted_virtio_memory_access();
}


where common xen.h contain a helper to be used by both Arm and x86:

static inline int xen_has_restricted_virtio_memory_access(void)
{
      if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() || 
xen_hvm_domain()))
          return 1;

      return 0;
}


But I would be happy with what you propose as well.


>
>
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>> +#endif
>> +
>>   static void __init xen_dt_guest_init(void)
>>   {
>>   	struct device_node *xen_node;
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index fc61f7a..56afe6a 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -347,7 +347,7 @@ config XEN_VIRTIO
>>   
>>   config XEN_HVM_VIRTIO_GRANT
>>   	bool "Require virtio for fully virtualized guests to use grant mappings"
>> -	depends on XEN_VIRTIO && X86_64
>> +	depends on XEN_VIRTIO && (X86_64 || ARM || ARM64)
> you can remove the architectural dependencies


According to the conversation in commit #2 we are considering just a 
single XEN_VIRTIO option, but it is going to has the
same architectural dependencies: (X86_64 || ARM || ARM64)

By removing the architectural dependencies here, we will leave also 
X86_32 covered (neither XEN_HVM_VIRTIO_GRANT nor XEN_PV_VIRTIO covered 
it). I don't know whether it is ok or not.

Shall I remove dependencies anyway?


>
>
>>   	default y
>>   	help
>>   	  Require virtio for fully virtualized guests to use grant mappings.
>> diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
>> index cfd5eda..c5b2ec9 100644
>> --- a/drivers/xen/xen-virtio.c
>> +++ b/drivers/xen/xen-virtio.c
>> @@ -7,12 +7,26 @@
>>   
>>   #include <linux/module.h>
>>   #include <linux/dma-map-ops.h>
>> +#include <linux/of.h>
>>   #include <linux/pci.h>
>>   #include <linux/pfn.h>
>>   #include <linux/virtio_config.h>
>>   #include <xen/xen.h>
>>   #include <xen/grant_table.h>
>>   
>> +struct xen_virtio_data {
>> +	/* The ID of backend domain */
>> +	domid_t dev_domid;
>> +	struct device *dev;
>> +	struct list_head list;
>> +	spinlock_t lock;
>> +	/* Is device behaving sane? */
>> +	bool broken;
> If you moved "broken" after "dev_domid" we would save a few bytes for
> every allocation due to padding.

ok, will do


>
> Is data->lock only there to protect accesses to "broken"? If so, we
> might not need it, but I am not sure.


Really good question, I introduced a lock for other purpose, I was 
thinking we needed to protect grants allocation and removing, but wasn't 
100% sure about it (I wrote a remark in commit description). But looking 
into grant_table.c again I see that grant table code uses it's own lock, 
so looks like we don't need an extra lock here. I need to re-check 
regarding "broken", but likely we don't need here as well. If so, I will 
remove the lock.


>
>
>> +};
>> +
>> +static LIST_HEAD(xen_virtio_devices);
>> +static DEFINE_SPINLOCK(xen_virtio_lock);
>> +
>>   #define XEN_GRANT_ADDR_OFF	0x8000000000000000ULL
>>   
>>   static inline dma_addr_t grant_to_dma(grant_ref_t grant)
>> @@ -25,6 +39,25 @@ static inline grant_ref_t dma_to_grant(dma_addr_t dma)
>>   	return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
>>   }
>>   
>> +static struct xen_virtio_data *find_xen_virtio_data(struct device *dev)
>> +{
>> +	struct xen_virtio_data *data = NULL;
>> +	bool found = false;
>> +
>> +	spin_lock(&xen_virtio_lock);
>> +
>> +	list_for_each_entry( data, &xen_virtio_devices, list) {
>> +		if (data->dev == dev) {
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	spin_unlock(&xen_virtio_lock);
>> +
>> +	return found ? data : NULL;
>> +}
>> +
>>   /*
>>    * DMA ops for Xen virtio frontends.
>>    *
>> @@ -43,48 +76,78 @@ static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
>>   				  dma_addr_t *dma_handle, gfp_t gfp,
>>   				  unsigned long attrs)
>>   {
>> -	unsigned int n_pages = PFN_UP(size);
>> -	unsigned int i;
>> +	struct xen_virtio_data *data;
>> +	unsigned int i, n_pages = PFN_UP(size);
>>   	unsigned long pfn;
>>   	grant_ref_t grant;
>> -	void *ret;
>> +	void *ret = NULL;
>>   
>> -	ret = (void *)__get_free_pages(gfp, get_order(size));
>> -	if (!ret)
>> +	data = find_xen_virtio_data(dev);
>> +	if (!data)
>>   		return NULL;
>>   
>> +	spin_lock(&data->lock);
>> +
>> +	if (unlikely(data->broken))
>> +		goto out;
>> +
>> +	ret = alloc_pages_exact(n_pages * PAGE_SIZE, gfp);
>> +	if (!ret)
>> +		goto out;
>> +
>>   	pfn = virt_to_pfn(ret);
>>   
>>   	if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
>> -		free_pages((unsigned long)ret, get_order(size));
>> -		return NULL;
>> +		free_pages_exact(ret, n_pages * PAGE_SIZE);
>> +		ret = NULL;
>> +		goto out;
>>   	}
>>   
>>   	for (i = 0; i < n_pages; i++) {
>> -		gnttab_grant_foreign_access_ref(grant + i, 0,
>> +		gnttab_grant_foreign_access_ref(grant + i, data->dev_domid,
>>   						pfn_to_gfn(pfn + i), 0);
>>   	}
>>   
>>   	*dma_handle = grant_to_dma(grant);
>>   
>> +out:
>> +	spin_unlock(&data->lock);
>> +
>>   	return ret;
>>   }
>>   
>>   static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
>>   				dma_addr_t dma_handle, unsigned long attrs)
>>   {
>> -	unsigned int n_pages = PFN_UP(size);
>> -	unsigned int i;
>> +	struct xen_virtio_data *data;
>> +	unsigned int i, n_pages = PFN_UP(size);
>>   	grant_ref_t grant;
>>   
>> +	data = find_xen_virtio_data(dev);
>> +	if (!data)
>> +		return;
>> +
>> +	spin_lock(&data->lock);
>> +
>> +	if (unlikely(data->broken))
>> +		goto out;
>> +
>>   	grant = dma_to_grant(dma_handle);
>>   
>> -	for (i = 0; i < n_pages; i++)
>> -		gnttab_end_foreign_access_ref(grant + i);
>> +	for (i = 0; i < n_pages; i++) {
>> +		if (unlikely(!gnttab_end_foreign_access_ref(grant + i))) {
>> +			dev_alert(dev, "Grant still in use by backend domain, disabled for further use\n");
>> +			data->broken = true;
>> +			goto out;
>> +		}
>> +	}
>>   
>>   	gnttab_free_grant_reference_seq(grant, n_pages);
>>   
>> -	free_pages((unsigned long)vaddr, get_order(size));
>> +	free_pages_exact(vaddr, n_pages * PAGE_SIZE);
>> +
>> +out:
>> +	spin_unlock(&data->lock);
>>   }
>>   
>>   static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
>> @@ -108,28 +171,71 @@ static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
>>   					  enum dma_data_direction dir,
>>   					  unsigned long attrs)
>>   {
>> +	struct xen_virtio_data *data;
>> +	unsigned int i, n_pages = PFN_UP(size);
>>   	grant_ref_t grant;
>> +	dma_addr_t dma_handle = DMA_MAPPING_ERROR;
>> +
>> +	BUG_ON(dir == DMA_NONE);
>> +
>> +	data = find_xen_virtio_data(dev);
>> +	if (!data)
>> +		return DMA_MAPPING_ERROR;
>> +
>> +	spin_lock(&data->lock);
>>   
>> -	if (gnttab_alloc_grant_references(1, &grant))
>> -		return 0;
>> +	if (unlikely(data->broken))
>> +		goto out;
>>   
>> -	gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
>> -					dir == DMA_TO_DEVICE);
>> +	if (gnttab_alloc_grant_reference_seq(n_pages, &grant))
>> +		goto out;
>>   
>> -	return grant_to_dma(grant) + offset;
>> +	for (i = 0; i < n_pages; i++) {
>> +		gnttab_grant_foreign_access_ref(grant + i, data->dev_domid,
>> +				xen_page_to_gfn(page) + i, dir == DMA_TO_DEVICE);
>> +	}
>> +
>> +	dma_handle = grant_to_dma(grant) + offset;
>> +
>> +out:
>> +	spin_unlock(&data->lock);
>> +
>> +	return dma_handle;
>>   }
>>   
>>   static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>>   				      size_t size, enum dma_data_direction dir,
>>   				      unsigned long attrs)
>>   {
>> +	struct xen_virtio_data *data;
>> +	unsigned int i, n_pages = PFN_UP(size);
>>   	grant_ref_t grant;
>>   
>> +	BUG_ON(dir == DMA_NONE);
>> +
>> +	data = find_xen_virtio_data(dev);
>> +	if (!data)
>> +		return;
>> +
>> +	spin_lock(&data->lock);
>> +
>> +	if (unlikely(data->broken))
>> +		goto out;
>> +
>>   	grant = dma_to_grant(dma_handle);
>>   
>> -	gnttab_end_foreign_access_ref(grant);
>> +	for (i = 0; i < n_pages; i++) {
>> +		if (unlikely(!gnttab_end_foreign_access_ref(grant + i))) {
>> +			dev_alert(dev, "Grant still in use by backend domain, disabled for further use\n");
>> +			data->broken = true;
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	gnttab_free_grant_reference_seq(grant, n_pages);
>>   
>> -	gnttab_free_grant_reference(grant);
>> +out:
>> +	spin_unlock(&data->lock);
>>   }
>>   
>>   static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
>> @@ -149,7 +255,7 @@ static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>>   
>>   static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
>>   {
>> -	return 1;
>> +	return mask == DMA_BIT_MASK(64);
>>   }
>>   
>>   static const struct dma_map_ops xen_virtio_dma_ops = {
>> @@ -166,9 +272,61 @@ static const struct dma_map_ops xen_virtio_dma_ops = {
>>   	.dma_supported = xen_virtio_dma_dma_supported,
>>   };
>>   
>> +bool xen_is_virtio_device(struct device *dev)
>> +{
>> +	/* XXX Handle only DT devices for now */
>> +	if (!dev->of_node)
>> +		return false;
>> +
>> +	if (!of_device_is_compatible(dev->of_node, "virtio,mmio"))
>> +		return false;
>> +
>> +	return of_property_read_bool(dev->of_node, "xen,dev-domid");
>> +}
>> +EXPORT_SYMBOL_GPL(xen_is_virtio_device);
>> +
>>   void xen_virtio_setup_dma_ops(struct device *dev)
>>   {
>> +	struct xen_virtio_data *data;
>> +	uint32_t dev_domid;
>> +
>> +	data = find_xen_virtio_data(dev);
>> +	if (data) {
>> +		dev_err(dev, "xen_virtio data is already created\n");
>> +		return;
>> +	}
>> +
>> +	if (dev_is_pci(dev)) {
>> +		/* XXX Leave it hard wired to dom0 for now */
>> +		dev_domid = 0;
>> +	} else if (dev->of_node) {
>> +		if (of_property_read_u32(dev->of_node, "xen,dev-domid", &dev_domid)) {
>> +			dev_err(dev, "xen,dev-domid property is not present\n");
>> +			goto err;
>> +		}
>> +	} else
>> +		/* The ACPI case is not supported */
>> +		goto err;
> If we get here, it means that xen_is_virtio_device returned true, so the
> PCI case is actually impossible?

Good catch, thank you. Yes, it is impossible on Arm for now (with 
changes in commit #6).


>
> I would rewrite these checks like this:
>
> /* XXX: ACPI and PCI unsupported for now */
> if (dev_is_pci(dev) || !dev->of_node) {
> 	goto err;
> }
> if (of_property_read_u32(dev->of_node, "xen,dev-domid", &dev_domid)) {
> 	dev_err(dev, "xen,dev-domid property is not present\n");
> 	goto err;
> }


ok, will do


>
>
>
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data) {
>> +		dev_err(dev, "Сannot allocate xen_virtio data\n");
>> +		goto err;
>> +	}
>> +	data->dev_domid = dev_domid;
>> +	data->dev = dev;
>> +	spin_lock_init(&data->lock);
>> +
>> +	spin_lock(&xen_virtio_lock);
>> +	list_add(&data->list, &xen_virtio_devices);
>> +	spin_unlock(&xen_virtio_lock);
>> +
>>   	dev->dma_ops = &xen_virtio_dma_ops;
>> +
>> +	return;
>> +
>> +err:
>> +	dev_err(dev, "Сannot set up xen_virtio DMA ops, retain platform DMA ops\n");
>>   }
>>   EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
>>   
>> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
>> index ae3c1bc..fdbcb99 100644
>> --- a/include/xen/xen-ops.h
>> +++ b/include/xen/xen-ops.h
>> @@ -223,10 +223,15 @@ static inline void xen_preemptible_hcall_end(void) { }
>>   
>>   #ifdef CONFIG_XEN_VIRTIO
>>   void xen_virtio_setup_dma_ops(struct device *dev);
>> +bool xen_is_virtio_device(struct device *dev);
>>   #else
>>   static inline void xen_virtio_setup_dma_ops(struct device *dev)
>>   {
>>   }
>> +static inline bool xen_is_virtio_device(struct device *dev)
>> +{
>> +	return false;
>> +}
>>   #endif /* CONFIG_XEN_VIRTIO */
>>   
>>   #endif /* INCLUDE_XEN_OPS_H */

-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer
  2022-04-16  6:05   ` Christoph Hellwig
@ 2022-04-17 18:39     ` Oleksandr
  0 siblings, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-17 18:39 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Stefano Stabellini, Russell King, Boris Ostrovsky, Juergen Gross,
	Julien Grall, Michael S. Tsirkin


On 16.04.22 09:05, Christoph Hellwig wrote:

Hello Christoph

> On Thu, Apr 14, 2022 at 10:19:31PM +0300, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Various updates is a big indicator that the patch should be split
> further.  Please do one change at a time, and fold updates to the
> previous patches in the series into those patches instead of fixing
> them up later.


Sure, next (non-RFC) version will do things properly.


-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 5/6] arm/xen: Introduce xen_setup_dma_ops()
  2022-04-15 22:02   ` Stefano Stabellini
@ 2022-04-17 18:43     ` Oleksandr
  0 siblings, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-17 18:43 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Russell King, Catalin Marinas, Will Deacon, Boris Ostrovsky,
	Juergen Gross, Logan Gunthorpe, David Hildenbrand,
	Martin Oliveira, Kees Cook, Jean-Philippe Brucker, Julien Grall,
	Christoph Hellwig, Michael S. Tsirkin


On 16.04.22 01:02, Stefano Stabellini wrote:

Hello Stefano

> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This patch introduces new helper and places it in new header.
>> The helper's purpose is to assign any Xen specific DMA ops in
>> a single place. For now, we deal with xen-swiotlb DMA ops only.
>> The subsequent patch will add xen-virtio DMA ops case.
>>
>> Also re-use the xen_swiotlb_detect() check on Arm32.
> Thanks for the patch, this is good to have in any case. I would move it
> to the beginning of the series.


ok, will move


>
>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   arch/arm/include/asm/xen/xen-ops.h   |  1 +
>>   arch/arm/mm/dma-mapping.c            |  5 ++---
>>   arch/arm64/include/asm/xen/xen-ops.h |  1 +
>>   arch/arm64/mm/dma-mapping.c          |  5 ++---
>>   include/xen/arm/xen-ops.h            | 13 +++++++++++++
>>   5 files changed, 19 insertions(+), 6 deletions(-)
>>   create mode 100644 arch/arm/include/asm/xen/xen-ops.h
>>   create mode 100644 arch/arm64/include/asm/xen/xen-ops.h
>>   create mode 100644 include/xen/arm/xen-ops.h
>>
>> diff --git a/arch/arm/include/asm/xen/xen-ops.h b/arch/arm/include/asm/xen/xen-ops.h
>> new file mode 100644
>> index 00000000..8d2fa24
>> --- /dev/null
>> +++ b/arch/arm/include/asm/xen/xen-ops.h
>> @@ -0,0 +1 @@
>> +#include <xen/arm/xen-ops.h>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index 82ffac6..a1bf9dd 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -33,7 +33,7 @@
>>   #include <asm/dma-iommu.h>
>>   #include <asm/mach/map.h>
>>   #include <asm/system_info.h>
>> -#include <xen/swiotlb-xen.h>
>> +#include <asm/xen/xen-ops.h>
>>   
>>   #include "dma.h"
>>   #include "mm.h"
>> @@ -2288,8 +2288,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>   	set_dma_ops(dev, dma_ops);
>>   
>>   #ifdef CONFIG_XEN
>> -	if (xen_initial_domain())
>> -		dev->dma_ops = &xen_swiotlb_dma_ops;
>> +	xen_setup_dma_ops(dev);
>>   #endif
> You can take this opportunity to also remove the #ifdef, by providing an
> empty stub implemention of xen_setup_dma_ops for the !CONFIG_XEN case.


agree, will do


>
>
>>   	dev->archdata.dma_ops_setup = true;
>>   }
>> diff --git a/arch/arm64/include/asm/xen/xen-ops.h b/arch/arm64/include/asm/xen/xen-ops.h
>> new file mode 100644
>> index 00000000..8d2fa24
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/xen/xen-ops.h
>> @@ -0,0 +1 @@
>> +#include <xen/arm/xen-ops.h>
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index 6719f9e..831e673 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -9,9 +9,9 @@
>>   #include <linux/dma-map-ops.h>
>>   #include <linux/dma-iommu.h>
>>   #include <xen/xen.h>
>> -#include <xen/swiotlb-xen.h>
>>   
>>   #include <asm/cacheflush.h>
>> +#include <asm/xen/xen-ops.h>
>>   
>>   void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>>   		enum dma_data_direction dir)
>> @@ -53,7 +53,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>   		iommu_setup_dma_ops(dev, dma_base, dma_base + size - 1);
>>   
>>   #ifdef CONFIG_XEN
>> -	if (xen_swiotlb_detect())
>> -		dev->dma_ops = &xen_swiotlb_dma_ops;
>> +	xen_setup_dma_ops(dev);
>>   #endif
> same here

ok


>
>
>>   }
>> diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h
>> new file mode 100644
>> index 00000000..621da05
>> --- /dev/null
>> +++ b/include/xen/arm/xen-ops.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_ARM_XEN_OPS_H
>> +#define _ASM_ARM_XEN_OPS_H
>> +
>> +#include <xen/swiotlb-xen.h>
>> +
>> +static inline void xen_setup_dma_ops(struct device *dev)
>> +{
>> +	if (xen_swiotlb_detect())
>> +		dev->dma_ops = &xen_swiotlb_dma_ops;
>> +}
>> +
>> +#endif /* _ASM_ARM_XEN_OPS_H */
>> -- 
>> 2.7.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-15 22:02   ` Stefano Stabellini
  2022-04-16  6:07     ` Christoph Hellwig
@ 2022-04-17 19:20     ` Oleksandr
  1 sibling, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-17 19:20 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Boris Ostrovsky, Juergen Gross, Julien Grall, Michael S. Tsirkin,
	Christoph Hellwig


On 16.04.22 01:02, Stefano Stabellini wrote:


Hello Stefano

> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Call xen_virtio_setup_dma_ops() only for Xen-aware virtio devices
>> in Xen guests if restricted access to the guest memory is enabled.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   include/xen/arm/xen-ops.h | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h
>> index 621da05..28b2ad3 100644
>> --- a/include/xen/arm/xen-ops.h
>> +++ b/include/xen/arm/xen-ops.h
>> @@ -2,12 +2,19 @@
>>   #ifndef _ASM_ARM_XEN_OPS_H
>>   #define _ASM_ARM_XEN_OPS_H
>>   
>> +#include <linux/virtio_config.h>
>>   #include <xen/swiotlb-xen.h>
>> +#include <xen/xen-ops.h>
>>   
>>   static inline void xen_setup_dma_ops(struct device *dev)
>>   {
>>   	if (xen_swiotlb_detect())
>>   		dev->dma_ops = &xen_swiotlb_dma_ops;
>> +
>> +#ifdef CONFIG_XEN_VIRTIO
>> +	if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev))
>> +		xen_virtio_setup_dma_ops(dev);
>> +#endif
> This makes sense overall.


thank you


> Considering that the swiotlb-xen case and the
> virtio case are mutually exclusive, I would write it like this:
>
> 	if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev))
> 		xen_virtio_setup_dma_ops(dev);
> 	else if (xen_swiotlb_detect())
> 		dev->dma_ops = &xen_swiotlb_dma_ops;


Agree, will do


>
> There is no need for #ifdef (also see other comments).


Agree, if !CONFIG_XEN_VIRTIO then xen_virtio_ are just stubs.


#ifdef CONFIG_XEN_VIRTIO
void xen_virtio_setup_dma_ops(struct device *dev);
bool xen_is_virtio_device(struct device *dev);
#else
static inline void xen_virtio_setup_dma_ops(struct device *dev)
{
}
static inline bool xen_is_virtio_device(struct device *dev)
{
     return false;
}
#endif /* CONFIG_XEN_VIRTIO */



-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-16  6:07     ` Christoph Hellwig
@ 2022-04-17 21:05       ` Oleksandr
  2022-04-18 19:11         ` Stefano Stabellini
  0 siblings, 1 reply; 40+ messages in thread
From: Oleksandr @ 2022-04-17 21:05 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Stefano Stabellini, xen-devel, linux-kernel, linux-arm-kernel,
	Oleksandr Tyshchenko, Boris Ostrovsky, Juergen Gross,
	Julien Grall, Michael S. Tsirkin


On 16.04.22 09:07, Christoph Hellwig wrote:

Hello Christoph

> On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
>> This makes sense overall. Considering that the swiotlb-xen case and the
>> virtio case are mutually exclusive, I would write it like this:
> Curious question:  Why can't the same grant scheme also be used for
> non-virtio devices?  I really hate having virtio hooks in the arch
> dma code.  Why can't Xen just say in DT/ACPI that grants can be used
> for a given device?


In Xen system:
- the grants are not used for "non-virtualized" devices at all (platform 
devices for the passthrough).
- the grants are widely used for "virtualized, but non-virtio" devices 
(traditional Xen PV devices), but grants for these Xen PV devices
are used in a different way and *not* at the DMA ops level like in 
current approach

Or I misunderstood your question?

This patch series tries to make things work with "virtio" devices in Xen 
system without introducing any modifications to code under drivers/virtio.
We could avoid having virtio hooks in the arch DMA code, but we need to 
trigger setting xen-virtio DMA ops for the virtio device from some other 
place.
For example, the following code would also work, but requires altering 
virtio_mmio_probe():

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 56128b9..8f48491 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -615,6 +615,9 @@ static int virtio_mmio_probe(struct platform_device 
*pdev)
                                               DMA_BIT_MASK(32 + 
PAGE_SHIFT));
         } else {
                 rc = dma_set_mask_and_coherent(&pdev->dev, 
DMA_BIT_MASK(64));
+
+               if (arch_has_restricted_virtio_memory_access())
+ xen_virtio_setup_dma_ops(&pdev->dev);
         }
         if (rc)
                 rc = dma_set_mask_and_coherent(&pdev->dev, 
DMA_BIT_MASK(32));


Another possible option could be to introduce local init function in 
drivers/xen/xen-virtio.c to scan the device tree and set xen-virtio DMA 
ops for all devices with the
"xen,dev-domid" property.


What do you think?

-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-17 21:05       ` Oleksandr
@ 2022-04-18 19:11         ` Stefano Stabellini
  2022-04-19 12:17           ` Oleksandr
  0 siblings, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-18 19:11 UTC (permalink / raw)
  To: Oleksandr
  Cc: Christoph Hellwig, Stefano Stabellini, xen-devel, linux-kernel,
	linux-arm-kernel, Oleksandr Tyshchenko, Boris Ostrovsky,
	Juergen Gross, Julien Grall, Michael S. Tsirkin

On Mon, 18 Apr 2022, Oleksandr wrote:
> On 16.04.22 09:07, Christoph Hellwig wrote:
> 
> Hello Christoph
> 
> > On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
> > > This makes sense overall. Considering that the swiotlb-xen case and the
> > > virtio case are mutually exclusive, I would write it like this:
> > Curious question:  Why can't the same grant scheme also be used for
> > non-virtio devices?  I really hate having virtio hooks in the arch
> > dma code.  Why can't Xen just say in DT/ACPI that grants can be used
> > for a given device?

[...]

> This patch series tries to make things work with "virtio" devices in Xen
> system without introducing any modifications to code under drivers/virtio.


Actually, I think Christoph has a point.

There is nothing inherently virtio specific in this patch series or in
the "xen,dev-domid" device tree binding. Assuming a given device is
emulated by a Xen backend, it could be used with grants as well.

For instance, we could provide an emulated e1000 NIC with a
"xen,dev-domid" property in device tree. Linux could use grants with it
and the backend could map the grants. It would work the same way as
virtio-net/block/etc. Passthrough devices wouldn't have the
"xen,dev-domid" property, so no problems.

So I think we could easily generalize this work and expand it to any
device. We just need to hook on the "xen,dev-domid" device tree
property.

I think it is just a matter of:
- remove the "virtio,mmio" check from xen_is_virtio_device
- rename xen_is_virtio_device to something more generic, like
  xen_is_grants_device
- rename xen_virtio_setup_dma_ops to something more generic, like
  xen_grants_setup_dma_ops

And that's pretty much it.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen
  2022-04-17 17:02     ` Oleksandr
@ 2022-04-18 19:11       ` Stefano Stabellini
  2022-04-19  6:21         ` Juergen Gross
  0 siblings, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-18 19:11 UTC (permalink / raw)
  To: Oleksandr
  Cc: Stefano Stabellini, xen-devel, x86, linux-kernel, Juergen Gross,
	Dave Hansen, Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Boris Ostrovsky,
	Julien Grall, Oleksandr Tyshchenko, linux-arm-kernel,
	Christoph Hellwig, Michael S. Tsirkin

[-- Attachment #1: Type: text/plain, Size: 7896 bytes --]

On Sun, 17 Apr 2022, Oleksandr wrote:
> On 16.04.22 01:01, Stefano Stabellini wrote:
> > On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> > > From: Juergen Gross <jgross@suse.com>
> > > 
> > > In order to support virtio in Xen guests add a config option enabling
> > > the user to specify whether in all Xen guests virtio should be able to
> > > access memory via Xen grant mappings only on the host side.
> > > 
> > > This applies to fully virtualized guests only, as for paravirtualized
> > > guests this is mandatory.
> > > 
> > > This requires to switch arch_has_restricted_virtio_memory_access()
> > > from a pure stub to a real function on x86 systems (Arm systems are
> > > not covered by now).
> > > 
> > > Add the needed functionality by providing a special set of DMA ops
> > > handling the needed grant operations for the I/O pages.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > ---
> > >   arch/x86/mm/init.c        |  15 ++++
> > >   arch/x86/mm/mem_encrypt.c |   5 --
> > >   arch/x86/xen/Kconfig      |   9 +++
> > >   drivers/xen/Kconfig       |  20 ++++++
> > >   drivers/xen/Makefile      |   1 +
> > >   drivers/xen/xen-virtio.c  | 177
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >   include/xen/xen-ops.h     |   8 +++
> > >   7 files changed, 230 insertions(+), 5 deletions(-)
> > >   create mode 100644 drivers/xen/xen-virtio.c
> > > 
> > > diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> > > index d8cfce2..526a3b2 100644
> > > --- a/arch/x86/mm/init.c
> > > +++ b/arch/x86/mm/init.c
> > > @@ -8,6 +8,8 @@
> > >   #include <linux/kmemleak.h>
> > >   #include <linux/sched/task.h>
> > >   +#include <xen/xen.h>
> > > +
> > >   #include <asm/set_memory.h>
> > >   #include <asm/e820/api.h>
> > >   #include <asm/init.h>
> > > @@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
> > >   	return pages;
> > >   }
> > >   #endif
> > > +
> > > +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> > > +int arch_has_restricted_virtio_memory_access(void)
> > > +{
> > > +	if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
> > > +		return 1;
> > > +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
> > > +		return 1;
> > I think these two checks could be moved to a separate function in a Xen
> > header, e.g. xen_restricted_virtio_memory_access, and here you could
> > just
> > 
> > if (xen_restricted_virtio_memory_access())
> >      return 1;
> 
> Agree, will do
> 
> 
> > 
> > 
> > 
> > > +	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
> > > +}
> > > +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
> > > +#endif
> > > diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> > > index 50d2099..dda020f 100644
> > > --- a/arch/x86/mm/mem_encrypt.c
> > > +++ b/arch/x86/mm/mem_encrypt.c
> > > @@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
> > >   	print_mem_encrypt_feature_info();
> > >   }
> > >   -int arch_has_restricted_virtio_memory_access(void)
> > > -{
> > > -	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
> > > -}
> > > -EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
> > > diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> > > index 85246dd..dffdffd 100644
> > > --- a/arch/x86/xen/Kconfig
> > > +++ b/arch/x86/xen/Kconfig
> > > @@ -92,3 +92,12 @@ config XEN_DOM0
> > >   	select X86_X2APIC if XEN_PVH && X86_64
> > >   	help
> > >   	  Support running as a Xen Dom0 guest.
> > > +
> > > +config XEN_PV_VIRTIO
> > > +	bool "Xen virtio support for PV guests"
> > > +	depends on XEN_VIRTIO && XEN_PV
> > > +	default y
> > > +	help
> > > +	  Support virtio for running as a paravirtualized guest. This will
> > > +	  need support on the backend side (qemu or kernel, depending on the
> > > +	  virtio device types used).
> > > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > > index 120d32f..fc61f7a 100644
> > > --- a/drivers/xen/Kconfig
> > > +++ b/drivers/xen/Kconfig
> > > @@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
> > >   	  having to balloon out RAM regions in order to obtain physical memory
> > >   	  space to create such mappings.
> > >   +config XEN_VIRTIO
> > > +	bool "Xen virtio support"
> > > +	default n
> > > +	depends on VIRTIO && DMA_OPS
> > > +	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> > > +	help
> > > +	  Enable virtio support for running as Xen guest. Depending on the
> > > +	  guest type this will require special support on the backend side
> > > +	  (qemu or kernel, depending on the virtio device types used).
> > > +
> > > +config XEN_HVM_VIRTIO_GRANT
> > > +	bool "Require virtio for fully virtualized guests to use grant
> > > mappings"
> > > +	depends on XEN_VIRTIO && X86_64
> > > +	default y
> > > +	help
> > > +	  Require virtio for fully virtualized guests to use grant mappings.
> > > +	  This will avoid the need to give the backend the right to map all
> > > +	  of the guest memory. This will need support on the backend side
> > > +	  (qemu or kernel, depending on the virtio device types used).
> > I don't think we need 3 visible kconfig options for this.
> > 
> > In fact, I would only add one: XEN_VIRTIO. We can have any X86 (or ARM)
> > specific dependencies in the "depends" line under XEN_VIRTIO. And I
> > don't think we need XEN_HVM_VIRTIO_GRANT as a kconfig option
> > necessarely. It doesn't seem like some we want as build time option. At
> > most, it could be a runtime option (like a command line) or a debug
> > option (like an #define at the top of the source file.)
> 
> 
> I don't know what was the initial idea of having and extra XEN_HVM_VIRTIO and
> XEN_PV_VIRTIO options, but taking into the account that
> they are only used in arch_has_restricted_virtio_memory_access() currently, I
> share your opinion regarding a single XEN_VIRTIO option.
> 
> Looking ahead (including changes in the commit #4), we can imagine the
> resulting option:
> 
> config XEN_VIRTIO
>     bool "Xen virtio support"
>     default n
>     depends on VIRTIO && DMA_OPS
>     depends on (X86_64 || ARM || ARM64)
>     select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>     help
>       Enable virtio support for running as Xen guest. Depending on the
>       guest type this will require special support on the backend side
>       (qemu or kernel, depending on the virtio device types used).
> 
> 
> and then arch_has_restricted_virtio_memory_access() per arch:
> 
> 
> 1. x86:
> 
> int arch_has_restricted_virtio_memory_access(void)
> {
>     return (xen_has_restricted_virtio_memory_access() ||
>             cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
> }
> 
> 
> 2. Arm:
> 
> int arch_has_restricted_virtio_memory_access(void)
> {
>     return xen_has_restricted_virtio_memory_access();
> }
> 
> 
> 3. xen.h:
> 
> static inline int xen_has_restricted_virtio_memory_access(void)
> {
>     if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() ||
> xen_hvm_domain()))
>         return 1;
> 
>     return 0;
> }
> 
> 
> Actually, as domain type on Arm is always XEN_HVM_DOMAIN, we could probably
> have the following on Arm:
> 
> int arch_has_restricted_virtio_memory_access(void)
> {
>     return IS_ENABLED(CONFIG_XEN_VIRTIO);
> }
> 
> but I would prefer not to diverge and use common
> xen_has_restricted_virtio_memory_access().
> 
> Any thoughts?

Yes, I would also prefer not to diverge between the x86 and arm versions
of xen_has_restricted_virtio_memory_access. But what case are we trying
to catch with (xen_pv_domain() || xen_hvm_domain()) ? Even on x86, it is
not going to leave much out. Is it really meant only to exclude pvh
domains?

I have the feeling that we could turn this check into:

static inline int xen_has_restricted_virtio_memory_access(void)
{
    return IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain();
}

even on x86, but one of the xen/x86 maintainers should confirm.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer
  2022-04-17 18:21     ` Oleksandr
@ 2022-04-18 19:11       ` Stefano Stabellini
  2022-04-19  6:58         ` Juergen Gross
  0 siblings, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-18 19:11 UTC (permalink / raw)
  To: Oleksandr
  Cc: Stefano Stabellini, xen-devel, linux-kernel, linux-arm-kernel,
	Oleksandr Tyshchenko, Russell King, Boris Ostrovsky,
	Juergen Gross, Julien Grall, Michael S. Tsirkin,
	Christoph Hellwig

[-- Attachment #1: Type: text/plain, Size: 4444 bytes --]

On Sun, 17 Apr 2022, Oleksandr wrote:
> On 16.04.22 01:02, Stefano Stabellini wrote:
> > On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > 
> > > In the context of current patch do the following:
> > > 1. Update code to support virtio-mmio devices
> > > 2. Introduce struct xen_virtio_data and account passed virtio devices
> > >     (using list) as we need to store some per-device data
> > > 3. Add multi-page support for xen_virtio_dma_map(unmap)_page callbacks
> > > 4. Harden code against malicious backend
> > > 5. Change to use alloc_pages_exact() instead of __get_free_pages()
> > > 6. Introduce locking scheme to protect mappings (I am not 100% sure
> > >     whether per-device lock is really needed)
> > > 7. Handle virtio device's DMA mask
> > > 8. Retrieve the ID of backend domain from DT for virtio-mmio device
> > >     instead of hardcoding it.
> > > 
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > ---
> > >   arch/arm/xen/enlighten.c |  11 +++
> > >   drivers/xen/Kconfig      |   2 +-
> > >   drivers/xen/xen-virtio.c | 200
> > > ++++++++++++++++++++++++++++++++++++++++++-----
> > >   include/xen/xen-ops.h    |   5 ++
> > >   4 files changed, 196 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > > index ec5b082..870d92f 100644
> > > --- a/arch/arm/xen/enlighten.c
> > > +++ b/arch/arm/xen/enlighten.c
> > > @@ -409,6 +409,17 @@ int __init arch_xen_unpopulated_init(struct resource
> > > **res)
> > >   }
> > >   #endif
> > >   +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> > > +int arch_has_restricted_virtio_memory_access(void)
> > > +{
> > > +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
> > > +		return 1;
> > Instead of xen_hvm_domain(), you can just use xen_domain(). Also there
> > is no need for the #ifdef
> > CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, given that:
> > 
> > CONFIG_XEN_HVM_VIRTIO_GRANT depends on XEN_VIRTIO which selects
> > ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> 
> 
> Yes, but please see my comments in commit #2 regarding
> CONFIG_XEN_HVM_VIRTIO_GRANT option and
> arch_has_restricted_virtio_memory_access() on Arm.
> 
> I propose to have the following on Arm:
> 
> int arch_has_restricted_virtio_memory_access(void)
> {
>      return xen_has_restricted_virtio_memory_access();
> }
> 
> 
> where common xen.h contain a helper to be used by both Arm and x86:
> 
> static inline int xen_has_restricted_virtio_memory_access(void)
> {
>      if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() ||
> xen_hvm_domain()))
>          return 1;
> 
>      return 0;
> }
> 
> 
> But I would be happy with what you propose as well.

As I wrote in the previous reply, I also prefer to share the code
between x86 and ARM, and I think it could look like:

int arch_has_restricted_virtio_memory_access(void)
{
     return xen_has_restricted_virtio_memory_access();
}
[...]
static inline int xen_has_restricted_virtio_memory_access(void)
{
     return (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain());
}

But let's check with Juergen and Boris.


> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
> > > +#endif
> > > +
> > >   static void __init xen_dt_guest_init(void)
> > >   {
> > >   	struct device_node *xen_node;
> > > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > > index fc61f7a..56afe6a 100644
> > > --- a/drivers/xen/Kconfig
> > > +++ b/drivers/xen/Kconfig
> > > @@ -347,7 +347,7 @@ config XEN_VIRTIO
> > >     config XEN_HVM_VIRTIO_GRANT
> > >   	bool "Require virtio for fully virtualized guests to use grant
> > > mappings"
> > > -	depends on XEN_VIRTIO && X86_64
> > > +	depends on XEN_VIRTIO && (X86_64 || ARM || ARM64)
> > you can remove the architectural dependencies
> 
> 
> According to the conversation in commit #2 we are considering just a single
> XEN_VIRTIO option, but it is going to has the
> same architectural dependencies: (X86_64 || ARM || ARM64)
> 
> By removing the architectural dependencies here, we will leave also X86_32
> covered (neither XEN_HVM_VIRTIO_GRANT nor XEN_PV_VIRTIO covered it). I don't
> know whether it is ok or not.
> 
> Shall I remove dependencies anyway?

No, good point. I don't know about X86_32. This is another detail where
Juergen or Boris should comment.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen
  2022-04-18 19:11       ` Stefano Stabellini
@ 2022-04-19  6:21         ` Juergen Gross
  2022-04-19  6:37           ` Oleksandr
  0 siblings, 1 reply; 40+ messages in thread
From: Juergen Gross @ 2022-04-19  6:21 UTC (permalink / raw)
  To: Stefano Stabellini, Oleksandr
  Cc: xen-devel, x86, linux-kernel, Dave Hansen, Andy Lutomirski,
	Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, Boris Ostrovsky, Julien Grall,
	Oleksandr Tyshchenko, linux-arm-kernel, Christoph Hellwig,
	Michael S. Tsirkin


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

On 18.04.22 21:11, Stefano Stabellini wrote:
> On Sun, 17 Apr 2022, Oleksandr wrote:
>> On 16.04.22 01:01, Stefano Stabellini wrote:
>>> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>>>> From: Juergen Gross <jgross@suse.com>
>>>>
>>>> In order to support virtio in Xen guests add a config option enabling
>>>> the user to specify whether in all Xen guests virtio should be able to
>>>> access memory via Xen grant mappings only on the host side.
>>>>
>>>> This applies to fully virtualized guests only, as for paravirtualized
>>>> guests this is mandatory.
>>>>
>>>> This requires to switch arch_has_restricted_virtio_memory_access()
>>>> from a pure stub to a real function on x86 systems (Arm systems are
>>>> not covered by now).
>>>>
>>>> Add the needed functionality by providing a special set of DMA ops
>>>> handling the needed grant operations for the I/O pages.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>    arch/x86/mm/init.c        |  15 ++++
>>>>    arch/x86/mm/mem_encrypt.c |   5 --
>>>>    arch/x86/xen/Kconfig      |   9 +++
>>>>    drivers/xen/Kconfig       |  20 ++++++
>>>>    drivers/xen/Makefile      |   1 +
>>>>    drivers/xen/xen-virtio.c  | 177
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/xen/xen-ops.h     |   8 +++
>>>>    7 files changed, 230 insertions(+), 5 deletions(-)
>>>>    create mode 100644 drivers/xen/xen-virtio.c
>>>>
>>>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>>>> index d8cfce2..526a3b2 100644
>>>> --- a/arch/x86/mm/init.c
>>>> +++ b/arch/x86/mm/init.c
>>>> @@ -8,6 +8,8 @@
>>>>    #include <linux/kmemleak.h>
>>>>    #include <linux/sched/task.h>
>>>>    +#include <xen/xen.h>
>>>> +
>>>>    #include <asm/set_memory.h>
>>>>    #include <asm/e820/api.h>
>>>>    #include <asm/init.h>
>>>> @@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
>>>>    	return pages;
>>>>    }
>>>>    #endif
>>>> +
>>>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>> +int arch_has_restricted_virtio_memory_access(void)
>>>> +{
>>>> +	if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
>>>> +		return 1;
>>>> +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>>>> +		return 1;
>>> I think these two checks could be moved to a separate function in a Xen
>>> header, e.g. xen_restricted_virtio_memory_access, and here you could
>>> just
>>>
>>> if (xen_restricted_virtio_memory_access())
>>>       return 1;
>>
>> Agree, will do
>>
>>
>>>
>>>
>>>
>>>> +	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>>>> +#endif
>>>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>>>> index 50d2099..dda020f 100644
>>>> --- a/arch/x86/mm/mem_encrypt.c
>>>> +++ b/arch/x86/mm/mem_encrypt.c
>>>> @@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
>>>>    	print_mem_encrypt_feature_info();
>>>>    }
>>>>    -int arch_has_restricted_virtio_memory_access(void)
>>>> -{
>>>> -	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>>>> -}
>>>> -EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>>>> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
>>>> index 85246dd..dffdffd 100644
>>>> --- a/arch/x86/xen/Kconfig
>>>> +++ b/arch/x86/xen/Kconfig
>>>> @@ -92,3 +92,12 @@ config XEN_DOM0
>>>>    	select X86_X2APIC if XEN_PVH && X86_64
>>>>    	help
>>>>    	  Support running as a Xen Dom0 guest.
>>>> +
>>>> +config XEN_PV_VIRTIO
>>>> +	bool "Xen virtio support for PV guests"
>>>> +	depends on XEN_VIRTIO && XEN_PV
>>>> +	default y
>>>> +	help
>>>> +	  Support virtio for running as a paravirtualized guest. This will
>>>> +	  need support on the backend side (qemu or kernel, depending on the
>>>> +	  virtio device types used).
>>>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>>>> index 120d32f..fc61f7a 100644
>>>> --- a/drivers/xen/Kconfig
>>>> +++ b/drivers/xen/Kconfig
>>>> @@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
>>>>    	  having to balloon out RAM regions in order to obtain physical memory
>>>>    	  space to create such mappings.
>>>>    +config XEN_VIRTIO
>>>> +	bool "Xen virtio support"
>>>> +	default n
>>>> +	depends on VIRTIO && DMA_OPS
>>>> +	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>> +	help
>>>> +	  Enable virtio support for running as Xen guest. Depending on the
>>>> +	  guest type this will require special support on the backend side
>>>> +	  (qemu or kernel, depending on the virtio device types used).
>>>> +
>>>> +config XEN_HVM_VIRTIO_GRANT
>>>> +	bool "Require virtio for fully virtualized guests to use grant
>>>> mappings"
>>>> +	depends on XEN_VIRTIO && X86_64
>>>> +	default y
>>>> +	help
>>>> +	  Require virtio for fully virtualized guests to use grant mappings.
>>>> +	  This will avoid the need to give the backend the right to map all
>>>> +	  of the guest memory. This will need support on the backend side
>>>> +	  (qemu or kernel, depending on the virtio device types used).
>>> I don't think we need 3 visible kconfig options for this.
>>>
>>> In fact, I would only add one: XEN_VIRTIO. We can have any X86 (or ARM)
>>> specific dependencies in the "depends" line under XEN_VIRTIO. And I
>>> don't think we need XEN_HVM_VIRTIO_GRANT as a kconfig option
>>> necessarely. It doesn't seem like some we want as build time option. At
>>> most, it could be a runtime option (like a command line) or a debug
>>> option (like an #define at the top of the source file.)
>>
>>
>> I don't know what was the initial idea of having and extra XEN_HVM_VIRTIO and
>> XEN_PV_VIRTIO options, but taking into the account that
>> they are only used in arch_has_restricted_virtio_memory_access() currently, I
>> share your opinion regarding a single XEN_VIRTIO option.
>>
>> Looking ahead (including changes in the commit #4), we can imagine the
>> resulting option:
>>
>> config XEN_VIRTIO
>>      bool "Xen virtio support"
>>      default n
>>      depends on VIRTIO && DMA_OPS
>>      depends on (X86_64 || ARM || ARM64)
>>      select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>      help
>>        Enable virtio support for running as Xen guest. Depending on the
>>        guest type this will require special support on the backend side
>>        (qemu or kernel, depending on the virtio device types used).
>>
>>
>> and then arch_has_restricted_virtio_memory_access() per arch:
>>
>>
>> 1. x86:
>>
>> int arch_has_restricted_virtio_memory_access(void)
>> {
>>      return (xen_has_restricted_virtio_memory_access() ||
>>              cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
>> }
>>
>>
>> 2. Arm:
>>
>> int arch_has_restricted_virtio_memory_access(void)
>> {
>>      return xen_has_restricted_virtio_memory_access();
>> }
>>
>>
>> 3. xen.h:
>>
>> static inline int xen_has_restricted_virtio_memory_access(void)
>> {
>>      if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() ||
>> xen_hvm_domain()))
>>          return 1;
>>
>>      return 0;
>> }
>>
>>
>> Actually, as domain type on Arm is always XEN_HVM_DOMAIN, we could probably
>> have the following on Arm:
>>
>> int arch_has_restricted_virtio_memory_access(void)
>> {
>>      return IS_ENABLED(CONFIG_XEN_VIRTIO);
>> }
>>
>> but I would prefer not to diverge and use common
>> xen_has_restricted_virtio_memory_access().
>>
>> Any thoughts?
> 
> Yes, I would also prefer not to diverge between the x86 and arm versions
> of xen_has_restricted_virtio_memory_access. But what case are we trying
> to catch with (xen_pv_domain() || xen_hvm_domain()) ? Even on x86, it is
> not going to leave much out. Is it really meant only to exclude pvh
> domains?

It wouldn't exclude pvh domains.

> 
> I have the feeling that we could turn this check into:
> 
> static inline int xen_has_restricted_virtio_memory_access(void)
> {
>      return IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain();
> }
> 
> even on x86, but one of the xen/x86 maintainers should confirm.

I do confirm this is better and functionally equivalent.


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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen
  2022-04-19  6:21         ` Juergen Gross
@ 2022-04-19  6:37           ` Oleksandr
  0 siblings, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-19  6:37 UTC (permalink / raw)
  To: Juergen Gross, Stefano Stabellini
  Cc: xen-devel, x86, linux-kernel, Dave Hansen, Andy Lutomirski,
	Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, Boris Ostrovsky, Julien Grall,
	Oleksandr Tyshchenko, linux-arm-kernel, Christoph Hellwig,
	Michael S. Tsirkin


Hello Stefano, Juergen


On 19.04.22 09:21, Juergen Gross wrote:
> On 18.04.22 21:11, Stefano Stabellini wrote:
>> On Sun, 17 Apr 2022, Oleksandr wrote:
>>> On 16.04.22 01:01, Stefano Stabellini wrote:
>>>> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>>>>> From: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> In order to support virtio in Xen guests add a config option enabling
>>>>> the user to specify whether in all Xen guests virtio should be 
>>>>> able to
>>>>> access memory via Xen grant mappings only on the host side.
>>>>>
>>>>> This applies to fully virtualized guests only, as for paravirtualized
>>>>> guests this is mandatory.
>>>>>
>>>>> This requires to switch arch_has_restricted_virtio_memory_access()
>>>>> from a pure stub to a real function on x86 systems (Arm systems are
>>>>> not covered by now).
>>>>>
>>>>> Add the needed functionality by providing a special set of DMA ops
>>>>> handling the needed grant operations for the I/O pages.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>>    arch/x86/mm/init.c        |  15 ++++
>>>>>    arch/x86/mm/mem_encrypt.c |   5 --
>>>>>    arch/x86/xen/Kconfig      |   9 +++
>>>>>    drivers/xen/Kconfig       |  20 ++++++
>>>>>    drivers/xen/Makefile      |   1 +
>>>>>    drivers/xen/xen-virtio.c  | 177
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    include/xen/xen-ops.h     |   8 +++
>>>>>    7 files changed, 230 insertions(+), 5 deletions(-)
>>>>>    create mode 100644 drivers/xen/xen-virtio.c
>>>>>
>>>>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>>>>> index d8cfce2..526a3b2 100644
>>>>> --- a/arch/x86/mm/init.c
>>>>> +++ b/arch/x86/mm/init.c
>>>>> @@ -8,6 +8,8 @@
>>>>>    #include <linux/kmemleak.h>
>>>>>    #include <linux/sched/task.h>
>>>>>    +#include <xen/xen.h>
>>>>> +
>>>>>    #include <asm/set_memory.h>
>>>>>    #include <asm/e820/api.h>
>>>>>    #include <asm/init.h>
>>>>> @@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
>>>>>        return pages;
>>>>>    }
>>>>>    #endif
>>>>> +
>>>>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>>> +int arch_has_restricted_virtio_memory_access(void)
>>>>> +{
>>>>> +    if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
>>>>> +        return 1;
>>>>> +    if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>>>>> +        return 1;
>>>> I think these two checks could be moved to a separate function in a 
>>>> Xen
>>>> header, e.g. xen_restricted_virtio_memory_access, and here you could
>>>> just
>>>>
>>>> if (xen_restricted_virtio_memory_access())
>>>>       return 1;
>>>
>>> Agree, will do
>>>
>>>
>>>>
>>>>
>>>>
>>>>> +    return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>>>>> +#endif
>>>>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>>>>> index 50d2099..dda020f 100644
>>>>> --- a/arch/x86/mm/mem_encrypt.c
>>>>> +++ b/arch/x86/mm/mem_encrypt.c
>>>>> @@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
>>>>>        print_mem_encrypt_feature_info();
>>>>>    }
>>>>>    -int arch_has_restricted_virtio_memory_access(void)
>>>>> -{
>>>>> -    return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>>>>> -}
>>>>> -EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>>>>> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
>>>>> index 85246dd..dffdffd 100644
>>>>> --- a/arch/x86/xen/Kconfig
>>>>> +++ b/arch/x86/xen/Kconfig
>>>>> @@ -92,3 +92,12 @@ config XEN_DOM0
>>>>>        select X86_X2APIC if XEN_PVH && X86_64
>>>>>        help
>>>>>          Support running as a Xen Dom0 guest.
>>>>> +
>>>>> +config XEN_PV_VIRTIO
>>>>> +    bool "Xen virtio support for PV guests"
>>>>> +    depends on XEN_VIRTIO && XEN_PV
>>>>> +    default y
>>>>> +    help
>>>>> +      Support virtio for running as a paravirtualized guest. This 
>>>>> will
>>>>> +      need support on the backend side (qemu or kernel, depending 
>>>>> on the
>>>>> +      virtio device types used).
>>>>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>>>>> index 120d32f..fc61f7a 100644
>>>>> --- a/drivers/xen/Kconfig
>>>>> +++ b/drivers/xen/Kconfig
>>>>> @@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
>>>>>          having to balloon out RAM regions in order to obtain 
>>>>> physical memory
>>>>>          space to create such mappings.
>>>>>    +config XEN_VIRTIO
>>>>> +    bool "Xen virtio support"
>>>>> +    default n
>>>>> +    depends on VIRTIO && DMA_OPS
>>>>> +    select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>>> +    help
>>>>> +      Enable virtio support for running as Xen guest. Depending 
>>>>> on the
>>>>> +      guest type this will require special support on the backend 
>>>>> side
>>>>> +      (qemu or kernel, depending on the virtio device types used).
>>>>> +
>>>>> +config XEN_HVM_VIRTIO_GRANT
>>>>> +    bool "Require virtio for fully virtualized guests to use grant
>>>>> mappings"
>>>>> +    depends on XEN_VIRTIO && X86_64
>>>>> +    default y
>>>>> +    help
>>>>> +      Require virtio for fully virtualized guests to use grant 
>>>>> mappings.
>>>>> +      This will avoid the need to give the backend the right to 
>>>>> map all
>>>>> +      of the guest memory. This will need support on the backend 
>>>>> side
>>>>> +      (qemu or kernel, depending on the virtio device types used).
>>>> I don't think we need 3 visible kconfig options for this.
>>>>
>>>> In fact, I would only add one: XEN_VIRTIO. We can have any X86 (or 
>>>> ARM)
>>>> specific dependencies in the "depends" line under XEN_VIRTIO. And I
>>>> don't think we need XEN_HVM_VIRTIO_GRANT as a kconfig option
>>>> necessarely. It doesn't seem like some we want as build time 
>>>> option. At
>>>> most, it could be a runtime option (like a command line) or a debug
>>>> option (like an #define at the top of the source file.)
>>>
>>>
>>> I don't know what was the initial idea of having and extra 
>>> XEN_HVM_VIRTIO and
>>> XEN_PV_VIRTIO options, but taking into the account that
>>> they are only used in arch_has_restricted_virtio_memory_access() 
>>> currently, I
>>> share your opinion regarding a single XEN_VIRTIO option.
>>>
>>> Looking ahead (including changes in the commit #4), we can imagine the
>>> resulting option:
>>>
>>> config XEN_VIRTIO
>>>      bool "Xen virtio support"
>>>      default n
>>>      depends on VIRTIO && DMA_OPS
>>>      depends on (X86_64 || ARM || ARM64)
>>>      select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>      help
>>>        Enable virtio support for running as Xen guest. Depending on the
>>>        guest type this will require special support on the backend side
>>>        (qemu or kernel, depending on the virtio device types used).
>>>
>>>
>>> and then arch_has_restricted_virtio_memory_access() per arch:
>>>
>>>
>>> 1. x86:
>>>
>>> int arch_has_restricted_virtio_memory_access(void)
>>> {
>>>      return (xen_has_restricted_virtio_memory_access() ||
>>>              cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
>>> }
>>>
>>>
>>> 2. Arm:
>>>
>>> int arch_has_restricted_virtio_memory_access(void)
>>> {
>>>      return xen_has_restricted_virtio_memory_access();
>>> }
>>>
>>>
>>> 3. xen.h:
>>>
>>> static inline int xen_has_restricted_virtio_memory_access(void)
>>> {
>>>      if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() ||
>>> xen_hvm_domain()))
>>>          return 1;
>>>
>>>      return 0;
>>> }
>>>
>>>
>>> Actually, as domain type on Arm is always XEN_HVM_DOMAIN, we could 
>>> probably
>>> have the following on Arm:
>>>
>>> int arch_has_restricted_virtio_memory_access(void)
>>> {
>>>      return IS_ENABLED(CONFIG_XEN_VIRTIO);
>>> }
>>>
>>> but I would prefer not to diverge and use common
>>> xen_has_restricted_virtio_memory_access().
>>>
>>> Any thoughts?
>>
>> Yes, I would also prefer not to diverge between the x86 and arm versions
>> of xen_has_restricted_virtio_memory_access. But what case are we trying
>> to catch with (xen_pv_domain() || xen_hvm_domain()) ? Even on x86, it is
>> not going to leave much out. Is it really meant only to exclude pvh
>> domains?

Good question. By leaving (xen_pv_domain() || xen_hvm_domain()) here I 
tried to retain what the *initial* version of 
arch_has_restricted_virtio_memory_access() covered.


>
> It wouldn't exclude pvh domains.


ok


>
>>
>> I have the feeling that we could turn this check into:
>>
>> static inline int xen_has_restricted_virtio_memory_access(void)
>> {
>>      return IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain();
>> }
>>
>> even on x86, but one of the xen/x86 maintainers should confirm.
>
> I do confirm this is better and functionally equivalent.


Perfect, thank you for confirming. Will use that check.


>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer
  2022-04-18 19:11       ` Stefano Stabellini
@ 2022-04-19  6:58         ` Juergen Gross
  2022-04-19  7:07           ` Oleksandr
  0 siblings, 1 reply; 40+ messages in thread
From: Juergen Gross @ 2022-04-19  6:58 UTC (permalink / raw)
  To: Stefano Stabellini, Oleksandr
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Russell King, Boris Ostrovsky, Julien Grall, Michael S. Tsirkin,
	Christoph Hellwig


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

On 18.04.22 21:11, Stefano Stabellini wrote:
> On Sun, 17 Apr 2022, Oleksandr wrote:
>> On 16.04.22 01:02, Stefano Stabellini wrote:
>>> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> In the context of current patch do the following:
>>>> 1. Update code to support virtio-mmio devices
>>>> 2. Introduce struct xen_virtio_data and account passed virtio devices
>>>>      (using list) as we need to store some per-device data
>>>> 3. Add multi-page support for xen_virtio_dma_map(unmap)_page callbacks
>>>> 4. Harden code against malicious backend
>>>> 5. Change to use alloc_pages_exact() instead of __get_free_pages()
>>>> 6. Introduce locking scheme to protect mappings (I am not 100% sure
>>>>      whether per-device lock is really needed)
>>>> 7. Handle virtio device's DMA mask
>>>> 8. Retrieve the ID of backend domain from DT for virtio-mmio device
>>>>      instead of hardcoding it.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>>    arch/arm/xen/enlighten.c |  11 +++
>>>>    drivers/xen/Kconfig      |   2 +-
>>>>    drivers/xen/xen-virtio.c | 200
>>>> ++++++++++++++++++++++++++++++++++++++++++-----
>>>>    include/xen/xen-ops.h    |   5 ++
>>>>    4 files changed, 196 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>>>> index ec5b082..870d92f 100644
>>>> --- a/arch/arm/xen/enlighten.c
>>>> +++ b/arch/arm/xen/enlighten.c
>>>> @@ -409,6 +409,17 @@ int __init arch_xen_unpopulated_init(struct resource
>>>> **res)
>>>>    }
>>>>    #endif
>>>>    +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>> +int arch_has_restricted_virtio_memory_access(void)
>>>> +{
>>>> +	if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>>>> +		return 1;
>>> Instead of xen_hvm_domain(), you can just use xen_domain(). Also there
>>> is no need for the #ifdef
>>> CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, given that:
>>>
>>> CONFIG_XEN_HVM_VIRTIO_GRANT depends on XEN_VIRTIO which selects
>>> ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>
>>
>> Yes, but please see my comments in commit #2 regarding
>> CONFIG_XEN_HVM_VIRTIO_GRANT option and
>> arch_has_restricted_virtio_memory_access() on Arm.
>>
>> I propose to have the following on Arm:
>>
>> int arch_has_restricted_virtio_memory_access(void)
>> {
>>       return xen_has_restricted_virtio_memory_access();
>> }
>>
>>
>> where common xen.h contain a helper to be used by both Arm and x86:
>>
>> static inline int xen_has_restricted_virtio_memory_access(void)
>> {
>>       if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() ||
>> xen_hvm_domain()))
>>           return 1;
>>
>>       return 0;
>> }
>>
>>
>> But I would be happy with what you propose as well.
> 
> As I wrote in the previous reply, I also prefer to share the code
> between x86 and ARM, and I think it could look like:
> 
> int arch_has_restricted_virtio_memory_access(void)
> {
>       return xen_has_restricted_virtio_memory_access();
> }
> [...]
> static inline int xen_has_restricted_virtio_memory_access(void)
> {
>       return (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain());
> }
> 
> But let's check with Juergen and Boris.
> 
> 
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>>>> +#endif
>>>> +
>>>>    static void __init xen_dt_guest_init(void)
>>>>    {
>>>>    	struct device_node *xen_node;
>>>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>>>> index fc61f7a..56afe6a 100644
>>>> --- a/drivers/xen/Kconfig
>>>> +++ b/drivers/xen/Kconfig
>>>> @@ -347,7 +347,7 @@ config XEN_VIRTIO
>>>>      config XEN_HVM_VIRTIO_GRANT
>>>>    	bool "Require virtio for fully virtualized guests to use grant
>>>> mappings"
>>>> -	depends on XEN_VIRTIO && X86_64
>>>> +	depends on XEN_VIRTIO && (X86_64 || ARM || ARM64)
>>> you can remove the architectural dependencies
>>
>>
>> According to the conversation in commit #2 we are considering just a single
>> XEN_VIRTIO option, but it is going to has the
>> same architectural dependencies: (X86_64 || ARM || ARM64)
>>
>> By removing the architectural dependencies here, we will leave also X86_32
>> covered (neither XEN_HVM_VIRTIO_GRANT nor XEN_PV_VIRTIO covered it). I don't
>> know whether it is ok or not.
>>
>> Shall I remove dependencies anyway?
> 
> No, good point. I don't know about X86_32. This is another detail where
> Juergen or Boris should comment.

X86_32 should in theory work (it is HVM/PVH only, as PV 32-bit guests are no
longer supported).


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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer
  2022-04-19  6:58         ` Juergen Gross
@ 2022-04-19  7:07           ` Oleksandr
  0 siblings, 0 replies; 40+ messages in thread
From: Oleksandr @ 2022-04-19  7:07 UTC (permalink / raw)
  To: Juergen Gross, Stefano Stabellini
  Cc: xen-devel, linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Russell King, Boris Ostrovsky, Julien Grall, Michael S. Tsirkin,
	Christoph Hellwig


Hello Stefano, Juergen


On 19.04.22 09:58, Juergen Gross wrote:
> On 18.04.22 21:11, Stefano Stabellini wrote:
>> On Sun, 17 Apr 2022, Oleksandr wrote:
>>> On 16.04.22 01:02, Stefano Stabellini wrote:
>>>> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>
>>>>> In the context of current patch do the following:
>>>>> 1. Update code to support virtio-mmio devices
>>>>> 2. Introduce struct xen_virtio_data and account passed virtio devices
>>>>>      (using list) as we need to store some per-device data
>>>>> 3. Add multi-page support for xen_virtio_dma_map(unmap)_page 
>>>>> callbacks
>>>>> 4. Harden code against malicious backend
>>>>> 5. Change to use alloc_pages_exact() instead of __get_free_pages()
>>>>> 6. Introduce locking scheme to protect mappings (I am not 100% sure
>>>>>      whether per-device lock is really needed)
>>>>> 7. Handle virtio device's DMA mask
>>>>> 8. Retrieve the ID of backend domain from DT for virtio-mmio device
>>>>>      instead of hardcoding it.
>>>>>
>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>> ---
>>>>>    arch/arm/xen/enlighten.c |  11 +++
>>>>>    drivers/xen/Kconfig      |   2 +-
>>>>>    drivers/xen/xen-virtio.c | 200
>>>>> ++++++++++++++++++++++++++++++++++++++++++-----
>>>>>    include/xen/xen-ops.h    |   5 ++
>>>>>    4 files changed, 196 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>>>>> index ec5b082..870d92f 100644
>>>>> --- a/arch/arm/xen/enlighten.c
>>>>> +++ b/arch/arm/xen/enlighten.c
>>>>> @@ -409,6 +409,17 @@ int __init arch_xen_unpopulated_init(struct 
>>>>> resource
>>>>> **res)
>>>>>    }
>>>>>    #endif
>>>>>    +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>>> +int arch_has_restricted_virtio_memory_access(void)
>>>>> +{
>>>>> +    if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>>>>> +        return 1;
>>>> Instead of xen_hvm_domain(), you can just use xen_domain(). Also there
>>>> is no need for the #ifdef
>>>> CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, given that:
>>>>
>>>> CONFIG_XEN_HVM_VIRTIO_GRANT depends on XEN_VIRTIO which selects
>>>> ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>
>>>
>>> Yes, but please see my comments in commit #2 regarding
>>> CONFIG_XEN_HVM_VIRTIO_GRANT option and
>>> arch_has_restricted_virtio_memory_access() on Arm.
>>>
>>> I propose to have the following on Arm:
>>>
>>> int arch_has_restricted_virtio_memory_access(void)
>>> {
>>>       return xen_has_restricted_virtio_memory_access();
>>> }
>>>
>>>
>>> where common xen.h contain a helper to be used by both Arm and x86:
>>>
>>> static inline int xen_has_restricted_virtio_memory_access(void)
>>> {
>>>       if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() ||
>>> xen_hvm_domain()))
>>>           return 1;
>>>
>>>       return 0;
>>> }
>>>
>>>
>>> But I would be happy with what you propose as well.
>>
>> As I wrote in the previous reply, I also prefer to share the code
>> between x86 and ARM, and I think it could look like:
>>
>> int arch_has_restricted_virtio_memory_access(void)
>> {
>>       return xen_has_restricted_virtio_memory_access();
>> }
>> [...]
>> static inline int xen_has_restricted_virtio_memory_access(void)
>> {
>>       return (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain());
>> }
>>
>> But let's check with Juergen and Boris.


for the record, it is already clarified in commit #2, I will use this 
variant.


>>
>>
>>
>>>>> +    return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>>>>> +#endif
>>>>> +
>>>>>    static void __init xen_dt_guest_init(void)
>>>>>    {
>>>>>        struct device_node *xen_node;
>>>>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>>>>> index fc61f7a..56afe6a 100644
>>>>> --- a/drivers/xen/Kconfig
>>>>> +++ b/drivers/xen/Kconfig
>>>>> @@ -347,7 +347,7 @@ config XEN_VIRTIO
>>>>>      config XEN_HVM_VIRTIO_GRANT
>>>>>        bool "Require virtio for fully virtualized guests to use grant
>>>>> mappings"
>>>>> -    depends on XEN_VIRTIO && X86_64
>>>>> +    depends on XEN_VIRTIO && (X86_64 || ARM || ARM64)
>>>> you can remove the architectural dependencies
>>>
>>>
>>> According to the conversation in commit #2 we are considering just a 
>>> single
>>> XEN_VIRTIO option, but it is going to has the
>>> same architectural dependencies: (X86_64 || ARM || ARM64)
>>>
>>> By removing the architectural dependencies here, we will leave also 
>>> X86_32
>>> covered (neither XEN_HVM_VIRTIO_GRANT nor XEN_PV_VIRTIO covered it). 
>>> I don't
>>> know whether it is ok or not.
>>>
>>> Shall I remove dependencies anyway?
>>
>> No, good point. I don't know about X86_32. This is another detail where
>> Juergen or Boris should comment.
>
> X86_32 should in theory work (it is HVM/PVH only, as PV 32-bit guests 
> are no
> longer supported).


ok, thank you for confirming. I will drop architectural dependencies then.


>
>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-18 19:11         ` Stefano Stabellini
@ 2022-04-19 12:17           ` Oleksandr
  2022-04-19 14:48             ` Juergen Gross
  0 siblings, 1 reply; 40+ messages in thread
From: Oleksandr @ 2022-04-19 12:17 UTC (permalink / raw)
  To: Stefano Stabellini, Juergen Gross
  Cc: Christoph Hellwig, xen-devel, linux-kernel, linux-arm-kernel,
	Oleksandr Tyshchenko, Boris Ostrovsky, Julien Grall,
	Michael S. Tsirkin


Hello Stefano, Juergen


On 18.04.22 22:11, Stefano Stabellini wrote:
> On Mon, 18 Apr 2022, Oleksandr wrote:
>> On 16.04.22 09:07, Christoph Hellwig wrote:
>>
>> Hello Christoph
>>
>>> On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
>>>> This makes sense overall. Considering that the swiotlb-xen case and the
>>>> virtio case are mutually exclusive, I would write it like this:
>>> Curious question:  Why can't the same grant scheme also be used for
>>> non-virtio devices?  I really hate having virtio hooks in the arch
>>> dma code.  Why can't Xen just say in DT/ACPI that grants can be used
>>> for a given device?
> [...]
>
>> This patch series tries to make things work with "virtio" devices in Xen
>> system without introducing any modifications to code under drivers/virtio.
>
> Actually, I think Christoph has a point.
>
> There is nothing inherently virtio specific in this patch series or in
> the "xen,dev-domid" device tree binding.


Although the main intention of this series was to enable using virtio 
devices in Xen guests, I agree that nothing in new DMA ops layer 
(xen-virtio.c) is virtio specific (at least at the moment). Regarding 
the whole patch series I am not quite sure, as it uses 
arch_has_restricted_virtio_memory_access().


>   Assuming a given device is
> emulated by a Xen backend, it could be used with grants as well.
>
> For instance, we could provide an emulated e1000 NIC with a
> "xen,dev-domid" property in device tree. Linux could use grants with it
> and the backend could map the grants. It would work the same way as
> virtio-net/block/etc. Passthrough devices wouldn't have the
> "xen,dev-domid" property, so no problems.
>
> So I think we could easily generalize this work and expand it to any
> device. We just need to hook on the "xen,dev-domid" device tree
> property.
>
> I think it is just a matter of:
> - remove the "virtio,mmio" check from xen_is_virtio_device
> - rename xen_is_virtio_device to something more generic, like
>    xen_is_grants_device
> - rename xen_virtio_setup_dma_ops to something more generic, like
>    xen_grants_setup_dma_ops
>
> And that's pretty much it.

+ likely renaming everything in that patch series not to mention virtio 
(mostly related to xen-virtio.c internals).


Stefano, thank you for clarifying Christoph's point.

Well, I am not against going this direction. Could we please make a 
decision on this? @Juergen, what is your opinion?



-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-19 12:17           ` Oleksandr
@ 2022-04-19 14:48             ` Juergen Gross
  2022-04-19 17:11               ` Oleksandr
  0 siblings, 1 reply; 40+ messages in thread
From: Juergen Gross @ 2022-04-19 14:48 UTC (permalink / raw)
  To: Oleksandr, Stefano Stabellini
  Cc: Christoph Hellwig, xen-devel, linux-kernel, linux-arm-kernel,
	Oleksandr Tyshchenko, Boris Ostrovsky, Julien Grall,
	Michael S. Tsirkin


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

On 19.04.22 14:17, Oleksandr wrote:
> 
> Hello Stefano, Juergen
> 
> 
> On 18.04.22 22:11, Stefano Stabellini wrote:
>> On Mon, 18 Apr 2022, Oleksandr wrote:
>>> On 16.04.22 09:07, Christoph Hellwig wrote:
>>>
>>> Hello Christoph
>>>
>>>> On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
>>>>> This makes sense overall. Considering that the swiotlb-xen case and the
>>>>> virtio case are mutually exclusive, I would write it like this:
>>>> Curious question:  Why can't the same grant scheme also be used for
>>>> non-virtio devices?  I really hate having virtio hooks in the arch
>>>> dma code.  Why can't Xen just say in DT/ACPI that grants can be used
>>>> for a given device?
>> [...]
>>
>>> This patch series tries to make things work with "virtio" devices in Xen
>>> system without introducing any modifications to code under drivers/virtio.
>>
>> Actually, I think Christoph has a point.
>>
>> There is nothing inherently virtio specific in this patch series or in
>> the "xen,dev-domid" device tree binding.
> 
> 
> Although the main intention of this series was to enable using virtio devices in 
> Xen guests, I agree that nothing in new DMA ops layer (xen-virtio.c) is virtio 
> specific (at least at the moment). Regarding the whole patch series I am not 
> quite sure, as it uses arch_has_restricted_virtio_memory_access(). >
>>   Assuming a given device is
>> emulated by a Xen backend, it could be used with grants as well.
>>
>> For instance, we could provide an emulated e1000 NIC with a
>> "xen,dev-domid" property in device tree. Linux could use grants with it
>> and the backend could map the grants. It would work the same way as
>> virtio-net/block/etc. Passthrough devices wouldn't have the
>> "xen,dev-domid" property, so no problems.
>>
>> So I think we could easily generalize this work and expand it to any
>> device. We just need to hook on the "xen,dev-domid" device tree
>> property.
>>
>> I think it is just a matter of:
>> - remove the "virtio,mmio" check from xen_is_virtio_device
>> - rename xen_is_virtio_device to something more generic, like
>>    xen_is_grants_device

xen_is_grants_dma_device, please. Normal Xen PV devices are covered by
grants, too, and I'd like to avoid the confusion arising from this.

>> - rename xen_virtio_setup_dma_ops to something more generic, like
>>    xen_grants_setup_dma_ops
>>
>> And that's pretty much it.
> 
> + likely renaming everything in that patch series not to mention virtio (mostly 
> related to xen-virtio.c internals).
> 
> 
> Stefano, thank you for clarifying Christoph's point.
> 
> Well, I am not against going this direction. Could we please make a decision on 
> this? @Juergen, what is your opinion?

Yes, why not.

Maybe rename xen-virtio.c to grant-dma.c?

I'd keep the XEN_VIRTIO related config option, as this will be the normal use
case. grant-dma.c should be covered by a new hidden config option XEN_GRANT_DMA
selected by XEN_VIRTIO.

CONFIG_XEN_VIRTIO should still guard xen_has_restricted_virtio_memory_access().


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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-19 14:48             ` Juergen Gross
@ 2022-04-19 17:11               ` Oleksandr
  2022-04-20  0:23                 ` Stefano Stabellini
  0 siblings, 1 reply; 40+ messages in thread
From: Oleksandr @ 2022-04-19 17:11 UTC (permalink / raw)
  To: Juergen Gross, Stefano Stabellini
  Cc: Christoph Hellwig, xen-devel, linux-kernel, linux-arm-kernel,
	Oleksandr Tyshchenko, Boris Ostrovsky, Julien Grall,
	Michael S. Tsirkin


Hello Stefano, Juergen


On 19.04.22 17:48, Juergen Gross wrote:
> On 19.04.22 14:17, Oleksandr wrote:
>>
>> Hello Stefano, Juergen
>>
>>
>> On 18.04.22 22:11, Stefano Stabellini wrote:
>>> On Mon, 18 Apr 2022, Oleksandr wrote:
>>>> On 16.04.22 09:07, Christoph Hellwig wrote:
>>>>
>>>> Hello Christoph
>>>>
>>>>> On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
>>>>>> This makes sense overall. Considering that the swiotlb-xen case 
>>>>>> and the
>>>>>> virtio case are mutually exclusive, I would write it like this:
>>>>> Curious question:  Why can't the same grant scheme also be used for
>>>>> non-virtio devices?  I really hate having virtio hooks in the arch
>>>>> dma code.  Why can't Xen just say in DT/ACPI that grants can be used
>>>>> for a given device?
>>> [...]
>>>
>>>> This patch series tries to make things work with "virtio" devices 
>>>> in Xen
>>>> system without introducing any modifications to code under 
>>>> drivers/virtio.
>>>
>>> Actually, I think Christoph has a point.
>>>
>>> There is nothing inherently virtio specific in this patch series or in
>>> the "xen,dev-domid" device tree binding.
>>
>>
>> Although the main intention of this series was to enable using virtio 
>> devices in Xen guests, I agree that nothing in new DMA ops layer 
>> (xen-virtio.c) is virtio specific (at least at the moment). Regarding 
>> the whole patch series I am not quite sure, as it uses 
>> arch_has_restricted_virtio_memory_access(). >
>>>   Assuming a given device is
>>> emulated by a Xen backend, it could be used with grants as well.
>>>
>>> For instance, we could provide an emulated e1000 NIC with a
>>> "xen,dev-domid" property in device tree. Linux could use grants with it
>>> and the backend could map the grants. It would work the same way as
>>> virtio-net/block/etc. Passthrough devices wouldn't have the
>>> "xen,dev-domid" property, so no problems.
>>>
>>> So I think we could easily generalize this work and expand it to any
>>> device. We just need to hook on the "xen,dev-domid" device tree
>>> property.
>>>
>>> I think it is just a matter of:
>>> - remove the "virtio,mmio" check from xen_is_virtio_device
>>> - rename xen_is_virtio_device to something more generic, like
>>>    xen_is_grants_device
>
> xen_is_grants_dma_device, please. Normal Xen PV devices are covered by
> grants, too, and I'd like to avoid the confusion arising from this.


yes, this definitely makes sense as we need to distinguish


>
>
>>> - rename xen_virtio_setup_dma_ops to something more generic, like
>>>    xen_grants_setup_dma_ops
>>>
>>> And that's pretty much it.
>>
>> + likely renaming everything in that patch series not to mention 
>> virtio (mostly related to xen-virtio.c internals).
>>
>>
>> Stefano, thank you for clarifying Christoph's point.
>>
>> Well, I am not against going this direction. Could we please make a 
>> decision on this? @Juergen, what is your opinion?
>
> Yes, why not.


ok, thank you for confirming.


>
>
> Maybe rename xen-virtio.c to grant-dma.c?


Personally I don't mind.


>
> I'd keep the XEN_VIRTIO related config option, as this will be the 
> normal use
> case. grant-dma.c should be covered by a new hidden config option 
> XEN_GRANT_DMA
> selected by XEN_VIRTIO.


I got it, ok


>
>
> CONFIG_XEN_VIRTIO should still guard 
> xen_has_restricted_virtio_memory_access().


ok


So a few questions to clarify:

1. What is the best place to keep "xen,dev-domid" binding's description 
now? I think that proposed in current series place 
(Documentation/devicetree/bindings/virtio/) is not good fit now.

2. I assume the logic in the current patch will remain the same, I mean 
we will still assign Xen grant DMA ops from xen_setup_dma_ops() here?


>
>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-19 17:11               ` Oleksandr
@ 2022-04-20  0:23                 ` Stefano Stabellini
  2022-04-20  9:00                   ` Oleksandr
  0 siblings, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-20  0:23 UTC (permalink / raw)
  To: Oleksandr
  Cc: Juergen Gross, Stefano Stabellini, Christoph Hellwig, xen-devel,
	linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Boris Ostrovsky, Julien Grall, Michael S. Tsirkin

[-- Attachment #1: Type: text/plain, Size: 4244 bytes --]

On Tue, 19 Apr 2022, Oleksandr wrote:
> On 19.04.22 17:48, Juergen Gross wrote:
> > On 19.04.22 14:17, Oleksandr wrote:
> > > 
> > > Hello Stefano, Juergen
> > > 
> > > 
> > > On 18.04.22 22:11, Stefano Stabellini wrote:
> > > > On Mon, 18 Apr 2022, Oleksandr wrote:
> > > > > On 16.04.22 09:07, Christoph Hellwig wrote:
> > > > > 
> > > > > Hello Christoph
> > > > > 
> > > > > > On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
> > > > > > > This makes sense overall. Considering that the swiotlb-xen case
> > > > > > > and the
> > > > > > > virtio case are mutually exclusive, I would write it like this:
> > > > > > Curious question:  Why can't the same grant scheme also be used for
> > > > > > non-virtio devices?  I really hate having virtio hooks in the arch
> > > > > > dma code.  Why can't Xen just say in DT/ACPI that grants can be used
> > > > > > for a given device?
> > > > [...]
> > > > 
> > > > > This patch series tries to make things work with "virtio" devices in
> > > > > Xen
> > > > > system without introducing any modifications to code under
> > > > > drivers/virtio.
> > > > 
> > > > Actually, I think Christoph has a point.
> > > > 
> > > > There is nothing inherently virtio specific in this patch series or in
> > > > the "xen,dev-domid" device tree binding.
> > > 
> > > 
> > > Although the main intention of this series was to enable using virtio
> > > devices in Xen guests, I agree that nothing in new DMA ops layer
> > > (xen-virtio.c) is virtio specific (at least at the moment). Regarding the
> > > whole patch series I am not quite sure, as it uses
> > > arch_has_restricted_virtio_memory_access(). >
> > > >   Assuming a given device is
> > > > emulated by a Xen backend, it could be used with grants as well.
> > > > 
> > > > For instance, we could provide an emulated e1000 NIC with a
> > > > "xen,dev-domid" property in device tree. Linux could use grants with it
> > > > and the backend could map the grants. It would work the same way as
> > > > virtio-net/block/etc. Passthrough devices wouldn't have the
> > > > "xen,dev-domid" property, so no problems.
> > > > 
> > > > So I think we could easily generalize this work and expand it to any
> > > > device. We just need to hook on the "xen,dev-domid" device tree
> > > > property.
> > > > 
> > > > I think it is just a matter of:
> > > > - remove the "virtio,mmio" check from xen_is_virtio_device
> > > > - rename xen_is_virtio_device to something more generic, like
> > > >    xen_is_grants_device
> > 
> > xen_is_grants_dma_device, please. Normal Xen PV devices are covered by
> > grants, too, and I'd like to avoid the confusion arising from this.
> 
> 
> yes, this definitely makes sense as we need to distinguish
> 
> 
> > 
> > 
> > > > - rename xen_virtio_setup_dma_ops to something more generic, like
> > > >    xen_grants_setup_dma_ops
> > > > 
> > > > And that's pretty much it.
> > > 
> > > + likely renaming everything in that patch series not to mention virtio
> > > (mostly related to xen-virtio.c internals).
> > > 
> > > 
> > > Stefano, thank you for clarifying Christoph's point.
> > > 
> > > Well, I am not against going this direction. Could we please make a
> > > decision on this? @Juergen, what is your opinion?
> > 
> > Yes, why not.
> 
> 
> ok, thank you for confirming.
> 
> 
> > 
> > 
> > Maybe rename xen-virtio.c to grant-dma.c?
> 
> 
> Personally I don't mind.
> 
> 
> > 
> > I'd keep the XEN_VIRTIO related config option, as this will be the normal
> > use
> > case. grant-dma.c should be covered by a new hidden config option
> > XEN_GRANT_DMA
> > selected by XEN_VIRTIO.
> 
> 
> I got it, ok
> 
> 
> > 
> > 
> > CONFIG_XEN_VIRTIO should still guard
> > xen_has_restricted_virtio_memory_access().
> 
> 
> ok
> 
> 
> So a few questions to clarify:
> 
> 1. What is the best place to keep "xen,dev-domid" binding's description now? I
> think that proposed in current series place
> (Documentation/devicetree/bindings/virtio/) is not good fit now.

I would probably add it to the existing
Documentation/devicetree/bindings/arm/xen.txt.


> 2. I assume the logic in the current patch will remain the same, I mean we
> will still assign Xen grant DMA ops from xen_setup_dma_ops() here?

Yes I think so

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-20  0:23                 ` Stefano Stabellini
@ 2022-04-20  9:00                   ` Oleksandr
  2022-04-20 22:49                     ` Stefano Stabellini
  0 siblings, 1 reply; 40+ messages in thread
From: Oleksandr @ 2022-04-20  9:00 UTC (permalink / raw)
  To: Stefano Stabellini, Juergen Gross
  Cc: Christoph Hellwig, xen-devel, linux-kernel, linux-arm-kernel,
	Oleksandr Tyshchenko, Boris Ostrovsky, Julien Grall,
	Michael S. Tsirkin


Hello Stefano, Juergen


On 20.04.22 03:23, Stefano Stabellini wrote:
> On Tue, 19 Apr 2022, Oleksandr wrote:
>> On 19.04.22 17:48, Juergen Gross wrote:
>>> On 19.04.22 14:17, Oleksandr wrote:
>>>> Hello Stefano, Juergen
>>>>
>>>>
>>>> On 18.04.22 22:11, Stefano Stabellini wrote:
>>>>> On Mon, 18 Apr 2022, Oleksandr wrote:
>>>>>> On 16.04.22 09:07, Christoph Hellwig wrote:
>>>>>>
>>>>>> Hello Christoph
>>>>>>
>>>>>>> On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
>>>>>>>> This makes sense overall. Considering that the swiotlb-xen case
>>>>>>>> and the
>>>>>>>> virtio case are mutually exclusive, I would write it like this:
>>>>>>> Curious question:  Why can't the same grant scheme also be used for
>>>>>>> non-virtio devices?  I really hate having virtio hooks in the arch
>>>>>>> dma code.  Why can't Xen just say in DT/ACPI that grants can be used
>>>>>>> for a given device?
>>>>> [...]
>>>>>
>>>>>> This patch series tries to make things work with "virtio" devices in
>>>>>> Xen
>>>>>> system without introducing any modifications to code under
>>>>>> drivers/virtio.
>>>>> Actually, I think Christoph has a point.
>>>>>
>>>>> There is nothing inherently virtio specific in this patch series or in
>>>>> the "xen,dev-domid" device tree binding.
>>>>
>>>> Although the main intention of this series was to enable using virtio
>>>> devices in Xen guests, I agree that nothing in new DMA ops layer
>>>> (xen-virtio.c) is virtio specific (at least at the moment). Regarding the
>>>> whole patch series I am not quite sure, as it uses
>>>> arch_has_restricted_virtio_memory_access(). >
>>>>>    Assuming a given device is
>>>>> emulated by a Xen backend, it could be used with grants as well.
>>>>>
>>>>> For instance, we could provide an emulated e1000 NIC with a
>>>>> "xen,dev-domid" property in device tree. Linux could use grants with it
>>>>> and the backend could map the grants. It would work the same way as
>>>>> virtio-net/block/etc. Passthrough devices wouldn't have the
>>>>> "xen,dev-domid" property, so no problems.
>>>>>
>>>>> So I think we could easily generalize this work and expand it to any
>>>>> device. We just need to hook on the "xen,dev-domid" device tree
>>>>> property.
>>>>>
>>>>> I think it is just a matter of:
>>>>> - remove the "virtio,mmio" check from xen_is_virtio_device
>>>>> - rename xen_is_virtio_device to something more generic, like
>>>>>     xen_is_grants_device
>>> xen_is_grants_dma_device, please. Normal Xen PV devices are covered by
>>> grants, too, and I'd like to avoid the confusion arising from this.
>>
>> yes, this definitely makes sense as we need to distinguish
>>
>>
>>>
>>>>> - rename xen_virtio_setup_dma_ops to something more generic, like
>>>>>     xen_grants_setup_dma_ops
>>>>>
>>>>> And that's pretty much it.
>>>> + likely renaming everything in that patch series not to mention virtio
>>>> (mostly related to xen-virtio.c internals).
>>>>
>>>>
>>>> Stefano, thank you for clarifying Christoph's point.
>>>>
>>>> Well, I am not against going this direction. Could we please make a
>>>> decision on this? @Juergen, what is your opinion?
>>> Yes, why not.
>>
>> ok, thank you for confirming.
>>
>>
>>>
>>> Maybe rename xen-virtio.c to grant-dma.c?
>>
>> Personally I don't mind.
>>
>>
>>> I'd keep the XEN_VIRTIO related config option, as this will be the normal
>>> use
>>> case. grant-dma.c should be covered by a new hidden config option
>>> XEN_GRANT_DMA
>>> selected by XEN_VIRTIO.
>>
>> I got it, ok
>>
>>
>>>
>>> CONFIG_XEN_VIRTIO should still guard
>>> xen_has_restricted_virtio_memory_access().
>>
>> ok
>>
>>
>> So a few questions to clarify:
>>
>> 1. What is the best place to keep "xen,dev-domid" binding's description now? I
>> think that proposed in current series place
>> (Documentation/devicetree/bindings/virtio/) is not good fit now.
> I would probably add it to the existing
> Documentation/devicetree/bindings/arm/xen.txt.
>
>
>> 2. I assume the logic in the current patch will remain the same, I mean we
>> will still assign Xen grant DMA ops from xen_setup_dma_ops() here?
> Yes I think so


Stefano, thank you for clarifying!


Regarding new naming scheme...

As there is an existing Kconfig option XEN_GRANT_DMA_ALLOC used for 
different purpose, we need to clarify naming scheme here a bit to avoid 
possible confusion.

For example, I am happy with proposed by Juergen ...

... Kconfig option: XEN_GRANT_DMA_OPS

and

... file: grant-dma-ops.c


-- 
Regards,

Oleksandr Tyshchenko


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests
  2022-04-20  9:00                   ` Oleksandr
@ 2022-04-20 22:49                     ` Stefano Stabellini
  0 siblings, 0 replies; 40+ messages in thread
From: Stefano Stabellini @ 2022-04-20 22:49 UTC (permalink / raw)
  To: Oleksandr
  Cc: Stefano Stabellini, Juergen Gross, Christoph Hellwig, xen-devel,
	linux-kernel, linux-arm-kernel, Oleksandr Tyshchenko,
	Boris Ostrovsky, Julien Grall, Michael S. Tsirkin

[-- Attachment #1: Type: text/plain, Size: 5290 bytes --]

On Wed, 20 Apr 2022, Oleksandr wrote:
> On 20.04.22 03:23, Stefano Stabellini wrote:
> > On Tue, 19 Apr 2022, Oleksandr wrote:
> > > On 19.04.22 17:48, Juergen Gross wrote:
> > > > On 19.04.22 14:17, Oleksandr wrote:
> > > > > Hello Stefano, Juergen
> > > > > 
> > > > > 
> > > > > On 18.04.22 22:11, Stefano Stabellini wrote:
> > > > > > On Mon, 18 Apr 2022, Oleksandr wrote:
> > > > > > > On 16.04.22 09:07, Christoph Hellwig wrote:
> > > > > > > 
> > > > > > > Hello Christoph
> > > > > > > 
> > > > > > > > On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini
> > > > > > > > wrote:
> > > > > > > > > This makes sense overall. Considering that the swiotlb-xen
> > > > > > > > > case
> > > > > > > > > and the
> > > > > > > > > virtio case are mutually exclusive, I would write it like
> > > > > > > > > this:
> > > > > > > > Curious question:  Why can't the same grant scheme also be used
> > > > > > > > for
> > > > > > > > non-virtio devices?  I really hate having virtio hooks in the
> > > > > > > > arch
> > > > > > > > dma code.  Why can't Xen just say in DT/ACPI that grants can be
> > > > > > > > used
> > > > > > > > for a given device?
> > > > > > [...]
> > > > > > 
> > > > > > > This patch series tries to make things work with "virtio" devices
> > > > > > > in
> > > > > > > Xen
> > > > > > > system without introducing any modifications to code under
> > > > > > > drivers/virtio.
> > > > > > Actually, I think Christoph has a point.
> > > > > > 
> > > > > > There is nothing inherently virtio specific in this patch series or
> > > > > > in
> > > > > > the "xen,dev-domid" device tree binding.
> > > > > 
> > > > > Although the main intention of this series was to enable using virtio
> > > > > devices in Xen guests, I agree that nothing in new DMA ops layer
> > > > > (xen-virtio.c) is virtio specific (at least at the moment). Regarding
> > > > > the
> > > > > whole patch series I am not quite sure, as it uses
> > > > > arch_has_restricted_virtio_memory_access(). >
> > > > > >    Assuming a given device is
> > > > > > emulated by a Xen backend, it could be used with grants as well.
> > > > > > 
> > > > > > For instance, we could provide an emulated e1000 NIC with a
> > > > > > "xen,dev-domid" property in device tree. Linux could use grants with
> > > > > > it
> > > > > > and the backend could map the grants. It would work the same way as
> > > > > > virtio-net/block/etc. Passthrough devices wouldn't have the
> > > > > > "xen,dev-domid" property, so no problems.
> > > > > > 
> > > > > > So I think we could easily generalize this work and expand it to any
> > > > > > device. We just need to hook on the "xen,dev-domid" device tree
> > > > > > property.
> > > > > > 
> > > > > > I think it is just a matter of:
> > > > > > - remove the "virtio,mmio" check from xen_is_virtio_device
> > > > > > - rename xen_is_virtio_device to something more generic, like
> > > > > >     xen_is_grants_device
> > > > xen_is_grants_dma_device, please. Normal Xen PV devices are covered by
> > > > grants, too, and I'd like to avoid the confusion arising from this.
> > > 
> > > yes, this definitely makes sense as we need to distinguish
> > > 
> > > 
> > > > 
> > > > > > - rename xen_virtio_setup_dma_ops to something more generic, like
> > > > > >     xen_grants_setup_dma_ops
> > > > > > 
> > > > > > And that's pretty much it.
> > > > > + likely renaming everything in that patch series not to mention
> > > > > virtio
> > > > > (mostly related to xen-virtio.c internals).
> > > > > 
> > > > > 
> > > > > Stefano, thank you for clarifying Christoph's point.
> > > > > 
> > > > > Well, I am not against going this direction. Could we please make a
> > > > > decision on this? @Juergen, what is your opinion?
> > > > Yes, why not.
> > > 
> > > ok, thank you for confirming.
> > > 
> > > 
> > > > 
> > > > Maybe rename xen-virtio.c to grant-dma.c?
> > > 
> > > Personally I don't mind.
> > > 
> > > 
> > > > I'd keep the XEN_VIRTIO related config option, as this will be the
> > > > normal
> > > > use
> > > > case. grant-dma.c should be covered by a new hidden config option
> > > > XEN_GRANT_DMA
> > > > selected by XEN_VIRTIO.
> > > 
> > > I got it, ok
> > > 
> > > 
> > > > 
> > > > CONFIG_XEN_VIRTIO should still guard
> > > > xen_has_restricted_virtio_memory_access().
> > > 
> > > ok
> > > 
> > > 
> > > So a few questions to clarify:
> > > 
> > > 1. What is the best place to keep "xen,dev-domid" binding's description
> > > now? I
> > > think that proposed in current series place
> > > (Documentation/devicetree/bindings/virtio/) is not good fit now.
> > I would probably add it to the existing
> > Documentation/devicetree/bindings/arm/xen.txt.
> > 
> > 
> > > 2. I assume the logic in the current patch will remain the same, I mean we
> > > will still assign Xen grant DMA ops from xen_setup_dma_ops() here?
> > Yes I think so
> 
> 
> Stefano, thank you for clarifying!
> 
> 
> Regarding new naming scheme...
> 
> As there is an existing Kconfig option XEN_GRANT_DMA_ALLOC used for different
> purpose, we need to clarify naming scheme here a bit to avoid possible
> confusion.
> 
> For example, I am happy with proposed by Juergen ...
> 
> ... Kconfig option: XEN_GRANT_DMA_OPS
> 
> and
> 
> ... file: grant-dma-ops.c

I think that's fine by me

^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2022-04-20 22:49 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-14 19:19 [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Oleksandr Tyshchenko
2022-04-14 19:19 ` [RFC PATCH 1/6] xen/grants: support allocating consecutive grants Oleksandr Tyshchenko
2022-04-14 19:19 ` [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen Oleksandr Tyshchenko
2022-04-14 19:43   ` H. Peter Anvin
2022-04-15 15:20     ` Oleksandr
2022-04-15 22:01   ` Stefano Stabellini
2022-04-17 17:02     ` Oleksandr
2022-04-18 19:11       ` Stefano Stabellini
2022-04-19  6:21         ` Juergen Gross
2022-04-19  6:37           ` Oleksandr
2022-04-14 19:19 ` [RFC PATCH 3/6] dt-bindings: xen: Add xen,dev-domid property description for xen-virtio layer Oleksandr Tyshchenko
2022-04-15 22:01   ` Stefano Stabellini
2022-04-17 17:24     ` Oleksandr
2022-04-14 19:19 ` [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer Oleksandr Tyshchenko
2022-04-15 22:02   ` Stefano Stabellini
2022-04-17 18:21     ` Oleksandr
2022-04-18 19:11       ` Stefano Stabellini
2022-04-19  6:58         ` Juergen Gross
2022-04-19  7:07           ` Oleksandr
2022-04-16  6:05   ` Christoph Hellwig
2022-04-17 18:39     ` Oleksandr
2022-04-14 19:19 ` [RFC PATCH 5/6] arm/xen: Introduce xen_setup_dma_ops() Oleksandr Tyshchenko
2022-04-15 22:02   ` Stefano Stabellini
2022-04-17 18:43     ` Oleksandr
2022-04-14 19:19 ` [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests Oleksandr Tyshchenko
2022-04-15 22:02   ` Stefano Stabellini
2022-04-16  6:07     ` Christoph Hellwig
2022-04-17 21:05       ` Oleksandr
2022-04-18 19:11         ` Stefano Stabellini
2022-04-19 12:17           ` Oleksandr
2022-04-19 14:48             ` Juergen Gross
2022-04-19 17:11               ` Oleksandr
2022-04-20  0:23                 ` Stefano Stabellini
2022-04-20  9:00                   ` Oleksandr
2022-04-20 22:49                     ` Stefano Stabellini
2022-04-17 19:20     ` Oleksandr
2022-04-15  7:41 ` [RFC PATCH 0/6] virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer Christoph Hellwig
2022-04-15 10:04   ` Oleksandr
2022-04-15  8:44 ` Michael S. Tsirkin
2022-04-15 15:29   ` Oleksandr

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).