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

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

On Wed, Aug 23, 2017 at 03:10:48PM +0800, Lan Tianyu wrote:
> On 2017年08月22日 23:27, Roger Pau Monné wrote:
> > On Thu, Aug 17, 2017 at 08:22:16PM -0400, Lan Tianyu wrote:
> >> +int viommu_register_type(u64 type, struct viommu_ops * ops)
> >> +{
> >> +    struct viommu_type *viommu_type = NULL;
> >> +
> >> +    if ( !viommu_enabled() )
> >> +        return -EINVAL;
> > 
> > ENODEV is maybe better here.
> 
> Will update.
> 
> > 
> >> +
> >> +    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;
> >> +}
> > 
> > Hm, I haven't seen the usage of this function, but from the looks of
> > it it seems like you want to use something similar to what's used by
> > the scheduler in order to register vIOMMU types.
> > 
> > See the logic around REGISTER_SCHEDULER in xen/sched-if.h.
> 
> Each vIOMMU devel model needs to call viommu_register_type() to register
> its vIOMMU type. Tool stack will pass viommu_type and vIOMMU abstract
> layer call vendor's vIOMMU callback to query vIOMMU capabilities, create
> and destroy() vIOMMU. We just need to maintain type list.
> REGISTER_SCHEDULER seems to heavy which reserve a scheduler array in the
> Xen data section.

I will let the maintainers comment, but I find it easier to use
something similar to REGISTER_SCHEDULER rather than have each IOMMU
type have it's initialization function hooked up in the init procedure
and calling viommu_register_type.

From an abstract point of view I don't really see how this is
different from a scheduler for example, the IOMMU types are fixed and
compiled into Xen, and they need to be initialized/registered at boot
time.

> >> +static inline bool viommu_enabled(void) { return opt_viommu; }
> > 
> > I think those static inline functions should also follow the coding
> > standard.
> 
> Yes, I am not sure what wrong here. Could you elaborate? Thanks.

IMHO they should be:

static inline bool viommu_enabled(void)
{
    return opt_viommu;
}

Roger.

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