|
[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, Feb 09, 2018 at 02:33:57PM +0000, Roger Pau Monné wrote:
>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.
Sorry for this. You gave the same remark on our v3. And we promised to
remove it. But we forgot this one. I will remove it.
>
>> 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).
>
Will do.
>> } :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.
>
Got it.
>> +{
>> + 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.
>
Got it.
>> +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.
yes.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |