Re: [PATCH v3 3/3] xen: don't require virtio with grants for non-PV guests

From: Oleksandr
Date: Wed Jun 22 2022 - 11:18:41 EST


On 22.06.22 17:35, Juergen Gross wrote:


Hello Juergen

On 22.06.22 11:03, Oleksandr wrote:

On 22.06.22 09:38, Juergen Gross wrote:

Hello Juergen

Commit fa1f57421e0b ("xen/virtio: Enable restricted memory access using
Xen grant mappings") introduced a new requirement for using virtio
devices: the backend now needs to support the VIRTIO_F_ACCESS_PLATFORM
feature.

This is an undue requirement for non-PV guests, as those can be operated
with existing backends without any problem, as long as those backends
are running in dom0.

Per default allow virtio devices without grant support for non-PV
guests.

On Arm require VIRTIO_F_ACCESS_PLATFORM for devices having been listed
in the device tree to use grants.

Add a new config item to always force use of grants for virtio.

Fixes: fa1f57421e0b ("xen/virtio: Enable restricted memory access using Xen grant mappings")
Reported-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
V2:
- remove command line parameter (Christoph Hellwig)
V3:
- rebase to callback method


Patch looks good, just one NIT ...


---
  arch/arm/xen/enlighten.c     |  4 +++-
  arch/x86/xen/enlighten_hvm.c |  4 +++-
  arch/x86/xen/enlighten_pv.c  |  5 ++++-
  drivers/xen/Kconfig          |  9 +++++++++
  drivers/xen/grant-dma-ops.c  | 10 ++++++++++
  include/xen/xen-ops.h        |  6 ++++++
  include/xen/xen.h            |  8 --------
  7 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 1f9c3ba32833..93c8ccbf2982 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -34,6 +34,7 @@
  #include <linux/timekeeping.h>
  #include <linux/timekeeper_internal.h>
  #include <linux/acpi.h>
+#include <linux/virtio_anchor.h>
  #include <linux/mm.h>
@@ -443,7 +444,8 @@ static int __init xen_guest_init(void)
      if (!xen_domain())
          return 0;
-    xen_set_restricted_virtio_memory_access();
+    if (IS_ENABLED(CONFIG_XEN_VIRTIO))
+        virtio_set_mem_acc_cb(xen_virtio_mem_acc);
      if (!acpi_disabled)
          xen_acpi_guest_init();
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 8b71b1dd7639..28762f800596 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -4,6 +4,7 @@
  #include <linux/cpu.h>
  #include <linux/kexec.h>
  #include <linux/memblock.h>
+#include <linux/virtio_anchor.h>
  #include <xen/features.h>
  #include <xen/events.h>
@@ -195,7 +196,8 @@ static void __init xen_hvm_guest_init(void)
      if (xen_pv_domain())
          return;
-    xen_set_restricted_virtio_memory_access();
+    if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
+ virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
      init_hvm_pv_info();
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index e3297b15701c..5aaae8a77f55 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -31,6 +31,7 @@
  #include <linux/gfp.h>
  #include <linux/edd.h>
  #include <linux/reboot.h>
+#include <linux/virtio_anchor.h>
  #include <xen/xen.h>
  #include <xen/events.h>
@@ -109,7 +110,9 @@ static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
  static void __init xen_pv_init_platform(void)
  {
-    xen_set_restricted_virtio_memory_access();
+    /* PV guests can't operate virtio devices without grants. */
+    if (IS_ENABLED(CONFIG_XEN_VIRTIO))
+ virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
      populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP));
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index bfd5f4f706bc..a65bd92121a5 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -355,4 +355,13 @@ config XEN_VIRTIO
        If in doubt, say n.
+config XEN_VIRTIO_FORCE_GRANT
+    bool "Require Xen virtio support to use grants"
+    depends on XEN_VIRTIO
+    help
+      Require virtio for Xen 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
+      (e.g. qemu or kernel, depending on the virtio device types used).
+
  endmenu
diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index fc0142484001..8973fc1e9ccc 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -12,6 +12,8 @@
  #include <linux/of.h>
  #include <linux/pfn.h>
  #include <linux/xarray.h>
+#include <linux/virtio_anchor.h>
+#include <linux/virtio.h>
  #include <xen/xen.h>
  #include <xen/xen-ops.h>
  #include <xen/grant_table.h>
@@ -287,6 +289,14 @@ bool xen_is_grant_dma_device(struct device *dev)
      return has_iommu;
  }
+bool xen_virtio_mem_acc(struct virtio_device *dev)
+{
+    if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
+        return true;
+
+    return xen_is_grant_dma_device(dev->dev.parent);
+}


    ... I am thinking would it be better to move this to xen/xen-ops.h as grant-dma-ops.c is generic (not only for virtio, although the virtio is the first use-case)

I dislike using a function marked as inline in a function vector.

We could add another module "xen-virtio" for this purpose, but this seems
to be overkill.

I think we should just leave it here and move it later in case more real
virtio dependent stuff is being added.

I am happy with that explanation.

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>





Juergen

--
Regards,

Oleksandr Tyshchenko