Re: [PATCH v4 15/28] x86/vvtd: Enable Queued Invalidation through GCMD

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Chao Gao <chao.gao@intel.com>
Cc: Lan Tianyu <tianyu.lan@intel.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v4 15/28] x86/vvtd: Enable Queued Invalidation through GCMD
Date: Mon, 12 Feb 2018 14:04:46 +0000	[thread overview]
Message-ID: <20180212140446.ebus473ovkf7lpig@MacBook-Pro-de-Roger.local> (raw)
In-Reply-To: <1510899755-40237-16-git-send-email-chao.gao@intel.com>

On Fri, Nov 17, 2017 at 02:22:22PM +0800, Chao Gao wrote:
> Software writes to QIE field of GCMD to enable or disable queued
> invalidations. This patch emulates QIE field of GCMD.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  xen/drivers/passthrough/vtd/iommu.h |  3 ++-
>  xen/drivers/passthrough/vtd/vvtd.c  | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
> index dc2df75..b71dab8 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -160,7 +160,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_SHIFT     26
> +#define DMA_GSTS_QIES   (((u64)1) << DMA_GSTS_QIES_SHIFT)
>  #define DMA_GSTS_IRES_SHIFT     25
>  #define DMA_GSTS_IRES   (((u64)1) << DMA_GSTS_IRES_SHIFT)
>  #define DMA_GSTS_SIRTPS_SHIFT   24
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c b/xen/drivers/passthrough/vtd/vvtd.c
> index 83805d1..a2fa64a 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -539,6 +539,20 @@ static void write_gcmd_ire(struct vvtd *vvtd, uint32_t val)
>          (vvtd, DMAR_GSTS_REG, DMA_GSTS_IRES_SHIFT);
>  }
>  
> +static void write_gcmd_qie(struct vvtd *vvtd, uint32_t val)
> +{
> +    bool set = val & DMA_GCMD_QIE;
> +
> +    vvtd_info("%sable Queue Invalidation\n", set ? "En" : "Dis");
> +
> +    if ( set )
> +        vvtd_set_reg_quad(vvtd, DMAR_IQH_REG, 0);

If QIE is already enabled and the user writes to GCMD with the QIE bit
set won't this wrongly clear the invalidation queue?

> +
> +    (set ? vvtd_set_bit : vvtd_clear_bit)
> +        (vvtd, DMAR_GSTS_REG, DMA_GSTS_QIES_SHIFT);
> +
> +}
> +
>  static void write_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
>  {
>      uint64_t irta = vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG);
> @@ -598,6 +612,10 @@ static void vvtd_write_gcmd(struct vvtd *vvtd, uint32_t val)
>          write_gcmd_sirtp(vvtd, val);
>      if ( changed & DMA_GCMD_IRE )
>          write_gcmd_ire(vvtd, val);
> +    if ( changed & DMA_GCMD_QIE )
> +        write_gcmd_qie(vvtd, val);
> +    if ( changed & ~(DMA_GCMD_SIRTP | DMA_GCMD_IRE | DMA_GCMD_QIE) )
> +        vvtd_info("Only SIRTP, IRE, QIE in GCMD are handled");

This seems quite likely to go out of sync. I would rather do:

if ( changed & DMA_GCMD_QIE )
{
    write_gcmd_qie(vvtd, val);
    changed &= ~DMA_GCMD_QIE;
}
...
if ( changed )
    vvtd_info("Unhandled bit detected: %...");

It seems also quite likely this can be simplified with a macro:

#define HANDLE_GCMD_BIT(bit)        \
if ( changed & DMA_GCMD_ ## bit )   \
{                                   \
    write_gcmd_ ## bit (vvtd, val); \
    changed &= ~DMA_GCMD_ ## bit;   \
}

So that you can write:

HANDLE_GCMD_BIT(IRE);
HANDLE_GCMD_BIT(QIE);
...

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