Messages in this thread | | | Subject | Re: [PATCH v3 3/3] xen: don't require virtio with grants for non-PV guests | From | Oleksandr <> | Date | Wed, 22 Jun 2022 18:18:26 +0300 |
| |
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 <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.
I am happy with that explanation.
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > > Juergen
-- Regards,
Oleksandr Tyshchenko
| |