Re: [PATCH V2 1/25] VIOMMU: Add vIOMMU helper functions to create, destroy and query capabilities

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Lan, Tianyu
> Sent: Friday, August 18, 2017 8:22 AM
> 
> This patch is to introduct an abstract layer for arch vIOMMU
> implementation
> to deal with requests from dom0. Arch vIOMMU code needs to provide
> callback
> to perform create, destroy and query capabilities operation.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  xen/arch/x86/Kconfig     |   1 +
>  xen/arch/x86/setup.c     |   1 +
>  xen/common/Kconfig       |   3 +
>  xen/common/Makefile      |   1 +
>  xen/common/domain.c      |   3 +
>  xen/common/viommu.c      | 165
> +++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/sched.h  |   2 +
>  xen/include/xen/viommu.h |  71 ++++++++++++++++++++
>  8 files changed, 247 insertions(+)
>  create mode 100644 xen/common/viommu.c
>  create mode 100644 xen/include/xen/viommu.h
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 30c2769..1f1de96 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -23,6 +23,7 @@ config X86
>  	select HAS_PDX
>  	select NUMA
>  	select VGA
> +	select VIOMMU
> 
>  config ARCH_DEFCONFIG
>  	string
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index db5df69..68f1631 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1513,6 +1513,7 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
>      early_msi_init();
> 
>      iommu_setup();    /* setup iommu if available */
> +    viommu_setup();

start_xen is about physical bits, why placing viommu stuff here?

> 
>      smp_prepare_cpus(max_cpus);
> 
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index dc8e876..2ad2c8d 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -49,6 +49,9 @@ config HAS_CHECKPOLICY
>  	string
>  	option env="XEN_HAS_CHECKPOLICY"
> 
> +config VIOMMU
> +	bool
> +
>  config KEXEC
>  	bool "kexec support"
>  	default y
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 26c5a64..852553d 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -56,6 +56,7 @@ obj-y += time.o
>  obj-y += timer.o
>  obj-y += trace.o
>  obj-y += version.o
> +obj-$(CONFIG_VIOMMU) += viommu.o
>  obj-y += virtual_region.o
>  obj-y += vm_event.o
>  obj-y += vmap.o
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index b22aacc..d1f9b10 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -396,6 +396,9 @@ struct domain *domain_create(domid_t domid,
> unsigned int domcr_flags,
>          spin_unlock(&domlist_update_lock);
>      }
> 
> +    if ( (err = viommu_init_domain(d)) != 0 )
> +        goto fail;
> +

though viommu framework is common, it's better to have arch
specific code invoke above function based on their own 
requirement (e.g. if any dependency required to enforce)

>      return d;
> 
>   fail:
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> new file mode 100644
> index 0000000..6874d9f
> --- /dev/null
> +++ b/xen/common/viommu.c
> @@ -0,0 +1,165 @@
> +/*
> + * common/viommu.c
> + *
> + * Copyright (c) 2017 Intel Corporation
> + * Author: Lan Tianyu <tianyu.lan@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/sched.h>
> +#include <xen/spinlock.h>
> +#include <xen/types.h>
> +#include <xen/viommu.h>
> +
> +bool __read_mostly opt_viommu;
> +boolean_param("viommu", opt_viommu);
> +
> +static spinlock_t type_list_lock;
> +static struct list_head type_list;
> +
> +struct viommu_type {
> +    u64 type;

please add some comment what 'type' stands for.

> +    struct viommu_ops *ops;
> +    struct list_head node;
> +};
> +
> +int viommu_init_domain(struct domain *d)
> +{
> +    d->viommu.nr_viommu = 0;
> +    return 0;
> +}
> +
> +static struct viommu_type *viommu_get_type(u64 type)
> +{
> +    struct viommu_type *viommu_type = NULL;
> +
> +    spin_lock(&type_list_lock);
> +    list_for_each_entry( viommu_type, &type_list, node )
> +    {
> +        if ( viommu_type->type == type )
> +        {
> +            spin_unlock(&type_list_lock);
> +            return viommu_type;
> +        }
> +    }
> +    spin_unlock(&type_list_lock);
> +
> +    return NULL;
> +}
> +
> +int viommu_register_type(u64 type, struct viommu_ops * ops)
> +{
> +    struct viommu_type *viommu_type = NULL;
> +
> +    if ( !viommu_enabled() )
> +        return -EINVAL;

shouldn't above be domain specific check?

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel