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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Feb 12, 2018 at 02:04:46PM +0000, Roger Pau Monné wrote:
>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?

No. If QIE is already enabled, writting to GCMD with QIE would be
ignored. write_gcmd_qie() is called only when QIE is changed in
vvtd_write_gcmd(). Actually, if we want to enable other features and not
want to disable QI, we should write to GCMD with the QIE bit set.

>
>> +
>> +    (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);
>...

Will use this macro.

Thanks
Chao

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