Re: [Xen-devel] [PATCH V2 12/25] x86/vvtd: Add MMIO handler for VVTD

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

On Wed, Aug 09, 2017 at 04:34:13PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> This patch adds VVTD MMIO handler to deal with MMIO access.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  xen/drivers/passthrough/vtd/vvtd.c | 114 
> +++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c 
> b/xen/drivers/passthrough/vtd/vvtd.c
> index 353fafe..94680e6 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -50,6 +50,38 @@ struct vvtd {
>      struct page_info *regs_page;
>  };
>  
> +#define __DEBUG_VVTD__
> +#ifdef __DEBUG_VVTD__

Why do you need this define? You can use NDEBUG which is the global
Xen debug define.

> +extern unsigned int vvtd_debug_level;
> +#define VVTD_DBG_INFO     1
> +#define VVTD_DBG_TRANS    (1<<1)
> +#define VVTD_DBG_RW       (1<<2)
> +#define VVTD_DBG_FAULT    (1<<3)
> +#define VVTD_DBG_EOI      (1<<4)
> +#define VVTD_DEBUG(lvl, _f, _a...) do { \
> +    if ( vvtd_debug_level & lvl ) \
> +        printk("VVTD %s:" _f "\n", __func__, ## _a);    \
> +} while(0)
> +#else
> +#define VVTD_DEBUG(fmt...) do {} while(0)
> +#endif
> +
> +unsigned int vvtd_debug_level __read_mostly;
> +integer_param("vvtd_debug", vvtd_debug_level);

I think this should be a top level option for viommu, not a vtd
specific one, like it's done for iommu. I would prefer to have
something like:

viommu=verbose,[...]

So that we can add further options to it in the future, and that
should be shared between all the vIOMMU implementations.

> +
> +struct vvtd *domain_vvtd(struct domain *d)
> +{
> +    struct viommu_info *info = &d->viommu;
> +
> +    BUILD_BUG_ON(NR_VIOMMU_PER_DOMAIN != 1);
> +    return (info && info->viommu[0]) ? info->viommu[0]->priv : NULL;
> +}
> +
> +static inline struct vvtd *vcpu_vvtd(struct vcpu *v)
> +{
> +    return domain_vvtd(v->domain);
> +}

Do you really need the above helper? Seems quite trivial to directly
open code the call to domain_vvtd.

> +
>  static inline void vvtd_set_reg(struct vvtd *vtd, uint32_t reg,
>                                  uint32_t value)
>  {
> @@ -76,6 +108,87 @@ static inline uint8_t vvtd_get_reg_byte(struct vvtd *vtd, 
> uint32_t reg)
>      vvtd_set_reg(vvtd, (reg) + 4, (val) >> 32); \
>  } while(0)
>  
> +static int vvtd_range(struct vcpu *v, unsigned long addr)

bool and maybe vvtd_in_range is more descriptive.

> +{
> +    struct vvtd *vvtd = vcpu_vvtd(v);
> +
> +    if ( vvtd )
> +        return (addr >= vvtd->base_addr) &&
> +               (addr < vvtd->base_addr + PAGE_SIZE);

So here you simply hardcode PAGE_SIZE, which makes me think that the
size parameter is indeed not needed.

> +    return 0;
> +}
> +
> +static int vvtd_read(struct vcpu *v, unsigned long addr,
> +                     unsigned int len, unsigned long *pval)
> +{
> +    struct vvtd *vvtd = vcpu_vvtd(v);
> +    unsigned int offset = addr - vvtd->base_addr;
> +    unsigned int offset_aligned = offset & ~3;

This is not needed IMHO.

> +
> +    VVTD_DEBUG(VVTD_DBG_RW, "READ INFO: offset %x len %d.", offset, len);
> +
> +    if ( !pval )
> +        return X86EMUL_UNHANDLEABLE;

I don't think you can get a NULL pval here.

> +
> +    if ( (offset & 3) || ((len != 4) && (len != 8)) )

Do you really intend to allow non-aligned 8 byte accesses? If so my
previous recommendation to use a union for the vvtd struct data field
is moot.

> +    {
> +        VVTD_DEBUG(VVTD_DBG_RW, "Alignment or length is not canonical");
> +        return X86EMUL_UNHANDLEABLE;

IMHO you should not return X86EMUL_UNHANDLEABLE here. The read does
indeed belong to the vIOMMU region, so what does bare-metal hardware
do when a non-aligned or non size compliant read is performed?

> +    }
> +
> +    if ( len == 4 )
> +        *pval = vvtd_get_reg(vvtd, offset_aligned);

You can just use offset here and below, the check that you do above
guarantees that offset & 3 == 0 (ie: at this point offset ==
offset_aligned).

> +    else
> +        vvtd_get_reg_quad(vvtd, offset_aligned, *pval);

Newline

> +    return X86EMUL_OKAY;
> +}
> +
> +static int vvtd_write(struct vcpu *v, unsigned long addr,
> +                      unsigned int len, unsigned long val)
> +{
> +    struct vvtd *vvtd = vcpu_vvtd(v);
> +    unsigned int offset = addr - vvtd->base_addr;
> +    unsigned int offset_aligned = offset & ~0x3;
> +    int ret;
> +
> +    VVTD_DEBUG(VVTD_DBG_RW, "WRITE INFO: offset %x len %d val %lx.",
> +               offset, len, val);
> +
> +    if ( (offset & 3) || ((len != 4) && (len != 8)) )

Same comment about alignment.

> +    {
> +        VVTD_DEBUG(VVTD_DBG_RW, "Alignment or length is not canonical");
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    ret = X86EMUL_UNHANDLEABLE;

Same here, you should not return X86EMUL_UNHANDLEABLE but instead do
what the native hardware would do, which I guess is to just ignore the
write?

> +    if ( len == 4 )
> +    {
> +        switch ( offset_aligned )
> +        {
> +        case DMAR_IEDATA_REG:
> +        case DMAR_IEADDR_REG:
> +        case DMAR_IEUADDR_REG:
> +        case DMAR_FEDATA_REG:
> +        case DMAR_FEADDR_REG:
> +        case DMAR_FEUADDR_REG:
> +            vvtd_set_reg(vvtd, offset_aligned, val);
> +            ret = X86EMUL_OKAY;
> +            break;
> +
> +        default:
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +static const struct hvm_mmio_ops vvtd_mmio_ops = {
> +    .check = vvtd_range,
> +    .read = vvtd_read,
> +    .write = vvtd_write
> +};
> +
>  static void vvtd_reset(struct vvtd *vvtd, uint64_t capability)
>  {
>      uint64_t cap = DMA_CAP_NFR | DMA_CAP_SLLPS | DMA_CAP_FRO |
> @@ -122,6 +235,7 @@ static int vvtd_create(struct domain *d, struct viommu 
> *viommu)
>      vvtd->length = viommu->length;
>      vvtd->domain = d;
>      vvtd->status = VIOMMU_STATUS_DEFAULT;
> +    register_mmio_handler(d, &vvtd_mmio_ops);

Newline.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel