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 <[email protected]>
Signed-off-by: Juergen Gross <[email protected]>
---
V2:
- remove command line parameter (Christoph Hellwig)
V3:
- rebase to callback method
---
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);
+}
+
void xen_grant_setup_dma_ops(struct device *dev)
{
struct xen_grant_dma_data *data;
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 80546960f8b7..98c399a960a3 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -5,6 +5,7 @@
#include <linux/percpu.h>
#include <linux/notifier.h>
#include <linux/efi.h>
+#include <linux/virtio_anchor.h>
#include <xen/features.h>
#include <asm/xen/interface.h>
#include <xen/interface/vcpu.h>
@@ -217,6 +218,7 @@ static inline void xen_preemptible_hcall_end(void) { }
#ifdef CONFIG_XEN_GRANT_DMA_OPS
void xen_grant_setup_dma_ops(struct device *dev);
bool xen_is_grant_dma_device(struct device *dev);
+bool xen_virtio_mem_acc(struct virtio_device *dev);
#else
static inline void xen_grant_setup_dma_ops(struct device *dev)
{
@@ -225,6 +227,10 @@ static inline bool xen_is_grant_dma_device(struct device *dev)
{
return false;
}
+static inline bool xen_virtio_mem_acc(struct virtio_device *dev)
+{
+ return false;
+}
#endif /* CONFIG_XEN_GRANT_DMA_OPS */
#endif /* INCLUDE_XEN_OPS_H */
diff --git a/include/xen/xen.h b/include/xen/xen.h
index ac5a144c6a65..a99bab817523 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -52,14 +52,6 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
extern u64 xen_saved_max_mem_size;
#endif
-#include <linux/virtio_anchor.h>
-
-static inline void xen_set_restricted_virtio_memory_access(void)
-{
- if (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain())
- virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
-}
-
#ifdef CONFIG_XEN_UNPOPULATED_ALLOC
int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages);
void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages);
--
2.35.3
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 <[email protected]>
> Signed-off-by: Juergen Gross <[email protected]>
> ---
> 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)
diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 8973fc1..fc01424 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -12,8 +12,6 @@
#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>
@@ -289,14 +287,6 @@ 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);
-}
-
void xen_grant_setup_dma_ops(struct device *dev)
{
struct xen_grant_dma_data *data;
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 98c399a..a9ae51b 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -6,6 +6,7 @@
#include <linux/notifier.h>
#include <linux/efi.h>
#include <linux/virtio_anchor.h>
+#include <linux/virtio.h>
#include <xen/features.h>
#include <asm/xen/interface.h>
#include <xen/interface/vcpu.h>
@@ -218,7 +219,13 @@ static inline void xen_preemptible_hcall_end(void) { }
#ifdef CONFIG_XEN_GRANT_DMA_OPS
void xen_grant_setup_dma_ops(struct device *dev);
bool xen_is_grant_dma_device(struct device *dev);
-bool xen_virtio_mem_acc(struct virtio_device *dev);
+static inline 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);
+}
#else
static inline void xen_grant_setup_dma_ops(struct device *dev)
{
> +
> void xen_grant_setup_dma_ops(struct device *dev)
> {
> struct xen_grant_dma_data *data;
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index 80546960f8b7..98c399a960a3 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -5,6 +5,7 @@
> #include <linux/percpu.h>
> #include <linux/notifier.h>
> #include <linux/efi.h>
> +#include <linux/virtio_anchor.h>
> #include <xen/features.h>
> #include <asm/xen/interface.h>
> #include <xen/interface/vcpu.h>
> @@ -217,6 +218,7 @@ static inline void xen_preemptible_hcall_end(void) { }
> #ifdef CONFIG_XEN_GRANT_DMA_OPS
> void xen_grant_setup_dma_ops(struct device *dev);
> bool xen_is_grant_dma_device(struct device *dev);
> +bool xen_virtio_mem_acc(struct virtio_device *dev);
> #else
> static inline void xen_grant_setup_dma_ops(struct device *dev)
> {
> @@ -225,6 +227,10 @@ static inline bool xen_is_grant_dma_device(struct device *dev)
> {
> return false;
> }
> +static inline bool xen_virtio_mem_acc(struct virtio_device *dev)
> +{
> + return false;
> +}
> #endif /* CONFIG_XEN_GRANT_DMA_OPS */
>
> #endif /* INCLUDE_XEN_OPS_H */
> diff --git a/include/xen/xen.h b/include/xen/xen.h
> index ac5a144c6a65..a99bab817523 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -52,14 +52,6 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
> extern u64 xen_saved_max_mem_size;
> #endif
>
> -#include <linux/virtio_anchor.h>
> -
> -static inline void xen_set_restricted_virtio_memory_access(void)
> -{
> - if (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain())
> - virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
> -}
> -
> #ifdef CONFIG_XEN_UNPOPULATED_ALLOC
> int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages);
> void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages);
--
Regards,
Oleksandr Tyshchenko
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 <[email protected]>
>> Signed-off-by: Juergen Gross <[email protected]>
>> ---
>> 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.
Juergen
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 <[email protected]>
>>> Signed-off-by: Juergen Gross <[email protected]>
>>> ---
>>> 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 <[email protected]>
>
>
>
> Juergen
--
Regards,
Oleksandr Tyshchenko