Re: [PATCH V3 13/29] x86/vvtd: Set Interrupt Remapping Table Pointer through GCMD

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Sep 21, 2017 at 11:01:54PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> 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@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> 
> ---
> v3:
>  - ignore unaligned r/w of vt-d hardware registers and return X86EMUL_OK
> ---
>  xen/drivers/passthrough/vtd/iommu.h | 12 ++++++-
>  xen/drivers/passthrough/vtd/vvtd.c  | 69 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
> index ef038c9..a0d5ec8 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -153,6 +153,8 @@
>  #define DMA_GCMD_IRE    (((u64)1) << 25)
>  #define DMA_GCMD_SIRTP  (((u64)1) << 24)
>  #define DMA_GCMD_CFI    (((u64)1) << 23)
> +/* mask of one-shot bits */
> +#define DMA_GCMD_ONE_SHOT_MASK 0x96ffffff 

Trailing white space.

>  
>  /* GSTS_REG */
>  #define DMA_GSTS_TES    (((u64)1) << 31)
> @@ -162,9 +164,17 @@
>  #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_SHIFT   24
> +#define DMA_GSTS_SIRTPS (((u64)1) << DMA_GSTS_SIRTPS_SHIFT)
>  #define DMA_GSTS_CFIS   (((u64)1) <<23)
>  
> +/* IRTA_REG */
> +/* The base of 4KB aligned interrupt remapping table */
> +#define DMA_IRTA_ADDR(val)      ((val) & ~0xfffULL)
> +/* The size of remapping table is 2^(x+1), where x is the size field in IRTA */
> +#define DMA_IRTA_S(val)         (val & 0xf)
> +#define DMA_IRTA_SIZE(val)      (1UL << (DMA_IRTA_S(val) + 1))
> +
>  /* 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 a3002c3..6736956 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -32,6 +32,13 @@
>  /* Supported capabilities by vvtd */
>  unsigned int vvtd_caps = VIOMMU_CAP_IRQ_REMAPPING;
>  
> +struct hvm_hw_vvtd_status {
> +    uint32_t eim_enabled : 1;

bool maybe?

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

If it's a gfn, use gfn_t as the type.

> +};
> +
>  union hvm_hw_vvtd_regs {
>      uint32_t data32[256];
>      uint64_t data64[128];
> @@ -43,6 +50,8 @@ struct vvtd {
>      uint64_t length;
>      /* Point back to the owner domain */
>      struct domain *domain;
> +
> +    struct hvm_hw_vvtd_status status;

Why you need a sub-struct for this, can't this just be placed inside
of hvm_hw_vvtd_regs directly?

>      union hvm_hw_vvtd_regs *regs;
>      struct page_info *regs_page;
>  };
> @@ -70,6 +79,11 @@ struct vvtd *domain_vvtd(struct domain *d)
>      return (d->viommu) ? d->viommu->priv : NULL;
>  }
>  
> +static inline void vvtd_set_bit(struct vvtd *vvtd, uint32_t reg, int nr)
> +{
> +    __set_bit(nr, &vvtd->regs->data32[reg/sizeof(uint32_t)]);
> +}
> +
>  static inline void vvtd_set_reg(struct vvtd *vtd, uint32_t reg, uint32_t value)
>  {
>      vtd->regs->data32[reg/sizeof(uint32_t)] = value;
> @@ -91,6 +105,44 @@ static inline uint64_t vvtd_get_reg_quad(struct vvtd *vtd, uint32_t reg)
>      return vtd->regs->data64[reg/sizeof(uint64_t)];
>  }
>  
> +static void vvtd_handle_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
> +{
> +    uint64_t irta = vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG);
> +
> +    if ( !(val & DMA_GCMD_SIRTP) )
> +        return;
> +
> +    vvtd->status.irt = DMA_IRTA_ADDR(irta) >> PAGE_SHIFT;
> +    vvtd->status.irt_max_entry = DMA_IRTA_SIZE(irta);
> +    vvtd->status.eim_enabled = !!(irta & IRTA_EIME);

If you use a bool you don't need the '!!'.

> +    vvtd_info("Update IR info (addr=%lx eim=%d size=%d).",

The final '.' is unneeded IMHO.

> +              vvtd->status.irt, vvtd->status.eim_enabled,
> +              vvtd->status.irt_max_entry);
> +    vvtd_set_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_SIRTPS_SHIFT);
> +}
> +
> +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 & DMA_GCMD_ONE_SHOT_MASK;   /* reset the one-shot bits */
> +    changed = orig ^ val;
> +
> +    if ( !changed )
> +        return X86EMUL_OKAY;
> +
> +    if ( changed & (changed - 1) )
> +        vvtd_info("Guest attempts to write %x to GCMD (current GSTS is %x)," 

Trailing white-space.

> +                  "it would lead to update multiple fields",

Also try to reduce the size of the message, so it can fit in a single
line.

> +                  val, orig);
> +
> +    if ( changed & DMA_GCMD_SIRTP )
> +        vvtd_handle_gcmd_sirtp(vvtd, val);
> +
> +    return X86EMUL_OKAY;
> +}
> +
>  static int vvtd_in_range(struct vcpu *v, unsigned long addr)
>  {
>      struct vvtd *vvtd = domain_vvtd(v->domain);
> @@ -135,12 +187,17 @@ static int vvtd_write(struct vcpu *v, unsigned long addr,
>      {
>          switch ( offset )
>          {
> +        case DMAR_GCMD_REG:
> +            return vvtd_write_gcmd(vvtd, val);
> +
>          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, val);
>              break;
>  
> @@ -148,6 +205,18 @@ static int vvtd_write(struct vcpu *v, unsigned long addr,
>              break;
>          }
>      }
> +    else /* len == 8 */
> +    {
> +        switch ( offset )
> +        {
> +        case DMAR_IRTA_REG:
> +            vvtd_set_reg_quad(vvtd, DMAR_IRTA_REG, val);

I have kind of a generic comment regarding the handlers in general,
which I will just make here. Don't you need some kind of locking to
prevent concurrent read/write accesses to the registers?

Also the 'if' to handle different sized accesses to the same registers
seems quite cumbersome. I would think there's a better way to handle
this with a single switch statement.

Thanks, Roger.

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