Re: [Xen-devel] [PATCH V2 18/25] x86/vvtd: Enable Interrupt Remapping through GCMD

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

On Wed, Aug 09, 2017 at 04:34:19PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> Software writes this field to enable/disable interrupt reampping. This patch
> emulate IRES 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  | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index 82bf6bc..e323352 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -192,7 +192,8 @@
>  #define DMA_GSTS_WBFS   (((u64)1) << 27)
>  #define DMA_GSTS_QIES_BIT       26
>  #define DMA_GSTS_QIES           (((u64)1) << DMA_GSTS_QIES_BIT)
> -#define DMA_GSTS_IRES   (((u64)1) <<25)
> +#define DMA_GSTS_IRES_BIT       25

_SHIFT.

> +#define DMA_GSTS_IRES   (((u64)1) << DMA_GSTS_IRES_BIT)
>  #define DMA_GSTS_SIRTPS_BIT     24
>  #define DMA_GSTS_SIRTPS (((u64)1) << DMA_GSTS_SIRTPS_BIT)
>  #define DMA_GSTS_CFIS   (((u64)1) <<23)
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c 
> b/xen/drivers/passthrough/vtd/vvtd.c
> index 470bc56..eae8f11 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -282,6 +282,25 @@ static int vvtd_handle_gcmd_qie(struct vvtd *vvtd, 
> uint32_t val)
>      return X86EMUL_OKAY;
>  }
>  
> +static int vvtd_handle_gcmd_ire(struct vvtd *vvtd, uint32_t val)
> +{
> +    VVTD_DEBUG(VVTD_DBG_RW, "%sable Interrupt Remapping.",
> +               (val & DMA_GCMD_IRE) ? "En" : "Dis");
> +
> +    if ( val & DMA_GCMD_IRE )
> +    {
> +        vvtd->status |= VIOMMU_STATUS_IRQ_REMAPPING_ENABLED;
> +        __vvtd_set_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_IRES_BIT);
> +    }
> +    else
> +    {
> +        vvtd->status |= ~VIOMMU_STATUS_IRQ_REMAPPING_ENABLED;

Hm, that's not correct, you are not clearing the bit here. It should
be '&=', not '|='.

> +        __vvtd_clear_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_IRES_BIT);
> +    }
> +
> +    return X86EMUL_OKAY;
> +}
> +
>  static int vvtd_handle_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
>  {
>      uint64_t irta;
> @@ -289,6 +308,10 @@ static int vvtd_handle_gcmd_sirtp(struct vvtd *vvtd, 
> uint32_t val)
>      if ( !(val & DMA_GCMD_SIRTP) )
>          return X86EMUL_OKAY;
>  
> +    if ( vvtd_irq_remapping_enabled(vvtd) )
> +        VVTD_DEBUG(VVTD_DBG_RW, "Update Interrupt Remapping Table when "
> +                   "active." );

Don't split console messages, instead add a newline and align to the
'('.

> +
>      vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG, irta);
>      vvtd->irt = DMA_IRTA_ADDR(irta) >> PAGE_SHIFT;
>      vvtd->irt_max_entry = DMA_IRTA_SIZE(irta);
> @@ -318,6 +341,10 @@ static int vvtd_write_gcmd(struct vvtd *vvtd, uint32_t 
> val)
>          vvtd_handle_gcmd_sirtp(vvtd, val);
>      if ( changed & DMA_GCMD_QIE )
>          vvtd_handle_gcmd_qie(vvtd, val);
> +    if ( changed & DMA_GCMD_IRE )
> +        vvtd_handle_gcmd_ire(vvtd, val);

Lost return value of vvtd_handle_gcmd_ire.

> +    if ( changed & ~(DMA_GCMD_QIE | DMA_GCMD_SIRTP | DMA_GCMD_IRE) )
> +        gdprintk(XENLOG_INFO, "Only QIE,SIRTP,IRE in GCMD_REG are 
> handled.\n");

Missing spaces between commas, and I think this should be a VVTD_DEBUG
in any case, although I'm not sure of it's usefulness.

Roger.

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