Message-ID: <alpine.DEB.2.22.394.2204151235440.915916@ubuntu-linux-20-04-desktop>
Date:   Fri, 15 Apr 2022 15:01:05 -0700 (PDT)
From:   Stefano Stabellini <sstabellini@...nel.org>
To:     Oleksandr Tyshchenko <olekstysh@...il.com>
cc:     xen-devel@...ts.xenproject.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, Juergen Gross <jgross@...e.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Julien Grall <julien@....org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>
Subject: 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@...e.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@...e.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@...e.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@...e.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
> 

Powered by blists - more mailing lists