Re: [Xen-devel] [RFC PATCH V2 1/26] VIOMMU: Add vIOMMU helper functions to create, destroy and query capabilities

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, May 18, 2017 at 01:34:31AM -0400, Lan Tianyu wrote:
> 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@xxxxxxxxx>
> ---
>  xen/arch/x86/setup.c        |   1 +
>  xen/common/Kconfig          |  11 +++
>  xen/common/Makefile         |   1 +
>  xen/common/domain.c         |   3 +
>  xen/common/viommu.c         | 169 
> ++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/viommu.h |  49 +++++++++++++
>  xen/include/xen/sched.h     |   2 +
>  xen/include/xen/viommu.h    |  79 +++++++++++++++++++++
>  8 files changed, 315 insertions(+)
>  create mode 100644 xen/common/viommu.c
>  create mode 100644 xen/include/public/viommu.h
>  create mode 100644 xen/include/xen/viommu.h
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index f7b9278..f204d71 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();
>  
>      smp_prepare_cpus(max_cpus);
>  
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index dc8e876..90e3741 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -73,6 +73,17 @@ config TMEM
>  
>         If unsure, say Y.
>  
> +config VIOMMU
> +     def_bool y
> +     prompt "Xen vIOMMU Support" if EXPERT = "y"
> +     depends on X86
> +     ---help---
> +      Virtual IOMMU provides interrupt remapping function for guest and
> +      it allows guest to boot up more than 255 vcpus which requires interrupt
> +      remapping function.
> +
> +       If unsure, say Y.

Indentation. And this should be disabled by default.

> +
>  config XENOPROF
>       def_bool y
>       prompt "Xen Oprofile Support" if EXPERT = "y"
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 26c5a64..f61e579 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -61,6 +61,7 @@ obj-y += vm_event.o
>  obj-y += vmap.o
>  obj-y += vsprintf.o
>  obj-y += wait.o
> +obj-$(CONFIG_VIOMMU) += viommu.o

Please sort this list alphabetically.

>  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 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;
> +
>      return d;
>  
>   fail:
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> new file mode 100644
> index 0000000..eadcecb
> --- /dev/null
> +++ b/xen/common/viommu.c
> @@ -0,0 +1,169 @@
> +/*
> + * 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/types.h>
> +#include <xen/sched.h>
> +#include <xen/spinlock.h>
[...]
> +
> +void viommu_unregister_type(u64 type)
> +{
> +    struct viommu_type *viommu_type = viommu_get_type(type);
> +
> +    if ( viommu_type )
> +    {
> +        spin_lock(&type_list_lock);
> +        list_del(&viommu_type->node);
> +        spin_unlock(&type_list_lock);
> +
> +        xfree(viommu_type);
> +    }
> +}
> +

Is the unregister functions really useful?

Xen doesn't support module. And I don't see the unregister function used
in your series.

If you don't actually care about dynamically loading and unloading ops,
I think the code can be simplified.

> +int viommu_create(struct domain *d, u64 type, u64 base_address,
> +                  u64 length, u64 caps)
> +{
> +    struct viommu_info *info = &d->viommu;
> +    struct viommu *viommu;
> +    struct viommu_type *viommu_type = NULL;
> +    int rc;
> +
> +    viommu_type = viommu_get_type(type);
> +    if ( !viommu_type )
> +        return -EFAULT;

EINVAL

> +
> +    if ( !info || info->nr_viommu >= NR_VIOMMU_PER_DOMAIN
> +        || !viommu_type->ops || !viommu_type->ops->create )
> +        return -EINVAL;
> +
> +    viommu = xzalloc(struct viommu);
> +    if ( !viommu )
> +        return -ENOMEM;
> +
> +    viommu->base_address = base_address;
> +    viommu->length = length;
> +    viommu->caps = caps;
> +    viommu->ops = viommu_type->ops;
> +    viommu->viommu_id = info->nr_viommu;
> +
> +    info->viommu[info->nr_viommu] = viommu;
> +    info->nr_viommu++;
> +
> +    rc = viommu->ops->create(d, viommu);
> +    if ( rc < 0 )
> +    {

Presumably you also need to reset info->viommu in the error path.

Or even better, use viommu_destroy to handle the error path.

> +        xfree(viommu);
> +        return rc;
> +    }
> +
> +    return viommu->viommu_id;
> +}
> +
> +int viommu_destroy(struct domain *d, u32 viommu_id)
> +{
> +    struct viommu_info *info = &d->viommu;
> +
> +    if ( !info || viommu_id > info->nr_viommu || !info->viommu[viommu_id] )
> +        return -EINVAL;
> +
> +    if ( info->viommu[viommu_id]->ops->destroy(info->viommu[viommu_id]) )
> +        return -EFAULT;
> +
> +    info->viommu[viommu_id] = NULL;
> +    return 0;
> +}
> +
> +u64 viommu_query_caps(struct domain *d, u64 type)
> +{
> +    struct viommu_type *viommu_type = viommu_get_type(type);
> +
> +    if ( !viommu_type )
> +        return -EFAULT;

EINVAL

> +
> +    return viommu_type->ops->query_caps(d);
> +}
> +
> +int __init viommu_setup(void)
> +{
> +    INIT_LIST_HEAD(&type_list);
> +    spin_lock_init(&type_list_lock);
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * End:
> + */
> diff --git a/xen/include/public/viommu.h b/xen/include/public/viommu.h
> new file mode 100644
> index 0000000..a4f7c47
> --- /dev/null
> +++ b/xen/include/public/viommu.h
> @@ -0,0 +1,49 @@
> +/*
> + * viommu.h
> + *
> + * Virtual IOMMU information
> + *
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without restriction,
> + * including without limitation the rights to use, copy, modify, merge,
> + * publish, distribute, sublicense, and/or sell copies of the Software,
> + * and to permit persons to whom the Software is furnished to do so,
> + * subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> + * IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef __XEN_PUBLIC_VIOMMU_H__
> +#define __XEN_PUBLIC_VIOMMU_H__
> +
> +/* VIOMMU type */
> +#define VIOMMU_TYPE_INTEL_VTD     (1 << 0)
> +
> +/* VIOMMU capabilities*/
> +#define VIOMMU_CAP_IRQ_REMAPPING  (1 << 0)
> +

1U in both cases.

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