Re: [PATCH v4 02/28] VIOMMU: Add vIOMMU framework and vIOMMU domctl

From: Chao Gao <chao.gao@intel.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Lan Tianyu <tianyu.lan@intel.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v4 02/28] VIOMMU: Add vIOMMU framework and vIOMMU domctl
Date: Sat, 10 Feb 2018 00:13:45 +0800	[thread overview]
Message-ID: <20180209161345.GB30322@skl-4s-chao.sh.intel.com> (raw)
In-Reply-To: <20180209143357.nyimj76niaxj4x42@MacBook-Pro-de-Roger.local>

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@intel.com>
>> 
>> 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@intel.com>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>> 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@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>
>> +
>> +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@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__
>> +
>> +#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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel