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

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

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@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel