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 <[email protected]>
---
.../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);
+}
+
static inline dma_addr_t grant_to_dma(grant_ref_t grant)
{
return XEN_GRANT_DMA_ADDR_OFF | ((dma_addr_t)grant << PAGE_SHIFT);
diff --git a/include/xen/xen.h b/include/xen/xen.h
index 0780a81e140d..e0b1d534366f 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -52,13 +52,11 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
extern u64 xen_saved_max_mem_size;
#endif
-#include <linux/platform-feature.h>
-
-static inline void xen_set_restricted_virtio_memory_access(void)
-{
- if (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain())
- platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
-}
+#ifdef CONFIG_XEN_GRANT_DMA_OPS
+void xen_set_restricted_virtio_memory_access(void);
+#else
+static inline void xen_set_restricted_virtio_memory_access(void) { }
+#endif
#ifdef CONFIG_XEN_UNPOPULATED_ALLOC
int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages);
--
2.35.3
On 15-06-22, 10:48, 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 <[email protected]>
> ---
> .../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(-)
Thanks for the quick fix.
With CONFIG_DEBUG_SECTION_MISMATCH=y, this generates a warning.
WARNING: modpost: vmlinux.o(.text+0x7a8270): Section mismatch in reference from the function xen_set_restricted_virtio_memory_access() to the variable .init.data:xen_virtio_grants
The function xen_set_restricted_virtio_memory_access() references
the variable __initdata xen_virtio_grants.
This is often because xen_set_restricted_virtio_memory_access lacks a __initdata
annotation or the annotation of xen_virtio_grants is wrong.
This can be fixed by:
diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index d1fae789dfad..1099097b4515 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -42,7 +42,7 @@ static __init int parse_use_grants(char *arg)
}
early_param("xen_virtio_grant", parse_use_grants);
-void xen_set_restricted_virtio_memory_access(void)
+void __init 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()))
With that:
Tested-by: Viresh Kumar <[email protected]>
--
viresh
On 15.06.22 11:25, Viresh Kumar wrote:
> On 15-06-22, 10:48, 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 <[email protected]>
>> ---
>> .../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(-)
>
> Thanks for the quick fix.
>
> With CONFIG_DEBUG_SECTION_MISMATCH=y, this generates a warning.
>
> WARNING: modpost: vmlinux.o(.text+0x7a8270): Section mismatch in reference from the function xen_set_restricted_virtio_memory_access() to the variable .init.data:xen_virtio_grants
> The function xen_set_restricted_virtio_memory_access() references
> the variable __initdata xen_virtio_grants.
> This is often because xen_set_restricted_virtio_memory_access lacks a __initdata
> annotation or the annotation of xen_virtio_grants is wrong.
Silly me. Thanks for the notice.
>
> This can be fixed by:
>
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index d1fae789dfad..1099097b4515 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -42,7 +42,7 @@ static __init int parse_use_grants(char *arg)
> }
> early_param("xen_virtio_grant", parse_use_grants);
>
> -void xen_set_restricted_virtio_memory_access(void)
> +void __init 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()))
>
> With that:
>
> Tested-by: Viresh Kumar <[email protected]>
>
Thanks,
Juergen
On 15.06.22 13:24, Christoph Hellwig wrote:
> I don't think this command line mess is a good idea. Instead the
> brand new grant devices need to be properly advertised in the device
> tree, and any device that isn't a grant device doesn't need
> VIRTIO_F_ACCESS_PLATFORM. No need for a command line or user
> intervention.
And on x86?
Juergen
On Wed, Jun 15, 2022 at 01:26:27PM +0200, Juergen Gross wrote:
> On 15.06.22 13:24, Christoph Hellwig wrote:
> > I don't think this command line mess is a good idea. Instead the
> > brand new grant devices need to be properly advertised in the device
> > tree, and any device that isn't a grant device doesn't need
> > VIRTIO_F_ACCESS_PLATFORM. No need for a command line or user
> > intervention.
>
> And on x86?
ACPI tables or whatever mechanism you otherwise use to advertise grant
based DMA. But that is irrelevant for now, as the code in mainline
only supports DT on arm/arm64 anyay.
I don't think this command line mess is a good idea. Instead the
brand new grant devices need to be properly advertised in the device
tree, and any device that isn't a grant device doesn't need
VIRTIO_F_ACCESS_PLATFORM. No need for a command line or user
intervention.
On Wed, Jun 15, 2022 at 01:39:01PM +0200, Juergen Gross wrote:
> No, it doesn't. I'm working on a qemu patch series enabling the qemu
> based backends to support grants with virtio. The code is working fine
> on x86, too (apart from the fact that the backends aren't ready yet).
The code right now in mainline only ever sets the ops for DMA. So
I can't see how you could make it work.
On 15.06.22 13:28, Christoph Hellwig wrote:
> On Wed, Jun 15, 2022 at 01:26:27PM +0200, Juergen Gross wrote:
>> On 15.06.22 13:24, Christoph Hellwig wrote:
>>> I don't think this command line mess is a good idea. Instead the
>>> brand new grant devices need to be properly advertised in the device
>>> tree, and any device that isn't a grant device doesn't need
>>> VIRTIO_F_ACCESS_PLATFORM. No need for a command line or user
>>> intervention.
>>
>> And on x86?
>
> ACPI tables or whatever mechanism you otherwise use to advertise grant
> based DMA. But that is irrelevant for now, as the code in mainline
> only supports DT on arm/arm64 anyay.
No, it doesn't. I'm working on a qemu patch series enabling the qemu
based backends to support grants with virtio. The code is working fine
on x86, too (apart from the fact that the backends aren't ready yet).
I'd be fine to just drop the command line parameter, but I think a
guest admin should be able to specify that he doesn't want the backend
to be able to map arbitrary memory of the guest in order to add
another line of defense.
Juergen
On 15.06.22 13:54, Christoph Hellwig wrote:
> On Wed, Jun 15, 2022 at 01:39:01PM +0200, Juergen Gross wrote:
>> No, it doesn't. I'm working on a qemu patch series enabling the qemu
>> based backends to support grants with virtio. The code is working fine
>> on x86, too (apart from the fact that the backends aren't ready yet).
>
> The code right now in mainline only ever sets the ops for DMA. So
> I can't see how you could make it work.
Ah, you are right. I was using a guest with an older version of the series.
Sorry for the noise.
Juergen
On Wed, Jun 15, 2022 at 02:53:54PM +0200, Juergen Gross wrote:
> On 15.06.22 13:54, Christoph Hellwig wrote:
> > On Wed, Jun 15, 2022 at 01:39:01PM +0200, Juergen Gross wrote:
> > > No, it doesn't. I'm working on a qemu patch series enabling the qemu
> > > based backends to support grants with virtio. The code is working fine
> > > on x86, too (apart from the fact that the backends aren't ready yet).
> >
> > The code right now in mainline only ever sets the ops for DMA. So
> > I can't see how you could make it work.
>
> Ah, you are right. I was using a guest with an older version of the series.
> Sorry for the noise.
No problem. But whatever you end up using to enable the grant DMA
ops n x86 should also require the platform access feature. We already
have that information so we can make use of it.
On 15.06.22 15:43, Christoph Hellwig wrote:
> On Wed, Jun 15, 2022 at 02:53:54PM +0200, Juergen Gross wrote:
>> On 15.06.22 13:54, Christoph Hellwig wrote:
>>> On Wed, Jun 15, 2022 at 01:39:01PM +0200, Juergen Gross wrote:
>>>> No, it doesn't. I'm working on a qemu patch series enabling the qemu
>>>> based backends to support grants with virtio. The code is working fine
>>>> on x86, too (apart from the fact that the backends aren't ready yet).
>>>
>>> The code right now in mainline only ever sets the ops for DMA. So
>>> I can't see how you could make it work.
>>
>> Ah, you are right. I was using a guest with an older version of the series.
>> Sorry for the noise.
>
> No problem. But whatever you end up using to enable the grant DMA
> ops n x86 should also require the platform access feature. We already
> have that information so we can make use of it.
Yes, of course.
Juergen
On 15.06.22 11:48, 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.
>
> 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 <[email protected]>
Thank you for the fix.
I have tested it on Arm64 guest (XEN_HVM_DOMAIN), it works.
With the "__init" fix (pointed out by Viresh) applied you can add my:
Tested-by: Oleksandr Tyshchenko <[email protected]> #Arm64 only
[snip]
--
Regards,
Oleksandr Tyshchenko
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 <[email protected]>
> ---
> .../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;
}