Re: [Xen-devel] [PATCH V3 27/29] x86/vvtd: Enable Queued Invalidation through GCMD

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

On Thu, Sep 21, 2017 at 11:02:08PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> 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  | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index e19b045..c69cd21 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -162,7 +162,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 745941c..55f7a46 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -496,6 +496,19 @@ static void vvtd_handle_gcmd_ire(struct vvtd *vvtd, 
> uint32_t val)
>      }
>  }
>  
> +static void vvtd_handle_gcmd_qie(struct vvtd *vvtd, uint32_t val)

I would use 'write' intead of 'handle', since this is only used by the
write path.

Also you should consider dropping the vvtd prefixes from the static
functions. It's quite clear they are vvtd related, and since they are
static there's no need to add such a prefix.

> +{
> +    vvtd_info("%sable Queue Invalidation", (val & DMA_GCMD_QIE) ? "En" : 
> "Dis");
> +
> +    if ( val & DMA_GCMD_QIE )
> +        vvtd_set_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_QIES_SHIFT);
> +    else
> +    {
> +        vvtd_set_reg_quad(vvtd, DMAR_IQH_REG, 0);
> +        vvtd_clear_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_QIES_SHIFT);
> +    }
> +}

Since I've seen this pattern in other functions, it might be worth
adding a helper that does:

VVTD_SET_BIT(reg, bit, val)
{
    if ( val )
        set_bit(...);
    else
        clear_bit(...);
}

Then the above function could be reduced to:

VVTD_SET_BIT(reg, bit, val);
if ( !val )
    vvtd_set_reg_quad(vvtd, DMAR_IQH_REG, 0);

I expect other functions can also be simplified by this macro.

Thanks, Roger.

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