Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 3/3] xen: don't require virtio with grants for non-PV guests | From | Oleksandr <> | Date | Wed, 22 Jun 2022 12:03:25 +0300 |
| |
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)
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
| |