Re: [Xen-devel] [PATCH v4 07/28] x86/hvm: Introduce a emulated VTD for HVM

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Nov 17, 2017 at 02:22:14PM +0800, Chao Gao wrote:
> This patch adds create/destroy function for the emulated VTD
> and adapts it to the common VIOMMU abstraction.
> 
> As the Makefile is changed here, put all files in alphabetic order
> by this chance.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> 
> ---
> v4:
> - use REGISTER_VIOMMU
> - shrink the size of hvm_hw_vvtd_regs
> - make hvm_hw_vvtd_regs a field inside struct vvtd
> ---
>  xen/drivers/passthrough/vtd/Makefile |   7 +-
>  xen/drivers/passthrough/vtd/iommu.h  |   9 +++
>  xen/drivers/passthrough/vtd/vvtd.c   | 150 
> +++++++++++++++++++++++++++++++++++
>  3 files changed, 163 insertions(+), 3 deletions(-)
>  create mode 100644 xen/drivers/passthrough/vtd/vvtd.c
> 
> diff --git a/xen/drivers/passthrough/vtd/Makefile 
> b/xen/drivers/passthrough/vtd/Makefile
> index f302653..163c7fe 100644
> --- a/xen/drivers/passthrough/vtd/Makefile
> +++ b/xen/drivers/passthrough/vtd/Makefile
> @@ -1,8 +1,9 @@
>  subdir-$(CONFIG_X86) += x86
>  
> -obj-y += iommu.o
>  obj-y += dmar.o
> -obj-y += utils.o
> -obj-y += qinval.o
>  obj-y += intremap.o
> +obj-y += iommu.o
> +obj-y += qinval.o
>  obj-y += quirks.o
> +obj-y += utils.o
> +obj-$(CONFIG_VIOMMU) += vvtd.o
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index db80b31..f2ef3dd 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -47,6 +47,7 @@
>  #define DMAR_IQH_REG            0x80 /* invalidation queue head */
>  #define DMAR_IQT_REG            0x88 /* invalidation queue tail */
>  #define DMAR_IQA_REG            0x90 /* invalidation queue addr */
> +#define DMAR_IECTL_REG          0xa0 /* invalidation event control register 
> */
>  #define DMAR_IRTA_REG           0xb8 /* intr remap */
>  
>  #define OFFSET_STRIDE        (9)
> @@ -89,6 +90,12 @@
>  #define cap_afl(c)        (((c) >> 3) & 1)
>  #define cap_ndoms(c)        (1 << (4 + 2 * ((c) & 0x7)))
>  
> +#define cap_set_num_fault_regs(c)   ((((c) - 1) & 0xff) << 40)
> +#define cap_set_fault_reg_offset(c) ((((c) / 16) & 0x3ff) << 24)
> +#define cap_set_mgaw(c)             ((((c) - 1) & 0x3f) << 16)
> +#define cap_set_sagaw(c)            (((c) & 0x1f) << 8)
> +#define cap_set_ndoms(c)            ((c) & 0x7)
> +
>  /*
>   * Extended Capability Register
>   */
> @@ -114,6 +121,8 @@
>  #define ecap_niotlb_iunits(e)    ((((e) >> 24) & 0xff) + 1)
>  #define ecap_iotlb_offset(e)     ((((e) >> 8) & 0x3ff) * 16)
>  
> +#define ecap_set_mhmv(e)         (((e) & 0xf) << 20)
> +
>  /* IOTLB_REG */
>  #define DMA_TLB_FLUSH_GRANU_OFFSET  60
>  #define DMA_TLB_GLOBAL_FLUSH (((u64)1) << 60)
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c 
> b/xen/drivers/passthrough/vtd/vvtd.c
> new file mode 100644
> index 0000000..9f76ccf
> --- /dev/null
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -0,0 +1,150 @@
> +/*
> + * vvtd.c
> + *
> + * virtualize VTD for HVM.
> + *
> + * Copyright (C) 2017 Chao Gao, Intel Corporation.
> + *
> + * 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 that 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/types.h>
> +#include <xen/viommu.h>
> +#include <xen/xmalloc.h>
> +#include <asm/current.h>
> +#include <asm/hvm/domain.h>
> +
> +#include "iommu.h"
> +
> +/* Supported capabilities by vvtd */
> +#define VVTD_MAX_CAPS VIOMMU_CAP_IRQ_REMAPPING
> +
> +#define VVTD_FRCD_NUM   1ULL
> +#define VVTD_FRCD_START (DMAR_IRTA_REG + 8)
> +#define VVTD_FRCD_END   (VVTD_FRCD_START + VVTD_FRCD_NUM * 16)
> +#define VVTD_MAX_OFFSET VVTD_FRCD_END
> +
> +struct hvm_hw_vvtd {
> +    uint32_t regs[VVTD_MAX_OFFSET/sizeof(uint32_t)];

Unless I'm mistaken this is 208bytes in size, yet you only seem to use
28bytes (from the registers used in vvtd_reset). I guess this is going
to change over the series so all this space is really needed.

Also I think this would be better as:

union hw_vvtd {
    uint32_t regs32[VVTD_MAX_OFFSET/sizeof(uint32_t)];
    uint64_t regs64[VVTD_MAX_OFFSET/sizeof(uint64_t)];
};

> +};
> +
> +struct vvtd {
> +    /* Base address of remapping hardware register-set */
> +    uint64_t base_addr;
> +    /* Point back to the owner domain */
> +    struct domain *domain;
> +
> +    struct hvm_hw_vvtd hw;
> +};
> +
> +/* Setting viommu_verbose enables debugging messages of vIOMMU */
> +bool __read_mostly viommu_verbose;

static?

> +boolean_runtime_param("viommu_verbose", viommu_verbose);
> +
> +#ifndef NDEBUG
> +#define vvtd_info(fmt...) do {                    \
> +    if ( viommu_verbose )                         \
> +        gprintk(XENLOG_INFO, ## fmt);             \
> +} while(0)
> +/*
> + * Use printk and '_G_' prefix because vvtd_debug() may be called
> + * in the context of another domain's vCPU. Don't output 'current'
> + * information to avoid confusion.
> + */
> +#define vvtd_debug(fmt...) do {                   \
> +    if ( viommu_verbose && printk_ratelimit())    \
> +        printk(XENLOG_G_DEBUG fmt);               \

I think printk is already rate-limited if you use _G_, so no need for
the ratelimit call.

> +} while(0)
> +#else
> +#define vvtd_info(...) do {} while(0)
> +#define vvtd_debug(...) do {} while(0)
> +#endif
> +
> +#define VVTD_REG_POS(vvtd, offset) &(vvtd->hw.regs[offset/sizeof(uint32_t)])
> +
> +static inline void vvtd_set_reg(struct vvtd *vvtd, uint32_t reg, uint32_t 
> value)

I don't think you need the vvtd prefix here, and I would leave adding
inline to the compiler discretion:

static void set_reg32(struct vvtd *vvtd, unsigned long offset, uint32_t val)
{
    vvtd->hw.regs32[offset / 4] = val
}

But I think you can even get rid of the helper functions and just use
macros directly, ie:

#define GET_REG(vvtd, offset, size) \
    ((vvtd)->hw.regs ## size [(offset) / size / 8 ])
#define SET_REG(vvtd, offset, val, size) \
    (GET_REG(vvtd, offset, val) = val)

This is better IMHO, and I'm not really sure the SET_REG macro is
really needed, you can just open code GET_REG(...) = val;

> +{
> +    *VVTD_REG_POS(vvtd, reg) = value;
> +}
> +
> +static inline uint32_t vvtd_get_reg(const struct vvtd *vvtd, uint32_t reg)
> +{
> +    return *VVTD_REG_POS(vvtd, reg);
> +}
> +
> +static inline void vvtd_set_reg_quad(struct vvtd *vvtd, uint32_t reg,
> +                                     uint64_t value)
> +{
> +    *(uint64_t*)VVTD_REG_POS(vvtd, reg) = value;
> +}
> +
> +static inline uint64_t vvtd_get_reg_quad(const struct vvtd *vvtd, uint32_t 
> reg)
> +{
> +    return *(uint64_t*)VVTD_REG_POS(vvtd, reg);
> +}
> +
> +static void vvtd_reset(struct vvtd *vvtd)
> +{
> +    uint64_t cap = cap_set_num_fault_regs(VVTD_FRCD_NUM)
> +                   | cap_set_fault_reg_offset(VVTD_FRCD_START)
> +                   | cap_set_mgaw(39) /* maximum guest address width */
> +                   | cap_set_sagaw(2) /* support 3-level page_table */
> +                   | cap_set_ndoms(6); /* support 64K domains */
> +    uint64_t ecap = DMA_ECAP_QUEUED_INVAL | DMA_ECAP_INTR_REMAP | 
> DMA_ECAP_EIM |
> +                    ecap_set_mhmv(0xf);
> +
> +    vvtd_set_reg(vvtd, DMAR_VER_REG, 0x10UL);
> +    vvtd_set_reg_quad(vvtd, DMAR_CAP_REG, cap);
> +    vvtd_set_reg_quad(vvtd, DMAR_ECAP_REG, ecap);
> +    vvtd_set_reg(vvtd, DMAR_FECTL_REG, 0x80000000UL);
> +    vvtd_set_reg(vvtd, DMAR_IECTL_REG, 0x80000000UL);
> +}
> +
> +static int vvtd_create(struct domain *d, struct viommu *viommu)
> +{
> +    struct vvtd *vvtd;
> +
> +    if ( !is_hvm_domain(d) || (viommu->base_address & (PAGE_SIZE - 1)) ||
> +         (~VVTD_MAX_CAPS & viommu->caps) )
> +        return -EINVAL;
> +
> +    vvtd = xzalloc_bytes(sizeof(struct vvtd));

vvtd = xzalloc(struct vvtd);

> +    if ( !vvtd )
> +        return ENOMEM;
> +
> +    vvtd_reset(vvtd);
> +    vvtd->base_addr = viommu->base_address;

I think it would be good to have some check here, so that the vIOMMU
is not for example positioned on top of a RAM region. Ideally you
should check that the gfns [base_address, base_address + size) are
unpopulated.

> +    vvtd->domain = d;
> +
> +    viommu->priv = vvtd;
> +
> +    return 0;
> +}
> +
> +static int vvtd_destroy(struct viommu *viommu)
> +{
> +    struct vvtd *vvtd = viommu->priv;
> +
> +    if ( vvtd )
> +        xfree(vvtd);
> +
> +    return 0;
> +}
> +
> +static const struct viommu_ops vvtd_hvm_vmx_ops = {

Is the vmx needed? vvtd is already Intel specific AFAICT. You could
probably omit the hvm also, so vvtd_ops.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel