Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen

Stefano Stabellini
2022-04-15T22:01:17Z

On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Juergen Gross <jgross@suse.com>
>
> In order to support virtio in Xen guests add a config option enabling
> the user to specify whether in all Xen guests virtio should be able to
> access memory via Xen grant mappings only on the host side.
>
> This applies to fully virtualized guests only, as for paravirtualized
> guests this is mandatory.
>
> This requires to switch arch_has_restricted_virtio_memory_access()
> from a pure stub to a real function on x86 systems (Arm systems are
> not covered by now).
>
> Add the needed functionality by providing a special set of DMA ops
> handling the needed grant operations for the I/O pages.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> arch/x86/mm/init.c | 15 ++++
> arch/x86/mm/mem_encrypt.c | 5 --
> arch/x86/xen/Kconfig | 9 +++
> drivers/xen/Kconfig | 20 ++++++
> drivers/xen/Makefile | 1 +
> drivers/xen/xen-virtio.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++
> include/xen/xen-ops.h | 8 +++
> 7 files changed, 230 insertions(+), 5 deletions(-)
> create mode 100644 drivers/xen/xen-virtio.c
>
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index d8cfce2..526a3b2 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -8,6 +8,8 @@
> #include <linux/kmemleak.h>
> #include <linux/sched/task.h>
>
> +#include <xen/xen.h>
> +
> #include <asm/set_memory.h>
> #include <asm/e820/api.h>
> #include <asm/init.h>
> @@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
> return pages;
> }
> #endif
> +
> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> +int arch_has_restricted_virtio_memory_access(void)
> +{
> + if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
> + return 1;
> + if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
> + return 1;

I think these two checks could be moved to a separate function in a Xen
header, e.g. xen_restricted_virtio_memory_access, and here you could
just

if (xen_restricted_virtio_memory_access())
return 1;

> + return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
> +}
> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
> +#endif
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 50d2099..dda020f 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
> print_mem_encrypt_feature_info();
> }
>
> -int arch_has_restricted_virtio_memory_access(void)
> -{
> - return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
> -}
> -EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> index 85246dd..dffdffd 100644
> --- a/arch/x86/xen/Kconfig
> +++ b/arch/x86/xen/Kconfig
> @@ -92,3 +92,12 @@ config XEN_DOM0
> select X86_X2APIC if XEN_PVH && X86_64
> help
> Support running as a Xen Dom0 guest.
> +
> +config XEN_PV_VIRTIO
> + bool "Xen virtio support for PV guests"
> + depends on XEN_VIRTIO && XEN_PV
> + default y
> + help
> + Support virtio for running as a paravirtualized guest. This will
> + need support on the backend side (qemu or kernel, depending on the
> + virtio device types used).
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 120d32f..fc61f7a 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
> having to balloon out RAM regions in order to obtain physical memory
> space to create such mappings.
>
> +config XEN_VIRTIO
> + bool "Xen virtio support"
> + default n
> + depends on VIRTIO && DMA_OPS
> + select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> + help
> + Enable virtio support for running as Xen guest. Depending on the
> + guest type this will require special support on the backend side
> + (qemu or kernel, depending on the virtio device types used).
> +
> +config XEN_HVM_VIRTIO_GRANT
> + bool "Require virtio for fully virtualized guests to use grant mappings"
> + depends on XEN_VIRTIO && X86_64
> + default y
> + help
> + Require virtio for fully virtualized 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
> + (qemu or kernel, depending on the virtio device types used).

I don't think we need 3 visible kconfig options for this.

In fact, I would only add one: XEN_VIRTIO. We can have any X86 (or ARM)
specific dependencies in the "depends" line under XEN_VIRTIO. And I
don't think we need XEN_HVM_VIRTIO_GRANT as a kconfig option
necessarely. It doesn't seem like some we want as build time option. At
most, it could be a runtime option (like a command line) or a debug
option (like an #define at the top of the source file.)

> endmenu
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 5aae66e..767009c 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -39,3 +39,4 @@ xen-gntalloc-y := gntalloc.o
> xen-privcmd-y := privcmd.o privcmd-buf.o
> obj-$(CONFIG_XEN_FRONT_PGDIR_SHBUF) += xen-front-pgdir-shbuf.o
> obj-$(CONFIG_XEN_UNPOPULATED_ALLOC) += unpopulated-alloc.o
> +obj-$(CONFIG_XEN_VIRTIO) += xen-virtio.o
> diff --git a/drivers/xen/xen-virtio.c b/drivers/xen/xen-virtio.c
> new file mode 100644
> index 00000000..cfd5eda
> --- /dev/null
> +++ b/drivers/xen/xen-virtio.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/******************************************************************************
> + * Xen virtio driver - enables using virtio devices in Xen guests.
> + *
> + * Copyright (c) 2021, Juergen Gross <jgross@suse.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/dma-map-ops.h>
> +#include <linux/pci.h>
> +#include <linux/pfn.h>
> +#include <linux/virtio_config.h>
> +#include <xen/xen.h>
> +#include <xen/grant_table.h>
> +
> +#define XEN_GRANT_ADDR_OFF 0x8000000000000000ULL

NIT: (1ULL << 31)

> +static inline dma_addr_t grant_to_dma(grant_ref_t grant)
> +{
> + return XEN_GRANT_ADDR_OFF | ((dma_addr_t)grant << PAGE_SHIFT);
> +}
> +
> +static inline grant_ref_t dma_to_grant(dma_addr_t dma)
> +{
> + return (grant_ref_t)((dma & ~XEN_GRANT_ADDR_OFF) >> PAGE_SHIFT);
> +}
> +
> +/*
> + * DMA ops for Xen virtio frontends.
> + *
> + * Used to act as a kind of software IOMMU for Xen guests by using grants as
> + * DMA addresses.
> + * Such a DMA address is formed by using the grant reference as a frame
> + * number and setting the highest address bit (this bit is for the backend
> + * to be able to distinguish it from e.g. a mmio address).
> + *
> + * Note that for now we hard wire dom0 to be the backend domain. In order to
> + * support any domain as backend we'd need to add a way to communicate the
> + * domid of this backend, e.g. via Xenstore or via the PCI-device's config
> + * space.

I would add device tree as possible way of domid communication

> + */
> +static void *xen_virtio_dma_alloc(struct device *dev, size_t size,
> + dma_addr_t *dma_handle, gfp_t gfp,
> + unsigned long attrs)
> +{
> + unsigned int n_pages = PFN_UP(size);
> + unsigned int i;
> + unsigned long pfn;
> + grant_ref_t grant;
> + void *ret;
> +
> + ret = (void *)__get_free_pages(gfp, get_order(size));
> + if (!ret)
> + return NULL;
> +
> + pfn = virt_to_pfn(ret);
> +
> + if (gnttab_alloc_grant_reference_seq(n_pages, &grant)) {
> + free_pages((unsigned long)ret, get_order(size));
> + return NULL;
> + }
> +
> + for (i = 0; i < n_pages; i++) {
> + gnttab_grant_foreign_access_ref(grant + i, 0,
> + pfn_to_gfn(pfn + i), 0);
> + }
> +
> + *dma_handle = grant_to_dma(grant);
> +
> + return ret;
> +}
> +
> +static void xen_virtio_dma_free(struct device *dev, size_t size, void *vaddr,
> + dma_addr_t dma_handle, unsigned long attrs)
> +{
> + unsigned int n_pages = PFN_UP(size);
> + unsigned int i;
> + grant_ref_t grant;
> +
> + grant = dma_to_grant(dma_handle);
> +
> + for (i = 0; i < n_pages; i++)
> + gnttab_end_foreign_access_ref(grant + i);
> +
> + gnttab_free_grant_reference_seq(grant, n_pages);
> +
> + free_pages((unsigned long)vaddr, get_order(size));
> +}
> +
> +static struct page *xen_virtio_dma_alloc_pages(struct device *dev, size_t size,
> + dma_addr_t *dma_handle,
> + enum dma_data_direction dir,
> + gfp_t gfp)
> +{
> + WARN_ONCE(1, "xen_virtio_dma_alloc_pages size %ld\n", size);
> + return NULL;
> +}
> +
> +static void xen_virtio_dma_free_pages(struct device *dev, size_t size,
> + struct page *vaddr, dma_addr_t dma_handle,
> + enum dma_data_direction dir)
> +{
> + WARN_ONCE(1, "xen_virtio_dma_free_pages size %ld\n", size);
> +}
> +
> +static dma_addr_t xen_virtio_dma_map_page(struct device *dev, struct page *page,
> + unsigned long offset, size_t size,
> + enum dma_data_direction dir,
> + unsigned long attrs)
> +{
> + grant_ref_t grant;
> +
> + if (gnttab_alloc_grant_references(1, &grant))
> + return 0;
> +
> + gnttab_grant_foreign_access_ref(grant, 0, xen_page_to_gfn(page),
> + dir == DMA_TO_DEVICE);
> + return grant_to_dma(grant) + offset;
> +}
> +
> +static void xen_virtio_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> + size_t size, enum dma_data_direction dir,
> + unsigned long attrs)
> +{
> + grant_ref_t grant;
> +
> + grant = dma_to_grant(dma_handle);
> +
> + gnttab_end_foreign_access_ref(grant);
> +
> + gnttab_free_grant_reference(grant);
> +}
> +
> +static int xen_virtio_dma_map_sg(struct device *dev, struct scatterlist *sg,
> + int nents, enum dma_data_direction dir,
> + unsigned long attrs)
> +{
> + WARN_ONCE(1, "xen_virtio_dma_map_sg nents %d\n", nents);
> + return -EINVAL;
> +}
> +
> +static void xen_virtio_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> + int nents, enum dma_data_direction dir,
> + unsigned long attrs)
> +{
> + WARN_ONCE(1, "xen_virtio_dma_unmap_sg nents %d\n", nents);
> +}

You can implement xen_virtio_dma_map_sg and xen_virtio_dma_unmap_sg
based on xen_virtio_dma_map_page and xen_virtio_dma_unmap_page, like we
do in drivers/xen/swiotlb-xen.c.

> +static int xen_virtio_dma_dma_supported(struct device *dev, u64 mask)
> +{
> + return 1;
> +}
> +
> +static const struct dma_map_ops xen_virtio_dma_ops = {
> + .alloc = xen_virtio_dma_alloc,
> + .free = xen_virtio_dma_free,
> + .alloc_pages = xen_virtio_dma_alloc_pages,
> + .free_pages = xen_virtio_dma_free_pages,
> + .mmap = dma_common_mmap,
> + .get_sgtable = dma_common_get_sgtable,
> + .map_page = xen_virtio_dma_map_page,
> + .unmap_page = xen_virtio_dma_unmap_page,
> + .map_sg = xen_virtio_dma_map_sg,
> + .unmap_sg = xen_virtio_dma_unmap_sg,
> + .dma_supported = xen_virtio_dma_dma_supported,
> +};
> +
> +void xen_virtio_setup_dma_ops(struct device *dev)
> +{
> + dev->dma_ops = &xen_virtio_dma_ops;
> +}
> +EXPORT_SYMBOL_GPL(xen_virtio_setup_dma_ops);
> +
> +MODULE_DESCRIPTION("Xen virtio support driver");
> +MODULE_AUTHOR("Juergen Gross <jgross@suse.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index a3584a3..ae3c1bc 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -221,4 +221,12 @@ static inline void xen_preemptible_hcall_end(void) { }
>
> #endif /* CONFIG_XEN_PV && !CONFIG_PREEMPTION */
>
> +#ifdef CONFIG_XEN_VIRTIO
> +void xen_virtio_setup_dma_ops(struct device *dev);
> +#else
> +static inline void xen_virtio_setup_dma_ops(struct device *dev)
> +{
> +}
> +#endif /* CONFIG_XEN_VIRTIO */
> +
> #endif /* INCLUDE_XEN_OPS_H */
> --
> 2.7.4
>