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

From: Juergen Gross
Date: Fri Jun 17 2022 - 01:41:45 EST


On 16.06.22 22:33, Oleksandr wrote:

On 16.06.22 11:56, Juergen Gross wrote:

Hello Juergen, all


On 16.06.22 09:31, Oleksandr wrote:

On 16.06.22 08:37, 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.

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)
---
  drivers/xen/Kconfig | 9 +++++++++
  include/xen/xen.h   | 2 +-
  2 files changed, 10 insertions(+), 1 deletion(-)

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/include/xen/xen.h b/include/xen/xen.h
index 0780a81e140d..4d4188f20337 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -56,7 +56,7 @@ extern u64 xen_saved_max_mem_size;
  static inline void xen_set_restricted_virtio_memory_access(void)
  {
-    if (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain())
+    if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain())
          platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);


Looks like, the flag will be *always* set for paravirtualized guests even if CONFIG_XEN_VIRTIO disabled.

Maybe we should clarify the check?


if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_pv_domain())

     platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);


Yes, we should. I had the function in grant-dma-ops.c in V1, and could drop the
CONFIG_XEN_VIRTIO dependency for that reason.

I'll wait for more comments before sending V3, though.

ok



Please note, I am happy with current patch and it works in my Arm64 based environment.

Just one moment to consider.


As it was already mentioned earlier in current thread the PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS (former arch_has_restricted_virtio_memory_access()) is not per device but about the whole guest. Being set it makes VIRTIO_F_ACCESS_PLATFORM and VIRTIO_F_VERSION_1 features mandatory for *all* virtio devices in the guest.

The question is “Do we want/need to lift this restriction for some devices (which backends are trusted so can access all guest memory) at the same time”?

No, if you need some virtio devices to not use grants, then don't set
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS.

Please see my answer to Stefano's alternative solution for my idea how to
resolve this via a per-device setting.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature