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, Oct 19, 2017 at 02:31:22PM +0800, Lan Tianyu wrote:
> On 2017年10月18日 22:05, Roger Pau Monné wrote:
> > On Thu, Sep 21, 2017 at 11:01:43PM -0400, Lan Tianyu wrote:
> >> +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.

If it's used in the next patch, then simply introduce it there.

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

Since ops is already a pointer I would rather do

ASSERT(viommu_type->ops && viommu_type->ops->create);

Or else you risk a NULL pointer dereference.

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

For all platforms supporting HVM, for PV I don't think it makes sense.
Since AFAIK ARM guest type is also HVM I would rather introduce this
field in the hvm_domain structure rather than the generic domain
structure.

You might want to wait for feedback from others regarding this issue.

Roger.

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