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

[Jan Beulich]

>>> On 30.10.17 at 02:51,  wrote:
> On 2017年10月18日 22:05, Roger Pau Monné wrote:
>>> +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(_list_lock);
>>> > +list_add_tail(_type->node, _list);
>>> > +spin_unlock(_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.
>> 
> 
> Hi Jan:
>   Could you help to check whether REGISTER_SCHEDULER is right direction
> for vIOMMU? It needs to change Xen lds layout. From my view, a list to
> manage vIOMMU device model types will be more easy and this maybe a
> common solution.

I think the suggested approach is generally the neater one; there
may be a few other things we could convert to a similar model, to
clean up code. Hence yes, unless there are strong reasons against
it, I agree with Roger.

Jan

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

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

[Roger Pau Monné]

On Mon, Oct 30, 2017 at 09:41:23AM +0800, Lan Tianyu wrote:
> On 2017年10月25日 09:43, Lan Tianyu wrote:
> >> 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.
> >> > 
> > This sounds reasonable.
> > 
> >> > You might want to wait for feedback from others regarding this issue.
> >> > 
> > I discussed with Julien before. He hoped no to add viommu code for ARM
> > first.So struct hvm_domain seems to be better place since it's arch
> > specific definition and only add struct viommu for struct hvm_domain of x86.
> 
> Hi Roger:
>   If PV guest needs PV IOMMU support, struct iommu should be put  into
> struct domain and it can be reused by full-virtualization and PV iommu.
> Malcolm Crossley sent out RFC patch of pv iommu before. I found it also
> needs to change struct domain.
> 
> https://lists.xenproject.org/archives/html/xen-devel/2016-02/msg01441.html

This patch series is from February 2016: almost two years old and
there's been no further repost.

If this can indeed be shared with a future pv-iommu work, have you
checked whether the current structure data and hooks would be
suitable for a pv-iommu implementation?

I would rather prefer to move the viommu structure from hvm_domain to
the generic domain struct when it's actually needed (ie: when pv-iommu
is implemented) rather than doing it here.

Roger.

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

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

[Lan Tianyu]

On 2017年10月18日 22:05, Roger Pau Monné wrote:
>> +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(_list_lock);
>> > +list_add_tail(_type->node, _list);
>> > +spin_unlock(_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.
> 

Hi Jan:
Could you help to check whether REGISTER_SCHEDULER is right direction
for vIOMMU? It needs to change Xen lds layout. From my view, a list to
manage vIOMMU device model types will be more easy and this maybe a
common solution.

-- 
Best regards
Tianyu Lan

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

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

[Lan Tianyu]

On 2017年10月25日 09:43, Lan Tianyu wrote:
>> 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.
>> > 
> This sounds reasonable.
> 
>> > You might want to wait for feedback from others regarding this issue.
>> > 
> I discussed with Julien before. He hoped no to add viommu code for ARM
> first.So struct hvm_domain seems to be better place since it's arch
> specific definition and only add struct viommu for struct hvm_domain of x86.

Hi Roger:
If PV guest needs PV IOMMU support, struct iommu should be put  into
struct domain and it can be reused by full-virtualization and PV iommu.
Malcolm Crossley sent out RFC patch of pv iommu before. I found it also
needs to change struct domain.

https://lists.xenproject.org/archives/html/xen-devel/2016-02/msg01441.html


-- 
Best regards
Tianyu Lan

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

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

[Lan Tianyu]

On 2017年10月19日 16:47, Roger Pau Monné wrote:
> 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.
> 

This sounds reasonable.

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

I discussed with Julien before. He hoped no to add viommu code for ARM
first.So struct hvm_domain seems to be better place since it's arch
specific definition and only add struct viommu for struct hvm_domain of x86.

-- 
Best regards
Tianyu Lan

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

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

[Roger Pau Monné]

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Lan Tianyu]

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(_list_lock);
>> +list_for_each_entry( viommu_type, _list, node )
>> +{
>> +if ( viommu_type->type == type )
>> +{
>> +spin_unlock(_list_lock);
>> +return viommu_type;
>> +}
>> +}
>> +spin_unlock(_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(_list_lock);
>> +list_add_tail(_type->node, _list);
>> +spin_unlock(_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 
>> +#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 000..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 

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

[Roger Pau Monné]

On Thu, Sep 21, 2017 at 11:01:43PM -0400, Lan Tianyu wrote:
> This patch is to introduce an abstract layer for arch vIOMMU implementation
> to deal with requests from dom0. Arch vIOMMU code needs to provide callback
> to do create and destroy operation.
> 
> Signed-off-by: Lan Tianyu 
> ---
>  docs/misc/xen-command-line.markdown |   7 ++
>  xen/arch/x86/Kconfig|   1 +
>  xen/common/Kconfig  |   3 +
>  xen/common/Makefile |   1 +
>  xen/common/domain.c |   4 +
>  xen/common/viommu.c | 144 
> 
>  xen/include/xen/sched.h |   8 ++
>  xen/include/xen/viommu.h|  63 
>  8 files changed, 231 insertions(+)
>  create mode 100644 xen/common/viommu.c
>  create mode 100644 xen/include/xen/viommu.h
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index 9797c8d..dfd1db5 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1825,3 +1825,10 @@ mode.
>  > Default: `true`
>  
>  Permit use of the `xsave/xrstor` instructions.
> +
> +### viommu
> +> `= `
> +
> +> Default: `false`
> +
> +Permit use of viommu interface to create and destroy viommu device model.
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 30c2769..1f1de96 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -23,6 +23,7 @@ config X86
>   select HAS_PDX
>   select NUMA
>   select VGA
> + select VIOMMU
>  
>  config ARCH_DEFCONFIG
>   string
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index dc8e876..2ad2c8d 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -49,6 +49,9 @@ config HAS_CHECKPOLICY
>   string
>   option env="XEN_HAS_CHECKPOLICY"
>  
> +config VIOMMU
> + bool
> +
>  config KEXEC
>   bool "kexec support"
>   default y
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 39e2614..da32f71 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -56,6 +56,7 @@ obj-y += time.o
>  obj-y += timer.o
>  obj-y += trace.o
>  obj-y += version.o
> +obj-$(CONFIG_VIOMMU) += viommu.o
>  obj-y += virtual_region.o
>  obj-y += vm_event.o
>  obj-y += vmap.o
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 5aebcf2..cdb1c9d 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -814,6 +814,10 @@ static void complete_domain_destroy(struct rcu_head 
> *head)
>  
>  sched_destroy_domain(d);
>  
> +#ifdef CONFIG_VIOMMU
> +viommu_destroy_domain(d);
> +#endif
> +
>  /* Free page used by xen oprofile buffer. */
>  #ifdef CONFIG_XENOPROF
>  free_xenoprof_pages(d);
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> new file mode 100644
> index 000..64d91e6
> --- /dev/null
> +++ b/xen/common/viommu.c
> @@ -0,0 +1,144 @@
> +/*
> + * common/viommu.c
> + *
> + * Copyright (c) 2017 Intel Corporation
> + * Author: Lan Tianyu 
> + *
> + * 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 .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +bool __read_mostly opt_viommu;
> +boolean_param("viommu", opt_viommu);
> +
> +static DEFINE_SPINLOCK(type_list_lock);
> +static LIST_HEAD(type_list);
> +
> +struct viommu_type {
> +uint64_t type;

The comment I've made about type being uint64_t in the other patch
stands here.

> +struct viommu_ops *ops;
> +struct list_head node;
> +};
> +
> +int viommu_destroy_domain(struct domain *d)
> +{
> +int ret;
> +
> +if ( !d->viommu )
> +return -EINVAL;

ENODEV would be better.

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

Newline preferably.

> +return 0;
> +}
> +
> +static struct viommu_type *viommu_get_type(uint64_t type)
> +{
> +struct viommu_type *viommu_type = NULL;
> +
> +spin_lock(_list_lock);
> +list_for_each_entry( viommu_type, _list, node )
> +{
> +if ( viommu_type->type == type )
> +{
> +spin_unlock(_list_lock);
> +return viommu_type;
> +}
> +}
> +spin_unlock(_list_lock);

Why do you need a lock here, and a list at all?

AFAICT vIOMMU types will never be added at runtime.

> +
> +return 

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

[Lan Tianyu]

This patch is to introduce an abstract layer for arch vIOMMU implementation
to deal with requests from dom0. Arch vIOMMU code needs to provide callback
to do create and destroy operation.

Signed-off-by: Lan Tianyu 
---
 docs/misc/xen-command-line.markdown |   7 ++
 xen/arch/x86/Kconfig|   1 +
 xen/common/Kconfig  |   3 +
 xen/common/Makefile |   1 +
 xen/common/domain.c |   4 +
 xen/common/viommu.c | 144 
 xen/include/xen/sched.h |   8 ++
 xen/include/xen/viommu.h|  63 
 8 files changed, 231 insertions(+)
 create mode 100644 xen/common/viommu.c
 create mode 100644 xen/include/xen/viommu.h

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 9797c8d..dfd1db5 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1825,3 +1825,10 @@ mode.
 > Default: `true`
 
 Permit use of the `xsave/xrstor` instructions.
+
+### viommu
+> `= `
+
+> Default: `false`
+
+Permit use of viommu interface to create and destroy viommu device model.
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 30c2769..1f1de96 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -23,6 +23,7 @@ config X86
select HAS_PDX
select NUMA
select VGA
+   select VIOMMU
 
 config ARCH_DEFCONFIG
string
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index dc8e876..2ad2c8d 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -49,6 +49,9 @@ config HAS_CHECKPOLICY
string
option env="XEN_HAS_CHECKPOLICY"
 
+config VIOMMU
+   bool
+
 config KEXEC
bool "kexec support"
default y
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 39e2614..da32f71 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -56,6 +56,7 @@ obj-y += time.o
 obj-y += timer.o
 obj-y += trace.o
 obj-y += version.o
+obj-$(CONFIG_VIOMMU) += viommu.o
 obj-y += virtual_region.o
 obj-y += vm_event.o
 obj-y += vmap.o
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 5aebcf2..cdb1c9d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -814,6 +814,10 @@ static void complete_domain_destroy(struct rcu_head *head)
 
 sched_destroy_domain(d);
 
+#ifdef CONFIG_VIOMMU
+viommu_destroy_domain(d);
+#endif
+
 /* Free page used by xen oprofile buffer. */
 #ifdef CONFIG_XENOPROF
 free_xenoprof_pages(d);
diff --git a/xen/common/viommu.c b/xen/common/viommu.c
new file mode 100644
index 000..64d91e6
--- /dev/null
+++ b/xen/common/viommu.c
@@ -0,0 +1,144 @@
+/*
+ * common/viommu.c
+ *
+ * Copyright (c) 2017 Intel Corporation
+ * Author: Lan Tianyu 
+ *
+ * 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 .
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+bool __read_mostly opt_viommu;
+boolean_param("viommu", opt_viommu);
+
+static DEFINE_SPINLOCK(type_list_lock);
+static LIST_HEAD(type_list);
+
+struct viommu_type {
+uint64_t type;
+struct viommu_ops *ops;
+struct list_head node;
+};
+
+int viommu_destroy_domain(struct domain *d)
+{
+int ret;
+
+if ( !d->viommu )
+return -EINVAL;
+
+ret = d->viommu->ops->destroy(d->viommu);
+if ( ret < 0 )
+return ret;
+
+xfree(d->viommu);
+d->viommu = NULL;
+return 0;
+}
+
+static struct viommu_type *viommu_get_type(uint64_t type)
+{
+struct viommu_type *viommu_type = NULL;
+
+spin_lock(_list_lock);
+list_for_each_entry( viommu_type, _list, node )
+{
+if ( viommu_type->type == type )
+{
+spin_unlock(_list_lock);
+return viommu_type;
+}
+}
+spin_unlock(_list_lock);
+
+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(_list_lock);
+list_add_tail(_type->node, _list);
+spin_unlock(_list_lock);
+
+return 0;
+}
+
+static int viommu_create(struct domain *d, uint64_t type,
+