Re: [PATCH] xen: don't require virtio with grants for non-PV guests
From: Stefano Stabellini
Date: Wed Jun 15 2022 - 19:40:57 EST
On Wed, 15 Jun 2022, Juergen Gross wrote:
> 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.
>
> The setting can be overridden by using the new "xen_virtio_grant"
> command line parameter.
>
> 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")
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> .../admin-guide/kernel-parameters.txt | 6 +++++
> drivers/xen/Kconfig | 9 ++++++++
> drivers/xen/grant-dma-ops.c | 22 +++++++++++++++++++
> include/xen/xen.h | 12 +++++-----
> 4 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 8090130b544b..7960480c6fe4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6695,6 +6695,12 @@
> improve timer resolution at the expense of processing
> more timer interrupts.
>
> + xen_virtio_grant= [XEN]
> + Control whether virtio devices are required to use
> + grants when running as a Xen guest. The default is
> + "yes" for PV guests or when the kernel has been built
> + with CONFIG_XEN_VIRTIO_FORCE_GRANT set.
> +
> xen.balloon_boot_timeout= [XEN]
> The time (in seconds) to wait before giving up to boot
> in case initial ballooning fails to free enough memory.
> 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..d1fae789dfad 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -11,6 +11,7 @@
> #include <linux/dma-map-ops.h>
> #include <linux/of.h>
> #include <linux/pfn.h>
> +#include <linux/platform-feature.h>
> #include <linux/xarray.h>
> #include <xen/xen.h>
> #include <xen/xen-ops.h>
> @@ -27,6 +28,27 @@ static DEFINE_XARRAY(xen_grant_dma_devices);
>
> #define XEN_GRANT_DMA_ADDR_OFF (1ULL << 63)
>
> +static bool __initdata xen_virtio_grants;
> +static bool __initdata xen_virtio_grants_set;
> +static __init int parse_use_grants(char *arg)
> +{
> + if (!strcmp(arg, "yes"))
> + xen_virtio_grants = true;
> + else if (!strcmp(arg, "no"))
> + xen_virtio_grants = false;
> + xen_virtio_grants_set = true;
> +
> + return 0;
> +}
> +early_param("xen_virtio_grant", parse_use_grants);
> +
> +void xen_set_restricted_virtio_memory_access(void)
> +{
> + if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_virtio_grants ||
> + (!xen_virtio_grants_set && xen_pv_domain()))
> + platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> +}
I agree with Christoph on this.
On ARM all guests are HVM guests. Unless I am reading this wrongly, with
this check, the user needs to pass the xen_virtio_grant command line
option or add CONFIG_XEN_VIRTIO_FORCE_GRANT to the build to use virtio
with grants. Instead, it should be automatic.
I am not against adding new command line or compile-time options. But
on ARM we already have all the information we need in device tree with
"iommus" and "xen,grant-dma". We don't need anything more.
On ARM if "xen,grant-dma" is present we need to enable
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS, otherwise we don't.
So I think it should be something like the appended (untested):
- on ARM we call xen_set_restricted_virtio_memory_access if
xen,grant-dma is present in device tree
- on x86 ideally we would have something like xen,grant-dma in a Xen
ACPI table, but for now:
- always restrict for PV guests (no change)
- only restrict for HVM guests if a new cmdline option is passed
- so the command line option is only for Xen x86 HVM guests
- no need for another build-time option
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2522b11e593f..cdd13d08f836 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6730,6 +6730,10 @@
improve timer resolution at the expense of processing
more timer interrupts.
+ xen_virtio_grant= [X86,XEN]
+ Control whether virtio devices are required to use
+ grants when running as a Xen HVM guest.
+
xen.balloon_boot_timeout= [XEN]
The time (in seconds) to wait before giving up to boot
in case initial ballooning fails to free enough memory.
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 1f9c3ba32833..07eb69f9e7df 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -443,8 +443,6 @@ static int __init xen_guest_init(void)
if (!xen_domain())
return 0;
- xen_set_restricted_virtio_memory_access();
-
if (!acpi_disabled)
xen_acpi_guest_init();
else
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 8b71b1dd7639..66b1d9d3d950 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -189,13 +189,27 @@ static int xen_cpu_dead_hvm(unsigned int cpu)
}
static bool no_vector_callback __initdata;
+static bool __initdata xen_virtio_grants;
+static bool __initdata xen_virtio_grants_set;
+static __init int parse_use_grants(char *arg)
+{
+ if (!strcmp(arg, "yes"))
+ xen_virtio_grants = true;
+ else if (!strcmp(arg, "no"))
+ xen_virtio_grants = false;
+ xen_virtio_grants_set = true;
+
+ return 0;
+}
+early_param("xen_virtio_grant", parse_use_grants);
static void __init xen_hvm_guest_init(void)
{
if (xen_pv_domain())
return;
- xen_set_restricted_virtio_memory_access();
+ if (xen_virtio_grant)
+ xen_set_restricted_virtio_memory_access();
init_hvm_pv_info();
diff --git a/drivers/xen/grant-dma-iommu.c b/drivers/xen/grant-dma-iommu.c
index 16b8bc0c0b33..b43a8906ef64 100644
--- a/drivers/xen/grant-dma-iommu.c
+++ b/drivers/xen/grant-dma-iommu.c
@@ -40,6 +40,7 @@ static int grant_dma_iommu_probe(struct platform_device *pdev)
return ret;
platform_set_drvdata(pdev, mmu);
+ xen_set_restricted_virtio_memory_access();
return 0;
}