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

[Julien Grall <julien.grall@arm.com>]

Hello Tinayu Lan,

On 03/17/2017 11:27 AM, Lan Tianyu wrote:
> This patch is to introduct an abstract layer for arch vIOMMU implementation

s/introduct/introduce/

> 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/common/Makefile          |  1 +
>  xen/common/domain.c          |  3 ++
>  xen/common/viommu.c          | 97 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/viommu.h | 30 ++++++++++++++
>  xen/include/asm-x86/viommu.h | 31 ++++++++++++++
>  xen/include/public/viommu.h  | 38 +++++++++++++++++
>  xen/include/xen/sched.h      |  2 +
>  xen/include/xen/viommu.h     | 62 ++++++++++++++++++++++++++++
>  8 files changed, 264 insertions(+)
>  create mode 100644 xen/common/viommu.c
>  create mode 100644 xen/include/asm-arm/viommu.h
>  create mode 100644 xen/include/asm-x86/viommu.h
>  create mode 100644 xen/include/public/viommu.h
>  create mode 100644 xen/include/xen/viommu.h
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 0fed30b..b58de63 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -60,6 +60,7 @@ obj-y += vm_event.o
>  obj-y += vmap.o
>  obj-y += vsprintf.o
>  obj-y += wait.o
> +obj-y += viommu.o

I see very little point to enable viommu by default on all architecture. 
This is x86 specific and I am yet sure how we would be able to use it on 
ARM as the current series rely on QEMU. Also this is waste space in 
struct domain.

I would prefer if you introduce a Kconfig that would be select by x86 only.

Regards,

>  obj-bin-y += warning.init.o
>  obj-$(CONFIG_XENOPROF) += xenoprof.o
>  obj-y += xmalloc_tlsf.o
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 4492c9c..aafc740 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -398,6 +398,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;
> +
>      return d;
>
>   fail:
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> new file mode 100644
> index 0000000..4c1c788
> --- /dev/null
> +++ b/xen/common/viommu.c
> @@ -0,0 +1,97 @@

[...]

> +int viommu_create(struct domain *d, u64 base_address, u64 length, u64 caps)
> +{
> +    struct viommu_info *info = &d->viommu;
> +    struct viommu *viommu;
> +    int rc;
> +
> +    if ( !info || !info->ops || !info->ops->create
> +	        || info->nr_viommu >= NR_VIOMMU_PER_DOMAIN )
> +        return -EINVAL;
> +
> +    viommu = xzalloc(struct viommu);
> +    if ( !viommu )
> +        return -EFAULT;
> +
> +    viommu->base_address = base_address;
> +    viommu->length = length;
> +    viommu->caps = caps;
> +    viommu->viommu_id = info->nr_viommu;
> +
> +    info->viommu[info->nr_viommu] = viommu;
> +    info->nr_viommu++;
> +
> +    rc = info->ops->create(d, viommu);
> +    if ( rc < 0 ) {

Coding style:

if ( ... )
{

[...]

> diff --git a/xen/include/asm-arm/viommu.h b/xen/include/asm-arm/viommu.h
> new file mode 100644
> index 0000000..ef6a60b
> --- /dev/null
> +++ b/xen/include/asm-arm/viommu.h
> @@ -0,0 +1,30 @@
> +/*
> + * include/asm-arm/viommu.h
> + *
> + * 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/>.
> + *
> + */
> +#ifndef __ARCH_ARM_VIOMMU_H__
> +#define __ARCH_ARM_VIOMMU_H__
> +
> +#include <xen/viommu.h>
> +
> +static inline const struct viommu_ops *viommu_get_ops(void)
> +{
> +    return NULL;
> +}
> +
> +#endif /* __ARCH_ARM_VIOMMU_H__ */

Missing emacs magic.

> diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h
> new file mode 100644
> index 0000000..efb435f
> --- /dev/null
> +++ b/xen/include/asm-x86/viommu.h
> @@ -0,0 +1,31 @@
> +/*
> + * include/asm-arm/viommu.h
> + *
> + * 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/>.
> + *
> + */
> +#ifndef __ARCH_X86_VIOMMU_H__
> +#define __ARCH_X86_VIOMMU_H__
> +
> +#include <xen/viommu.h>
> +#include <asm/types.h>
> +
> +static inline const struct viommu_ops *viommu_get_ops(void)
> +{
> +    return NULL;
> +}
> +
> +#endif /* __ARCH_X86_VIOMMU_H__ */

Ditto

> diff --git a/xen/include/public/viommu.h b/xen/include/public/viommu.h
> new file mode 100644
> index 0000000..ca2419b
> --- /dev/null
> +++ b/xen/include/public/viommu.h
> @@ -0,0 +1,38 @@
> +/*
> + * include/public/viommu.h
> + *
> + * 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.

Public headers sould not be GPLv2 otherwise it will cause some trouble 
for non-GPLv2 OS. See the license in xen/include/public/COPYING.

Regards.

-- 
Julien Grall

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