[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Juergen Gross <jgross@xxxxxxxx>
> 
> 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@xxxxxxxx>
> ---
>  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@xxxxxxxx>
> + */
> +
> +#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@xxxxxxxx>");
> +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
> 



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.