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

[Lan Tianyu <tianyu.lan@intel.com>]

On 2017年10月18日 22:05, Roger Pau Monné wrote:
> On Thu, Sep 21, 2017 at 11:01:43PM -0400, Lan Tianyu wrote:
>> +int viommu_destroy_domain(struct domain *d)
>> +{
>> +    int ret;
>> +
>> +    if ( !d->viommu )
>> +        return -EINVAL;
> 
> ENODEV would be better.

OK. Will update.

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

OK.

> 
>> +    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.

Yes, will remove it.

> 
>> +
>> +    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.

This function will be used in the next patch. "DOMCTL: Introduce new
DOMCTL commands for vIOMMU support.". So this doesn't break patchset
build. Will combine these two patches to avoid such issue.


> 
>> +{
>> +    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.

How about add ASSERT(viommu_type->ops->create) here?

> 
>> +
>> +    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.

vIOMMU framework should be generic for all platforms and so didn't put
this in arch/x86.

> 
>> +
>>      /* 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@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 __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?

After some considerations, CONFIG_VIOMMU should protect all field(new
structure definition and function declaration) in the file except some
dummy function. This will help to remove some CONFIG_VIOMMU check in
other places.

> 
>> +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?
> 

After above change,  I think we just need viommu_destroy_domain() and
viommu_domctl() which is called in the common code path for x86 and ARM.

-- 
Best regards
Tianyu Lan

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