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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jun 29, 2017 at 01:50:33AM -0400, Lan Tianyu wrote:
[...]
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index dc8e876..8ba4f5a 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -73,6 +73,18 @@ config TMEM
>  
>         If unsure, say Y.
>  
> +config VIOMMU
> +     def_bool y
> +     depends on X86

This depends on x86 but the code is in common/. What's the game plan /
expectation here?

>   fail:
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> new file mode 100644
> index 0000000..19ba529
> --- /dev/null
> +++ b/xen/common/viommu.c
> @@ -0,0 +1,163 @@
> +/*
> + * 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>

Please order these files alphabetically.

> +
> +bool_t __read_mostly opt_viommu = 0;

bool and no need to initialise it.

> +boolean_param("viommu", opt_viommu);
> +
> +spinlock_t type_list_lock;

static

> +static struct list_head type_list;
> +
[...]
> +
> +static int viommu_create(struct domain *d, u64 type, u64 base_address,
> +                  u64 length, u64 caps)

Indentation.

> +{
> +    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 -EINVAL;
> +
> +    if ( 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 )
> +    {
> +        xfree(viommu);
> +             info->nr_viommu--;
> +             info->viommu[info->nr_viommu] = NULL;

You have some tabs here.

> +        return rc;
> +    }
> +
> +    return viommu->viommu_id;
> +}
> +
> +static int viommu_destroy(struct domain *d, u32 viommu_id)
> +{
> +    struct viommu_info *info = &d->viommu;
> +
> +    if ( viommu_id > info->nr_viommu || !info->viommu[viommu_id] )

  >= here.

> +        return -EINVAL;
> +
> +    if ( info->viommu[viommu_id]->ops->destroy(info->viommu[viommu_id]) )
> +        return -EFAULT;
> +
> +    info->viommu[viommu_id] = NULL;

You leak the viommu struct.


> +
> +struct viommu_info {
> +    u32 nr_viommu;
> +    struct viommu *viommu[NR_VIOMMU_PER_DOMAIN]; /* viommu array*/
> +};
> +
> +#ifdef CONFIG_VIOMMU
> +extern bool_t opt_viommu;
> +static inline bool_t viommu_enabled(void) { return opt_viommu; }

bool

> +int viommu_init_domain(struct domain *d);
> +int viommu_register_type(u64 type, struct viommu_ops * ops);
> +int viommu_setup(void);
> +#else
> +static inline int viommu_init_domain(struct domain *d) { return 0; }
> +static inline int viommu_register_type(u64 type, struct viommu_ops * ops)
> +{ return 0; }
> +static inline int __init viommu_setup(void) { return 0; }
> +static inline bool_t viommu_enabled(void) { return 0; }


bool and return false.

> +#endif
> +
> +#endif /* __XEN_VIOMMU_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 1.8.3.1
> 

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