[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V2 13/25] x86/vvtd: Set Interrupt Remapping Table Pointer through GCMD



On Wed, Aug 09, 2017 at 04:34:14PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> Software sets this field to set/update the interrupt remapping table pointer
> used by hardware. The interrupt remapping table pointer is specified through
> the Interrupt Remapping Table Address (IRTA_REG) register.
> 
> This patch emulates this operation and adds some new fields in VVTD to track
> info (e.g. the table's gfn and max supported entries) of interrupt remapping
> table.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  xen/drivers/passthrough/vtd/iommu.h |  9 ++++-
>  xen/drivers/passthrough/vtd/vvtd.c  | 73 
> +++++++++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index 55f3b6e..102b4f3 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -192,9 +192,16 @@
>  #define DMA_GSTS_WBFS   (((u64)1) << 27)
>  #define DMA_GSTS_QIES   (((u64)1) <<26)
>  #define DMA_GSTS_IRES   (((u64)1) <<25)
> -#define DMA_GSTS_SIRTPS (((u64)1) << 24)
> +#define DMA_GSTS_SIRTPS_BIT     24

Those kind of defines are usually suffixed with _SHIFT, not _BIT.

> +#define DMA_GSTS_SIRTPS (((u64)1) << DMA_GSTS_SIRTPS_BIT)
>  #define DMA_GSTS_CFIS   (((u64)1) <<23)
>  
> +/* IRTA_REG */
> +#define DMA_IRTA_ADDR(val)      (val & ~0xfffULL)
> +#define DMA_IRTA_EIME(val)      (!!(val & (1 << 11)))

No need for the outer parentheses.

> +#define DMA_IRTA_S(val)         (val & 0xf)
> +#define DMA_IRTA_SIZE(val)      (1UL << (DMA_IRTA_S(val) + 1))

All those defines above seem kind of magic. Can we maybe get a small
comment describing it's meaning?

>  /* PMEN_REG */
>  #define DMA_PMEN_EPM    (((u32)1) << 31)
>  #define DMA_PMEN_PRS    (((u32)1) << 0)
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c 
> b/xen/drivers/passthrough/vtd/vvtd.c
> index 94680e6..8e8dbe6 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -46,6 +46,13 @@ struct vvtd {
>      uint64_t length;
>      /* Point back to the owner domain */
>      struct domain *domain;
> +    /* Is in Extended Interrupt Mode? */
> +    bool eim;
> +    /* Max remapping entries in IRT */
> +    int irt_max_entry;

unsigned int.

> +    /* Interrupt remapping table base gfn */
> +    uint64_t irt;

If it's a gfn you should use gfn_t.

> +
>      struct hvm_hw_vvtd_regs *regs;
>      struct page_info *regs_page;
>  };
> @@ -82,6 +89,11 @@ static inline struct vvtd *vcpu_vvtd(struct vcpu *v)
>      return domain_vvtd(v->domain);
>  }
>  
> +static inline void __vvtd_set_bit(struct vvtd *vvtd, uint32_t reg, int nr)

No leading underscores, and unsigned int for nr.

> +{
> +    return __set_bit(nr, (uint32_t *)&vvtd->regs->data[reg]);

Why the return?

> +}
> +
>  static inline void vvtd_set_reg(struct vvtd *vtd, uint32_t reg,
>                                  uint32_t value)
>  {
> @@ -108,6 +120,44 @@ 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_handle_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
> +{
> +    uint64_t irta;
> +
> +    if ( !(val & DMA_GCMD_SIRTP) )
> +        return X86EMUL_OKAY;
> +
> +    vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG, irta);
> +    vvtd->irt = DMA_IRTA_ADDR(irta) >> PAGE_SHIFT;
> +    vvtd->irt_max_entry = DMA_IRTA_SIZE(irta);
> +    vvtd->eim = DMA_IRTA_EIME(irta);
> +    VVTD_DEBUG(VVTD_DBG_RW, "Update IR info (addr=%lx eim=%d size=%d).",
> +               vvtd->irt, vvtd->eim, vvtd->irt_max_entry);
> +    __vvtd_set_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_SIRTPS_BIT);
> +
> +    return X86EMUL_OKAY;
> +}
> +
> +static int vvtd_write_gcmd(struct vvtd *vvtd, uint32_t val)
> +{
> +    uint32_t orig = vvtd_get_reg(vvtd, DMAR_GSTS_REG);
> +    uint32_t changed;
> +
> +    orig = orig & 0x96ffffff;    /* reset the one-shot bits */

Some kind of define for this magic value is needed.

> +    changed = orig ^ val;
> +
> +    if ( !changed )
> +        return X86EMUL_OKAY;
> +    if ( (changed & (changed - 1)) )
> +        VVTD_DEBUG(VVTD_DBG_RW, "Guest attempts to update multiple fields "
> +                     "of GCMD_REG in one write transation.");

Since this is a debug message it would be good to print at least the
value of changed, or possibly even better the values of both orig and
val.

> +
> +    if ( changed & DMA_GCMD_SIRTP )
> +        vvtd_handle_gcmd_sirtp(vvtd, val);
> +
> +    return X86EMUL_OKAY;
> +}
> +
>  static int vvtd_range(struct vcpu *v, unsigned long addr)
>  {
>      struct vvtd *vvtd = vcpu_vvtd(v);
> @@ -165,12 +215,18 @@ static int vvtd_write(struct vcpu *v, unsigned long 
> addr,
>      {
>          switch ( offset_aligned )
>          {
> +        case DMAR_GCMD_REG:
> +            ret = vvtd_write_gcmd(vvtd, val);
> +            break;
> +
>          case DMAR_IEDATA_REG:
>          case DMAR_IEADDR_REG:
>          case DMAR_IEUADDR_REG:
>          case DMAR_FEDATA_REG:
>          case DMAR_FEADDR_REG:
>          case DMAR_FEUADDR_REG:
> +        case DMAR_IRTA_REG:
> +        case DMAR_IRTA_REG_HI:
>              vvtd_set_reg(vvtd, offset_aligned, val);
>              ret = X86EMUL_OKAY;
>              break;
> @@ -179,6 +235,20 @@ static int vvtd_write(struct vcpu *v, unsigned long addr,
>              break;
>          }
>      }
> +    else /* len == 8 */
> +    {
> +        switch ( offset_aligned )
> +        {
> +        case DMAR_IRTA_REG:
> +            vvtd_set_reg_quad(vvtd, DMAR_IRTA_REG, val);
> +            ret = X86EMUL_OKAY;
> +            break;
> +
> +        default:
> +            ret = X86EMUL_UNHANDLEABLE;

Same here, you should not return X86EMUL_UNHANDLEABLE but instead
mimic what native hardware would do in such case.

> +            break;
> +        }
> +    }
>  
>      return ret;
>  }
> @@ -235,6 +305,9 @@ static int vvtd_create(struct domain *d, struct viommu 
> *viommu)
>      vvtd->length = viommu->length;
>      vvtd->domain = d;
>      vvtd->status = VIOMMU_STATUS_DEFAULT;
> +    vvtd->eim = 0;
> +    vvtd->irt = 0;
> +    vvtd->irt_max_entry = 0;

Maybe you should consider using xzalloc which will already initialize
this to 0.

Roger.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.