| Message ID | 1506049330-11196-3-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <tianyu.lan@intel.com> > --- > 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 > +> `= <boolean>` > + > +> 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 0000000..64d91e6 > --- /dev/null > +++ b/xen/common/viommu.c > @@ -0,0 +1,144 @@ > +/* > + * common/viommu.c > + * > + * 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/>. > + */ > + > +#include <xen/sched.h> > +#include <xen/spinlock.h> > +#include <xen/types.h> > +#include <xen/viommu.h> > + > +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(&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. > + > + 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. > +{ > + 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. > + > + 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. > + > /* 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? > +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? Thanks, Roger.
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.
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.
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.
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
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(&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. > 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.
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.
>>> On 30.10.17 at 02:51, <tianyu.lan@intel.com> 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(&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. >> > > 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
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 +> `= <boolean>` + +> 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 0000000..64d91e6 --- /dev/null +++ b/xen/common/viommu.c @@ -0,0 +1,144 @@ +/* + * common/viommu.c + * + * 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/>. + */ + +#include <xen/sched.h> +#include <xen/spinlock.h> +#include <xen/types.h> +#include <xen/viommu.h> + +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(&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); + + 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; +} + +static int viommu_create(struct domain *d, uint64_t type, + uint64_t base_address, uint64_t caps, + uint32_t *viommu_id) +{ + 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; + + 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 + /* * Stats * @@ -479,6 +483,10 @@ struct domain rwlock_t vnuma_rwlock; struct vnuma_info *vnuma; +#ifdef CONFIG_VIOMMU + struct viommu *viommu; +#endif + /* 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 +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; +} +#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: + */
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 <tianyu.lan@intel.com> --- 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