Re: [Xen-devel] [PATCH V2 17/25] x86/vvtd: Enable Queued Invalidation through GCMD

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

On Wed, Aug 09, 2017 at 04:34:18PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> Software writes to QIE fields of GCMD to enable or disable queued

fields or field?

> invalidations. This patch emulates QIE fields of GCMD.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  xen/drivers/passthrough/vtd/iommu.h |  3 ++-
>  xen/drivers/passthrough/vtd/vvtd.c  | 22 ++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index 70e64cf..82bf6bc 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -190,7 +190,8 @@
>  #define DMA_GSTS_FLS    (((u64)1) << 29)
>  #define DMA_GSTS_AFLS   (((u64)1) << 28)
>  #define DMA_GSTS_WBFS   (((u64)1) << 27)
> -#define DMA_GSTS_QIES   (((u64)1) <<26)
> +#define DMA_GSTS_QIES_BIT       26

_SHIFT.

> +#define DMA_GSTS_QIES           (((u64)1) << DMA_GSTS_QIES_BIT)
>  #define DMA_GSTS_IRES   (((u64)1) <<25)
>  #define DMA_GSTS_SIRTPS_BIT     24
>  #define DMA_GSTS_SIRTPS (((u64)1) << DMA_GSTS_SIRTPS_BIT)
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c 
> b/xen/drivers/passthrough/vtd/vvtd.c
> index 374fd88..470bc56 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -102,6 +102,11 @@ static inline void __vvtd_set_bit(struct vvtd *vvtd, 
> uint32_t reg, int nr)
>      return __set_bit(nr, (uint32_t *)&vvtd->regs->data[reg]);
>  }
>  
> +static inline void __vvtd_clear_bit(struct vvtd *vvtd, uint32_t reg, int nr)

No underscore prefixes please.

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

Unneeded return.

> +}
> +
>  static inline void vvtd_set_reg(struct vvtd *vtd, uint32_t reg,
>                                  uint32_t value)
>  {
> @@ -262,6 +267,21 @@ static int vvtd_record_fault(struct vvtd *vvtd,
>      return 0;
>  }
>  
> +static int vvtd_handle_gcmd_qie(struct vvtd *vvtd, uint32_t val)
> +{
> +    VVTD_DEBUG(VVTD_DBG_RW, "%sable Queue Invalidation.",
> +               (val & DMA_GCMD_QIE) ? "En" : "Dis");
> +
> +    if ( val & DMA_GCMD_QIE )
> +        __vvtd_set_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_QIES_BIT);
> +    else
> +    {
> +        vvtd_set_reg_quad(vvtd, DMAR_IQH_REG, 0ULL);

0 should be fine, no need for the ull suffix.

> +        __vvtd_clear_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_QIES_BIT);
> +    }

Newline.

> +    return X86EMUL_OKAY;
> +}
> +
>  static int vvtd_handle_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
>  {
>      uint64_t irta;
> @@ -296,6 +316,8 @@ static int vvtd_write_gcmd(struct vvtd *vvtd, uint32_t 
> val)
>  
>      if ( changed & DMA_GCMD_SIRTP )
>          vvtd_handle_gcmd_sirtp(vvtd, val);
> +    if ( changed & DMA_GCMD_QIE )
> +        vvtd_handle_gcmd_qie(vvtd, val);

You are losing the return value of vvtd_handle_gcmd_qie. So you either
make the function void or do something with the return value.

Roger.

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