Messages in this thread | | | Date | Wed, 22 Jun 2022 16:35:22 +0200 | Subject | Re: [PATCH v3 3/3] xen: don't require virtio with grants for non-PV guests | From | Juergen Gross <> |
| |
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@linaro.org> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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 [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |