|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 02/28] VIOMMU: Add vIOMMU framework and vIOMMU domctl
On Fri, Nov 17, 2017 at 02:22:09PM +0800, Chao Gao wrote:
> From: Lan Tianyu <tianyu.lan@xxxxxxxxx>
>
> This patch is to introduce an abstract layer for arch vIOMMU implementation
> and vIOMMU domctl to deal with requests from tool stack. Arch vIOMMU code
> needs to
> provide callback. vIOMMU domctl supports to create vIOMMU instance in
> hypervisor
> and it will be destroyed during destroying domain.
>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v4:
> - introduce REGISTER_VIOMMU() to register viommu types and ops.
> - remove unneeded domctl interface to destroy viommu.
> ---
> docs/misc/xen-command-line.markdown | 7 ++
> xen/arch/x86/Kconfig | 1 +
> xen/arch/x86/hvm/hvm.c | 3 +
> xen/arch/x86/xen.lds.S | 3 +
> xen/common/Kconfig | 3 +
> xen/common/Makefile | 1 +
> xen/common/domctl.c | 7 ++
> xen/common/viommu.c | 125
> ++++++++++++++++++++++++++++++++++++
> xen/include/asm-x86/hvm/domain.h | 3 +
> xen/include/public/domctl.h | 31 +++++++++
> xen/include/xen/viommu.h | 69 ++++++++++++++++++++
> 11 files changed, 253 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 eb4995e..d097382 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1836,3 +1836,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.
I'm not sure about the point of having this command line option, this
is a guest feature and just setting it from the config file should be
enough IMHO.
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 64955dc..df254e4 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -25,6 +25,7 @@ config X86
> select HAS_UBSAN
> select NUMA
> select VGA
> + select VIOMMU
>
> config ARCH_DEFCONFIG
> string
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 205b4cb..964418a 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -36,6 +36,7 @@
> #include <xen/rangeset.h>
> #include <xen/monitor.h>
> #include <xen/warning.h>
> +#include <xen/viommu.h>
> #include <asm/shadow.h>
> #include <asm/hap.h>
> #include <asm/current.h>
> @@ -693,6 +694,8 @@ void hvm_domain_relinquish_resources(struct domain *d)
> pmtimer_deinit(d);
> hpet_deinit(d);
> }
> +
> + viommu_destroy_domain(d);
This returns a value, but you ignore it (read below for how I think
this should be solved).
> }
>
> void hvm_domain_destroy(struct domain *d)
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index d5e8821..7f8d2b8 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -231,6 +231,9 @@ SECTIONS
> __start_schedulers_array = .;
> *(.data.schedulers)
> __end_schedulers_array = .;
> + __start_viommus_array = .;
> + *(.data.viommus)
> + __end_viommus_array = .;
This should be protected with #ifdef CONFIG_VIOMMU. And please place
it at the end of the rodata section (AFAICT there's no need for it to
be in the data.read_mostly section).
> } :text
>
> .data : { /* Data */
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 103ef44..62aaa76 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -52,6 +52,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 66cc2c8..182b3ac 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/domctl.c b/xen/common/domctl.c
> index 3c6fa4e..9c5651d 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -25,6 +25,7 @@
> #include <xen/paging.h>
> #include <xen/hypercall.h>
> #include <xen/vm_event.h>
> +#include <xen/viommu.h>
> #include <xen/monitor.h>
> #include <asm/current.h>
> #include <asm/irq.h>
> @@ -1155,6 +1156,12 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
> u_domctl)
>
> op->u.set_gnttab_limits.maptrack_frames);
> break;
>
> + case XEN_DOMCTL_viommu_op:
> + ret = viommu_domctl(d, &op->u.viommu_op);
> + if ( !ret )
> + copyback = 1;
> + break;
> +
> default:
> ret = arch_do_domctl(op, d, u_domctl);
> break;
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> new file mode 100644
> index 0000000..fd8b7fd
> --- /dev/null
> +++ b/xen/common/viommu.c
> @@ -0,0 +1,125 @@
> +/*
> + * common/viommu.c
> + *
> + * Copyright (c) 2017 Intel Corporation
> + * Author: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> + *
> + * 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>
> +
> +extern const struct viommu_ops *__start_viommus_array[],
> *__end_viommus_array[];
> +#define NUM_VIOMMU_TYPE (__end_viommus_array - __start_viommus_array)
> +#define viommu_type_array __start_viommus_array
> +
> +int viommu_destroy_domain(struct domain *d)
IMHO I would rather prefer the destroy operation to not be allowed to
fail, that would allow switching it return value to void.
The more that you simply ignore the return value in
hvm_domain_relinquish_resources.
> +{
> + struct viommu *viommu = d->arch.hvm_domain.viommu;
> + int ret;
> +
> + if ( !viommu )
> + return -ENODEV;
> +
> + ret = viommu->ops->destroy(viommu);
> + if ( ret < 0 )
> + return ret;
> +
> + xfree(viommu);
> + d->arch.hvm_domain.viommu = NULL;
> +
> + return 0;
> +}
> +
> +static const struct viommu_ops *viommu_get_ops(uint8_t type)
> +{
> + int i;
unsigned int.
> +
> + for ( i = 0; i < NUM_VIOMMU_TYPE; i++)
> + {
> + if ( viommu_type_array[i]->type == type )
> + return viommu_type_array[i];
> + }
> +
> + return NULL;
> +}
> +
> +static int viommu_create(struct domain *d, uint8_t type,
> + uint64_t base_address, uint64_t caps,
> + uint32_t *viommu_id)
> +{
> + struct viommu *viommu;
> + const struct viommu_ops *viommu_ops = NULL;
You can initialize viommu_ops here directly:
const struct viommu_ops *viommu_ops = viommu_get_ops(type);
> + int rc;
> +
> + /* Only support one vIOMMU per domain. */
> + if ( d->arch.hvm_domain.viommu )
> + return -E2BIG;
EEXIST is maybe better?
> +
> + viommu_ops = viommu_get_ops(type);
> + if ( !viommu_ops )
> + return -EINVAL;
> +
> + ASSERT(viommu_ops->create);
> +
> + viommu = xzalloc(struct viommu);
> + if ( !viommu )
> + return -ENOMEM;
> +
> + viommu->base_address = base_address;
> + viommu->caps = caps;
> + viommu->ops = viommu_ops;
> +
> + rc = viommu_ops->create(d, viommu);
> + if ( rc < 0 )
> + {
> + xfree(viommu);
> + return rc;
> + }
> +
> + d->arch.hvm_domain.viommu = viommu;
> +
> + /* Only support one vIOMMU per domain. */
> + *viommu_id = 0;
> + return 0;
> +}
> +
> +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op)
> +{
> + int rc;
> +
> + switch ( op->cmd )
> + {
> + case XEN_DOMCTL_viommu_create:
> + rc = viommu_create(d, op->u.create.type, op->u.create.base_address,
> + op->u.create.capabilities, &op->u.create.id);
> + break;
Newline.
> + default:
> + return -ENOSYS;
-EOPNOTSUPP
> + }
> +
> + return rc;
> +}
> +
> +/*
> + * 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/asm-x86/hvm/domain.h
> b/xen/include/asm-x86/hvm/domain.h
> index 7f128c0..fcd3482 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -21,6 +21,7 @@
> #define __ASM_X86_HVM_DOMAIN_H__
>
> #include <xen/iommu.h>
> +#include <xen/viommu.h>
> #include <asm/hvm/irq.h>
> #include <asm/hvm/vpt.h>
> #include <asm/hvm/vlapic.h>
> @@ -196,6 +197,8 @@ struct hvm_domain {
> struct vmx_domain vmx;
> struct svm_domain svm;
> };
> +
> + struct viommu *viommu;
Are you sure this will compile if you don't select CONFIG_VIOMMU?
AFAICT struct viommu is only defined if VIOMMU is selected, so you
should add something like...
> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h
> new file mode 100644
> index 0000000..a859d80
> --- /dev/null
> +++ b/xen/include/xen/viommu.h
> @@ -0,0 +1,69 @@
> +/*
> + * include/xen/viommu.h
> + *
> + * Copyright (c) 2017, Intel Corporation
> + * Author: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> + *
> + * 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__
> +
> +#ifdef CONFIG_VIOMMU
> +
> +struct viommu;
> +
> +struct viommu_ops {
> + uint8_t type;
> + 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;
> +};
> +
> +#define REGISTER_VIOMMU(x) static const struct viommu_ops *x##_entry \
> + __used_section(".data.viommus") = &x;
> +
> +
> +int viommu_register_type(uint8_t type, struct viommu_ops *ops);
> +int viommu_destroy_domain(struct domain *d);
> +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op);
> +#else
...
struct viommu {
};
here.
> +static inline int viommu_destroy_domain(struct domain *d)
> +{
> + return -EINVAL;
ENODEV if you really have to return an error here, note that I think
destroy should not return anything.
> +}
> +static inline
> +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op)
The style should be:
static inline int viommu_domctl(struct domain *d,
struct xen_domctl_viommu_op *op)
{
...
> +{
> + return false;
Urg, no please. This should be -EOPNOTSUP.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |