Re: [Xen-devel] [PATCH V3 2/29] VIOMMU: Add vIOMMU helper functions to create, destroy vIOMMU instance

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 21, 2017 at 11:01:43PM -0400, Lan Tianyu wrote:
> This patch is to introduce an abstract layer for arch vIOMMU implementation
> to deal with requests from dom0. Arch vIOMMU code needs to provide callback
> to do create and destroy operation.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  docs/misc/xen-command-line.markdown |   7 ++
>  xen/arch/x86/Kconfig                |   1 +
>  xen/common/Kconfig                  |   3 +
>  xen/common/Makefile                 |   1 +
>  xen/common/domain.c                 |   4 +
>  xen/common/viommu.c                 | 144 
> ++++++++++++++++++++++++++++++++++++
>  xen/include/xen/sched.h             |   8 ++
>  xen/include/xen/viommu.h            |  63 ++++++++++++++++
>  8 files changed, 231 insertions(+)
>  create mode 100644 xen/common/viommu.c
>  create mode 100644 xen/include/xen/viommu.h
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index 9797c8d..dfd1db5 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1825,3 +1825,10 @@ mode.
>  > Default: `true`
>  
>  Permit use of the `xsave/xrstor` instructions.
> +
> +### viommu
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Permit use of viommu interface to create and destroy viommu device model.
> 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/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 39e2614..da32f71 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 5aebcf2..cdb1c9d 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -814,6 +814,10 @@ static void complete_domain_destroy(struct rcu_head 
> *head)
>  
>      sched_destroy_domain(d);
>  
> +#ifdef CONFIG_VIOMMU
> +    viommu_destroy_domain(d);
> +#endif
> +
>      /* Free page used by xen oprofile buffer. */
>  #ifdef CONFIG_XENOPROF
>      free_xenoprof_pages(d);
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> new file mode 100644
> index 0000000..64d91e6
> --- /dev/null
> +++ b/xen/common/viommu.c
> @@ -0,0 +1,144 @@
> +/*
> + * common/viommu.c
> + *
> + * Copyright (c) 2017 Intel Corporation
> + * Author: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> + *
> + * 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 DEFINE_SPINLOCK(type_list_lock);
> +static LIST_HEAD(type_list);
> +
> +struct viommu_type {
> +    uint64_t type;

The comment I've made about type being uint64_t in the other patch
stands here.

> +    struct viommu_ops *ops;
> +    struct list_head node;
> +};
> +
> +int viommu_destroy_domain(struct domain *d)
> +{
> +    int ret;
> +
> +    if ( !d->viommu )
> +        return -EINVAL;

ENODEV would be better.

> +
> +    ret = d->viommu->ops->destroy(d->viommu);
> +    if ( ret < 0 )
> +        return ret;
> +
> +    xfree(d->viommu);
> +    d->viommu = NULL;

Newline preferably.

> +    return 0;
> +}
> +
> +static struct viommu_type *viommu_get_type(uint64_t 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);

Why do you need a lock here, and a list at all?

AFAICT vIOMMU types will never be added at runtime.

> +
> +    return NULL;
> +}
> +
> +int viommu_register_type(uint64_t type, struct viommu_ops *ops)
> +{
> +    struct viommu_type *viommu_type = NULL;
> +
> +    if ( !viommu_enabled() )
> +        return -ENODEV;
> +
> +    if ( viommu_get_type(type) )
> +        return -EEXIST;
> +
> +    viommu_type = xzalloc(struct viommu_type);
> +    if ( !viommu_type )
> +        return -ENOMEM;
> +
> +    viommu_type->type = type;
> +    viommu_type->ops = ops;
> +
> +    spin_lock(&type_list_lock);
> +    list_add_tail(&viommu_type->node, &type_list);
> +    spin_unlock(&type_list_lock);
> +
> +    return 0;
> +}

As mentioned above, I think this viommu_register_type helper could be
avoided. I would rather use a macro similar to REGISTER_SCHEDULER in
order to populate an array at link time, and then just iterate over
it.

> +
> +static int viommu_create(struct domain *d, uint64_t type,
> +                         uint64_t base_address, uint64_t caps,
> +                         uint32_t *viommu_id)

I'm quite sure this doesn't compile, you are adding a static function
here that's not used at all in this patch. Please be careful and don't
introduce patches that will break the build.

> +{
> +    struct viommu *viommu;
> +    struct viommu_type *viommu_type = NULL;
> +    int rc;
> +
> +    /* Only support one vIOMMU per domain. */
> +    if ( d->viommu )
> +        return -E2BIG;
> +
> +    viommu_type = viommu_get_type(type);
> +    if ( !viommu_type )
> +        return -EINVAL;
> +
> +    if ( !viommu_type->ops || !viommu_type->ops->create )
> +        return -EINVAL;

Can this really happen? What's the point in having a iommu_type
without ops or without the create op? I think this should be an ASSERT
instead.

> +
> +    viommu = xzalloc(struct viommu);
> +    if ( !viommu )
> +        return -ENOMEM;
> +
> +    viommu->base_address = base_address;
> +    viommu->caps = caps;
> +    viommu->ops = viommu_type->ops;
> +
> +    rc = viommu->ops->create(d, viommu);
> +    if ( rc < 0 )
> +    {
> +        xfree(viommu);
> +        return rc;
> +    }
> +
> +    d->viommu = viommu;
> +
> +    /* Only support one vIOMMU per domain. */
> +    *viommu_id = 0;
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 5b8f8c6..750f235 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -33,6 +33,10 @@
>  DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t);
>  #endif
>  
> +#ifdef CONFIG_VIOMMU
> +#include <xen/viommu.h>
> +#endif

I would suggest you place the CONFIG_VIOMMU inside of the header
itself.

> +
>  /*
>   * Stats
>   *
> @@ -479,6 +483,10 @@ struct domain
>      rwlock_t vnuma_rwlock;
>      struct vnuma_info *vnuma;
>  
> +#ifdef CONFIG_VIOMMU
> +    struct viommu *viommu;
> +#endif

Shouldn't this go inside of x86/hvm/domain.h? (hvm_domain) PV guests
will certainly never be able to use it.

> +
>      /* Common monitor options */
>      struct {
>          unsigned int guest_request_enabled       : 1;
> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h
> new file mode 100644
> index 0000000..636a2a3
> --- /dev/null
> +++ b/xen/include/xen/viommu.h
> @@ -0,0 +1,63 @@
> +/*
> + * include/xen/viommu.h
> + *
> + * Copyright (c) 2017, Intel Corporation
> + * Author: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> + *
> + * 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 __XEN_VIOMMU_H__
> +#define __XEN_VIOMMU_H__
> +
> +struct viommu;
> +
> +struct viommu_ops {
> +    int (*create)(struct domain *d, struct viommu *viommu);
> +    int (*destroy)(struct viommu *viommu);
> +};
> +
> +struct viommu {
> +    uint64_t base_address;
> +    uint64_t caps;
> +    const struct viommu_ops *ops;
> +    void *priv;
> +};
> +
> +#ifdef CONFIG_VIOMMU

Why do you only protect certain parts of the file with
CONFIG_VIOMMU?

> +extern bool opt_viommu;
> +static inline bool viommu_enabled(void)
> +{
> +    return opt_viommu;
> +}
> +
> +int viommu_register_type(uint64_t type, struct viommu_ops *ops);
> +int viommu_destroy_domain(struct domain *d);
> +#else
> +static inline int viommu_register_type(uint64_t type, struct viommu_ops *ops)
> +{
> +    return -EINVAL;
> +}

Why don't you also provide a dummy viommu_destroy_domain helper to be
used in domain.c?

Thanks, Roger.

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